Pass use_default_shell_env = True for protoc

[jesseschalken]

If protoc is compiled with MinGW it will depend on libstdc++-6.dll which is found in C:\msys64\mingw64\bin and can only be found by inheriting PATH from the environment.

Fixes #8983

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jesseschalken / name: Jesse Schalken (566958c)

[sanjaypujare]

There are 2 failures in the "Linux artifacts" CI. I understand the first one is a flake but the second one?

io.grpc.CallOptionsTest > withDeadlineAfter FAILED
    expected               : 59.979414243s from now
    but was                : 59.966459425s from now
    outside tolerance in ns: 10000000
        at io.grpc.CallOptionsTest.withDeadlineAfter(CallOptionsTest.java:141)

> Task :grpc-core:checkstyleTest
> Task :grpc-rls:compileTestJava
> Task :grpc-rls:testClasses

> Task :grpc-benchmarks:test

io.grpc.benchmarks.driver.LoadWorkerTest > runUnaryBlockingClosedLoop FAILED
    java.lang.AssertionError
        at org.junit.Assert.fail(Assert.java:86)
        at org.junit.Assert.assertTrue(Assert.java:41)
        at org.junit.Assert.assertTrue(Assert.java:52)
        at io.grpc.benchmarks.driver.LoadWorkerTest.assertWorkOccurred(LoadWorkerTest.java:198)
        at io.grpc.benchmarks.driver.LoadWorkerTest.runUnaryBlockingClosedLoop(LoadWorkerTest.java:90)

[jesseschalken]

@sanjaypujare I'll see if I can reproduce it locally.

[sanjaypujare]

@sanjaypujare I'll see if I can reproduce it locally.

Thanks. Most probably the failure is not related to your change.

Also looking at https://groups.google.com/g/bazel-discuss/c/7B0FaU5Pkmo/m/6uvsIs2oAQAJ which says

"Why do you need PATH inside the action?

If you intend to use a tool in the action, the tool should be an input of the action, otherwise Bazel doesn't know about the dependency and cannot guarantee build correctness."

Based on this is there an alternative fix for what you want to achieve?

[jesseschalken]

If you intend to use a tool in the action, the tool should be an input of the action, otherwise Bazel doesn't know about the dependency and cannot guarantee build correctness.

In general yes but Bazel's default system toolchains are not fully hermetic. The system compiler, headers and runtime libraries exist outside of Bazel's build graph. As expected this sometimes requires clearing Bazel's cache when those things change. Setting up a hermetic toolchain is an effort that Bazel users are in general not required to pursue.

For MSVC the dynamically linked protoc works because the MSVC runtime libraries are installed to C:\Windows\System32 which Windows will search for DLLs by default even with an empty PATH, but MinGW does not install its DLLs there and thus requires C:\msys64\mingw64\bin to be in PATH.

[sanjaypujare]

...
For MSVC the dynamically linked protoc works because the MSVC runtime libraries are installed to C:\Windows\System32 which Windows will search for DLLs by default even with an empty PATH, but MinGW does not install its DLLs there and thus requires C:\msys64\mingw64\bin to be in PATH.

CC @ejona86 - whether to accept this change to support MinGW.

[ejona86]

The test failures are unrelated. This change could only impact the Bazel CI.

Hmm... I don't see good docs surrounding this. It isn't clear what the environment is when use_default_shell_env=False nor when it is appropriate to use use_default_shell_env=True. And with bazelbuild/bazel#5980, it makes me uncomfortable and seems like "this is not normal."

This seems like a very basic issue. If Bazel has to have this to execute binaries it builds, it seems that should be documented somewhere and would impact a lot (including bazel run).

I do see proto_library swapped to use_default_shell_env = True, and that commit is woefully unhelpful. bazelbuild/rules_go#3025 does provide a bit more info. I guess this is safe since the common proto infra is doing it.

Although, this actually exposes that it seems the proto_common stuff is far enough along now that we don't have to execute protoc ourselves and we can tie into the common infra. I hadn't realized that the proto swap to bzl had finally happened, although it did happen pretty recently and hasn't been released yet. That deserves playing around with the rule some more later.

[jesseschalken]

This seems like a very basic issue. If Bazel has to have this to execute binaries it builds, it seems that should be documented somewhere and would impact a lot (including bazel run).

It is a basic issue. I would expect a simple cc_binary and simple ctx.actions.run running it to work without any special consideration for MinGW, but alas it does not.

Someone more familiar with Bazel's C++ toolchains might be able to suggest a solution at the toolchain level, such as copying libstdc++-6.dll next to the output binary, but at present binaries produced by the Bazel 5.0.0 MinGW toochain require C:\msys64\mingw64\bin in PATH.

[sanjaypujare]

...
Someone more familiar with Bazel's C++ toolchains might be able to suggest a solution at the toolchain level, such as copying libstdc++-6.dll next to the output binary, but at present binaries produced by the Bazel 5.0.0 MinGW toochain require C:\msys64\mingw64\bin in PATH.

May be open an issue in https://github.com/bazelbuild/rules_cc to follow up on this?

[jesseschalken]

May be open an issue in https://github.com/bazelbuild/rules_cc to follow up on this?

bazelbuild/bazel#15059

(I wasn't sure it would get any visibility on rules_cc as it looks a bit dormant)

[sanjaypujare]

Tests failed again:

github/grpc-java/api/build/test-results/test/TEST-io.grpc.CallOptionsTest
Failed
11:45:31 AM
14:13
github/grpc-java/benchmarks/build/test-results/test/TEST-io.grpc.benchmarks.driver.LoadWorkerTest
Failed
11:45:31 AM
14:13

I'll try this one more time.

[sanjaypujare]

LGTM

[ejona86]

Apparently, rules_go doesn't actually use proto_common stuff (i.e., create_proto_compile_action). I guess the test just used proto_common indirectly through proto_library and the test failed. But the rules themselves would have been unimpacted.

FWIW, rules_go manages Windows a bit differently, and I'm not convinced it is better:
https://github.com/bazelbuild/rules_go/blob/8553d97674742ad4c110cffa999017db21052b97/proto/compiler.bzl#L126-L130