Windows: tests fail with --incompatible_windows_native_test_wrapper

[laszlocsomor]

🐞 bug report

Is this a regression?

no

Description

On Windows, some tests fail with --incompatible_windows_native_test_wrapper.

🔬 Minimal Reproduction

PATH=c:\git\mingw64\bin;c:\msys64\usr\bin;c:\src\bazel-releases\current;c:\windows\system32

Tests pass with --noincompatible_windows_native_test_wrapper:

C:\src\tw-migrations\rules_nodejs>bazel --ignore_all_rc_files --output_user_root=c:/b test --noincompatible_windows_native_test_wrapper --experimental_allow_incremental_repository_updates --test_output=errors //internal/e2e/node:test
(...)
//internal/e2e/node:test                                        (cached) PASSED in 1.8s

But fail with --incompatible_windows_native_test_wrapper:

C:\src\tw-migrations\rules_nodejs>bazel --ignore_all_rc_files --output_user_root=c:/b test --incompatible_windows_native_test_wrapper --experimental_allow_incremental_repository_updates --test_output=errors //internal/e2e/node:test
(...)
INFO: From Testing //internal/e2e/node:test:
==================== Test output for //internal/e2e/node:test:
ERROR(tools/test/windows/tw.cc:1248) ERROR: src/main/native/windows/process.cc(184): CreateProcessW("C:\b\apk52u5o\execroot\build_bazel_rules_nodejs\bazel-out\x64_windows-fastbuild\bin\internal\e2e\node\test.sh"): %1 is not a valid Win32 application.

ERROR(tools/test/windows/tw.cc:1405) Failed to start test process (arg: C:\b\apk52u5o\execroot\build_bazel_rules_nodejs\bazel-out\x64_windows-fastbuild\bin\internal\e2e\node\test.sh)
================================================================================

The --incompatible_windows_native_test_wrapper flag forces Bazel to run the test script directly (with CreateProcess) instead of running through Bash.

Since test.sh is not a native executable, CreateProcess fails.

🌍 Your Environment

Operating System:

  
Windows 10
  

Output of bazel version:

  
Build label: 0.28.1
Build target: bazel-out/x64_windows-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Fri Jul 19 15:20:45 2019 (1563549645)
Build timestamp: 1563549645
Build timestamp as int: 1563549645
  

Rules version (SHA):

Anything else relevant?

This is blocking bazelbuild/bazel#6622, which I really want to ship with Bazel 1.0 (September 2019 release).

[laszlocsomor]

Complete list of failed tests with this flag:

"Artifacts" tab on https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1109#25b73074-7ffd-4925-8591-26da21731703

[laszlocsomor]

/cc @alexeagle

[laszlocsomor]

ping

[alexeagle]

Let me see if we can just remove this shell wrapping of nodejs_binary and nodejs_test - I added these for Windows initially, and they cause pain as macros usually do, eg by not forwarding the right attrs to the right underlying rules.

#791

[laszlocsomor]

Thanks!

If the rules return [DefaultInfo(... executable=foo)] where foo is a shell script, then you will need a wrapping sh_binary or sh_test though.

[laszlocsomor]

Hey @alexeagle , any update on this?

[alexeagle]

@meteorcloudy could you help out with this one? My #791 to remove the sh wrapper needs some windows help. Then I'm also not sure whether that change actually fixes this problem.

[laszlocsomor]

Alex, you don't need to remove the sh wrapper. It's probably easiest to wrap the generated .sh file in a sh_test rule. That's perfectly fine and will fix this problem.

Here's an example where I did exactly that: bazelbuild/bazel@77b9875
This commit turned custom test rules into file-generating rules, and add a macro around them that also instantiated a native.sh_test with the generated source.

[laszlocsomor]

Sorry, I realized that all the while i thought your goal was to remove the generated .sh file.

But I'm wondering, what problems do the sh_test / sh_binary wrappers cause?
#791 fixes two bugs:

• jasmine_node_test should not need an sh_binary wrapper macro for nodejs_test #51, but this does not give any reason
• nodejs_test seems to be ignoring the timeout attribute #733, which is easy to fix if you pass the macro's **kwargs to the test (just be mindful of attributes that tests can and cannot have, see bazelbuild/bazel@6d0b14b)

[meteorcloudy]

I think generally we don't want MSYS as a dependency for Angular on Windows, so it's better to remove the .sh wrapper if we could.

[meteorcloudy]

@alexeagle I took a closer look into #791, it won't fix this issue. But the nodejs_test_macro and nodejs_binary_macro can actually help us solve this issue.

The reason some tests are failing with --incompatible_windows_native_test_wrapper is because jasmine_node_test is using nodejs_test instead of nodejs_test_macro.
https://github.com/bazelbuild/rules_nodejs/blob/14a0b6be0d0597c711702aa597e948f29612d765/packages/jasmine/src/jasmine_node_test.bzl#L22

I'll send a fix for this.

Eventually, we want get rid of Bash dependency. But that requires us to find a replacement for node_launcher.sh.

[meteorcloudy]

Close via #1034