[native_assets_cli] Protocol environment variables

[dcharkes]

Leaning towards passing in all environment variables, and reinvoking the hooks on any environment variable change.

• This needs to be documented somewhere.

Problem

With what environment variables should we invoke build.dart?

build.dart is invoked with a BuildConfig (which is read from args/env/file), but the CLI itself most likely invokes other processes (for example a C compiler). These processes are influenced by which environment variables are set.

Minimum env variables

Let's say we get rust_compiler package that tries to find rustc on PATH, then we must at least pass through PATH on our build.dart invocation.

On Windows, various tools rely on a host of other environment variables as well:

• SYSTEMROOT is required for cl.exe from MSVC
• TMP and TEMP are required for cl.exe in some cases in order to be able to write temporary files.

So the minimum list of environment variables we should pass through are the ones mentioned above.

Maximum environment variables

Conversely, there are also environment variables which are conflicting with the Dart ecosystem and native tooling.

• SDKROOT is set by tools/test.py which breaks xcodebuild invocations as it interprets the SDKROOT as a path for the iOS or MacOS SDK rather than the Dart SDK.

Caching correctness and cache misses

If any of the environment variables influences the build, it could throw off caching of builds:

• If we ignore all environment variables when determining whether a cache can be reused, we might accidentally not rebuild leading to stale builds. (Currently the only way for a user to fix that is to find the folder in which the build has happened and delete it.)
• If we take all environment variables into account when determining if a cache can be reused, we might throw away build caches too often.

Possible solutions

We could consider listing the environment variables we pass through, making an allow list.
Or we could consider listing the environment variables we use for caching, but passing through all except for a deny list.

1. SDK: Pass through everything except deny list + use these for caching

Pros:

• Correct caching
• Users using new environment variables not blocked

Cons:

• We might throw out valid cached for spurious environment changes.

(We should probably change SDKROOT to DARTSDKROOT in test.py and it's users.)

Even when implementing the env-filtering in the Dart and Flutter SDK, we should probably make the deny list (or allow list) part of this package, so that this package contains the whole protocol spec.

2. SDK: Pass through only an allow list + use these for caching

Pros:

• Correct caching
• Efficient caching

Cons:

• If users want rely on a new environment variable, they need make an SDK PR and wait for the next Dart SDK release.

3. SDK: Pass through everything except for deny list + use allowlist for caching

Pros:

• Users using new environment variables not blocked
• Efficient caching

Cons:

• Potentially incorrect caching. Users would need to make a PR to the Dart SDK to make it work.

4. package:native_assets_cli solution

Always just invoke build.dart with all environment variables, but add the allowList or denyList to package:native_assets_cli.

There would be an .environment getter on BuildConfig likely.

Pro:

• Updating allowList/denyList does not require Dart SDK / Flutter updates.
• We could potentially Isolate.spawnUri to run build rather than Process.run (but that would mean no exit(int) calling in any build.dart script, and tougher handling of unhandled exceptions etc.) [native_assets_cli] Protocol exit codes / unhandled exceptions #33

Con:

• Every process invocation in build.dart requires explicitly passing environment and includeParentEnvironment: false.
• Incorrect caching until that version of native_assets_cli rolls into the Dart SDK.

Maybe there are solutions I haven't thought about yet. I'm open to suggestions. (Otherwise I might lean towards solution 1 or 2.)

internal design doc with some initial discussion about the env vars

cc @mkustermann @liamappelbe @HosseinYousefi

[dcharkes]

We should strongly consider having

• includeParentEnvironment: false
• have a minimal environment for the subprocess
• allow embedder to add extra environment variables

This avoids hooks to accidentally depend on things they shouldn't and would be a step towards a more hermetic/sandboxed build.

(Environment variables may expose also secrets - e.g. various access tokens / api keys / ... that we may not want to blindly pass to all the hook invocations)

Originally posted by @mkustermann in #1759 (comment)

Let's take this discussion to where we already have context.

• includeParentEnvironment: false
• have a minimal environment for the subprocess

I've been playing around a bit with restricting the environment variables:

• [native_assets_builder] Don't pass in the whole environment #1764

  static const _environmentVariablesFilter = {
    'ANDROID_HOME',
    'PATH',
    'PROGRAMDATA',
    'SYSTEMROOT',
    'TEMP',
    'TMP',
  };

That's the list of env vars needed to pass the tests on the CI which invoke clang, msvc, and the NDK.

Invalidating caching based on the full set of env vars seems overly cautious, as those might change often @mosuem

For example, these can cause a rebuild when not needed:

CHOCOLATEYLASTPATHUPDATE 133062298470192620
VSCODE_GIT_IPC_HANDLE \\.\pipe\vscode-git-db44fa1249-sock

This would probably cause a rebuild when restarting vscode, and a rebuild when updating (unrelated) packages.

• allow embedder to add extra environment variables

@mkustermann The embedder? Or the user? Probably the user should be able to that, otherwise it would still require PRs on the embedder. But I'm slightly worried that that will make the CLI for dart run // flutter run etc quite nasty. Listing out a bunch of allowed environment variable keys.

If we want to allowlist, how much should we add?

  static const _environmentVariablesFilter = {
    // minimum to pass testing
    'ANDROID_HOME',
    'PATH',
    'PROGRAMDATA',
    'SYSTEMROOT',
    'TEMP',
    'TMP',
    // used in our own code to locate compilers (not exercised on GitHub CI)
    'HOME',
    'USER_PROFILE',
    // more of the same
    'ANDROID_AVD_HOME',
    'HOMEDRIVE',
    'HOMEPATH',
    'SYSTEMDRIVE',
    'SYSTEMROOT',
    'TMPDIR', // Otherwise Directory.systemTemp defaults to `/tmp` on Unix / MacOS
    'USER',
    'USERNAME',
    // flutter already does code signing, so we could skip?
    'CODE_SIGN_IDENTITY', 
    // more access to things on Windows, who knows what things on Windows depend on.
    'ALLUSERSPROFILE',
    'APPDATA',
    'COMMONPROGRAMFILES',
    'COMMONPROGRAMFILES(X86)',
    'COMMONPROGRAMW6432',
    'LOCALAPPDATA',
    'PROGRAMDATA',
    'PROGRAMFILES',
    'PROGRAMFILES(X86)',
    'PROGRAMW6432',
    'WINDIR',
    // other compilers
    'EMSDK_NODE',
    'EMSDK_PYTHON',
    'EMSDK',
    'NVM_BIN',
    'NVM_CD_FLAGS',
    'NVM_DIR',
    'NVM_INC',
    // access to the Dart and Flutter SDK?
    'DART_SDK_PATH',
    'DART',
    'FLUTTER_ROOT',
  };

We probably want to pass in TMP, TEMP, and TMPDIR but not invalidate the cache for these. The temp-dirs don't actually seem to change on MacOS/Windows, and Linux doesn't have one set by default.