[native_assets] Process.start hangs when executed in build method on Windows.

[MichealReed]

Reproduction:

1. Navigate to any native assets example.
2. From the root of the example directory git clone https://github.com/google/dawn.git
3. At the top of the hooks/build.dart build method add.
4. If you kill the process with CTRL + C and repeat, you will see many detached processes like cl.exe, msbuild.exe, and cmake.exe start to accumulate, but that may relate to github.com/Child processes remain after parent termination on Windows sdk#49234.

    final sourceDir = Directory('./dawn');

    final process = await Process.start("cmake", [
      "--log-level",
      "TRACE",
      "-S",
      ".",
      "-B",
      "build_test",
      "-Wno-dev",
    ], workingDirectory: sourceDir.path);

    process.stdout.transform(utf8.decoder).listen((data) {
      stderr.writeln(data);
    });
    process.stderr.transform(utf8.decoder).listen((data) {
      stderr.writeln(data);
    });

    await process.exitCode;

Now in a isolate directory,

1. git clone https://github.com/google/dawn.git (or another large cmake project)
2. add a file to the root of the dawn directory build_dawn.dart
3. dart -v run build_dawn.dart with the code below

// build_dawn.dart
import "dart:io";
void main(List<String> arguments) {
    final process = await Process.run('cmake', [
    '--log-level',
    'TRACE',
    '-S',
    '.',
    '-B',
    'build',
    '-Wno-dev',
  ]);
  process.stdout.transform(utf8.decoder).listen((data) {
    stdout.writeln(data);
  });
  await process.exitCode;
}

The process will not get stuck and will complete.

Any ideas why Process.start would work differently in the native_assets build hook on Windows?

[dcharkes]

It works for clang and MSVCs cl.exe in package:native_toolchain_c, so it must be specific to cmake.exe.

One thing that's different in the hooks is that they only get a subset of the environment variables Platform.environment. Could you try to see if you run your script and you don't pass the parent environment and only those environment variables that CMake also gets stuck or not?

(See the discussion on #32 about why we've chosen to hide most of the environment variables from the hooks.)

[rainyl]

One thing that's different in the hooks is that they only get a subset of the environment variables Platform.environment

I can confirm it's the problem of environment variables.

In my computer, the Platform.environment in build.dart is:

{
  "COMSPEC": "C:\\WINDOWS\\SYSTEM32\\cmd.exe",
  "PATH": XXX,
  "PATHEXT": ".COM;.EXE;.BAT;.CMD;.VBS;.JS;.WS;.MSC",
  "PROGRAMDATA": "C:\\ProgramData",
  "PROMPT": "$P$G",
  "SYSTEMROOT": "C:\\WINDOWS",
  "TEMP": "C:\\Users\\rainy\\AppData\\Local\\Temp",
  "TMP": "C:\\Users\\rainy\\AppData\\Local\\Temp"
}

with some debugging, I found that appending "WINDIR": r"C:\WINDOWS" can solve this, you can manually copy Platform.environment to a modifiable Map and add WINDIR to test it.

final Map<String, String> env  = Map.from(Platform.environment);
env["WINDIR"] = r"C:\WINDOWS";
final result = await runProcess(
  executable: cm!.uri,
  arguments: [
    "-S",
    sourceDir.toFilePath(),
    "-B",
    dstDir.toFilePath(),
  ],
  logger: logger,
  environment: env,
);
assert(result.exitCode == 0);

May I ask why WINDIR is hidden in Native-Assets? This variable is unlikely to be changed frequently so won't affect the cache system.

[MichealReed]

It works for clang and MSVCs cl.exe in package:native_toolchain_c, so it must be specific to cmake.exe.

One thing that's different in the hooks is that they only get a subset of the environment variables Platform.environment. Could you try to see if you run your script and you don't pass the parent environment and only those environment variables that CMake also gets stuck or not?

(See the discussion on #32 about why we've chosen to hide most of the environment variables from the hooks.)

Did #1764 get reverted somehow? I do not see _filteredEnvironment in main but I see it here

native/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart

Line 436 in 44b1dc4

_filteredEnvironment(_environmentVariablesFilter),

.

[MichealReed]

May I ask why WINDIR is hidden in Native-Assets? This variable is unlikely to be changed frequently so won't affect the cache system.

Adding WINDIR does seem to prevent the freeze, but I think we are going to run into a mess of compile issues if the toolchain defaults are not used.

CMake Error at CMakeLists.txt:37 (project):

  No CMAKE_C_COMPILER could be found.




CMake Error at CMakeLists.txt:37 (project):

  No CMAKE_CXX_COMPILER could be found.

Map.from(Platform.environment); is missing many environment variables for me on Windows so we cannot pass that.

    Map<String, String> env = Map.from(Platform.environment);
    env["WINDIR"] = r"C:\WINDOWS";
    for (var key in env.keys) {
      stderr.writeln('$key: ${env[key]}');
    }
    final toolchain = String.fromEnvironment("CMAKE_TOOLCHAIN_FILE");

I don't think there's a way for us to get the CMAKE_TOOLCHAIN_FILE env variable.

[rainyl]

Oh, i forgot to attach it, too late last night.

Try to also add 'SYSTEMDRIVE' with value of 'C:' and it will find c compiler in my condition.

[rainyl]

I am not sure why but it seems some variables are necessary for cmake, for the exact reasons, we need to dig into the source code of cmake.

So, what's the solution?

Adding the above 2 vars to allowed list seems to work, however, what if more variables are required in the future by other building systems or languages?

Personally i think it's inflexible to hide most environment variables and there is no way to get them when called by native assets, it's also break the consistency of Platform.environment.

Maybe we can use another variable like buildEnvrionment or assetsEnvironment for the purpose in #32 , or at least provide the original not filtered Platform.environment like Platform.environmentFull or Platform.environmentNotFiltered, we can discuss more about it.

[MichealReed]

Platform.environmentFull or Platform.environmentNotFiltered, we can discuss more about it.

Seems like a great suggestion, maybe runProcess should become runBuildProcess and required in build hooks?

https://github.com/dart-lang/native/blob/main/pkgs/native_toolchain_c/lib/src/utils/run_process.dart

This could solve the problem of piping logs from child processes as well as the environment variable issue here.

[dcharkes]

Please make a PR to

native/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart

Line 426 in 318f841

static const hookEnvironmentVariablesFilter = {

with the required env vars for now. That will unblock you for now.

Personally i think it's inflexible to hide most environment variables and there is no way to get them when called by native assets, it's also break the consistency of Platform.environment.

Maybe we can use another variable like buildEnvrionment or assetsEnvironment for the purpose in #32 , or at least provide the original not filtered Platform.environment like Platform.environmentFull or Platform.environmentNotFiltered, we can discuss more about it.

Agreed that this is not ideal. Because opening up later would be non-breaking, while closing down later would be breaking, is why we decided to limit the environment variables for to start with. We can restart this conversation later on #32, once we have a longer list of environment variables that are needed for different use cases, and discuss a way on how to get access to them.

[rainyl]

I have submitted a PR, thanks for your suggestions 😄

Agreed that this is not ideal. Because opening up later would be non-breaking, while closing down later would be breaking, is why we decided to limit the environment variables for to start with. We can restart this conversation later on #32, once we have a longer list of environment variables that are needed for different use cases, and discuss a way on how to get access to them.

Agree, the current solution is acceptable.