Flip use_default_shell_env to True in run_node ctx.actions.run

[gregmagolan]

Once bazelbuild/bazel#5980 is resolved and use_default_shell_env=True can be used without overwritting env{} (which we rely on to set the COMPILATION_MODE env for build actions).

This will allow passing over environment variables to npm_package_bin build actions via --action_env.

See https://docs.bazel.build/versions/master/skylark/lib/actions.html#run for more info on use_default_shell_env.

See https://docs.bazel.build/versions/master/skylark/lib/configuration.html#default_shell_env for more info on ctx.configuration.default_shell_env.

It looks like the intention of the Bazel team is to flip the use_default_shell_env default to true: see https://bazel.build/designs/2016/06/21/environment.html#new-flag---action_env.

A possible (but less than ideal) workaround is to set https://docs.bazel.build/versions/master/command-line-reference.html#flag--distinct_host_configuration to False and don't use env{} to pass COMPILATION_MODE to build actions. But a distinct host configuration is desirable so we should keep using env{} and wait until the #5890 is resolved and use_default_shell_env=True can be used without overwritting env{}

[gregmagolan]

Related issues:
bazelbuild/bazel#4008
bazelbuild/bazel#6473
bazelbuild/bazel#4643

[thesayyn]

#1648 We have this problem there too.

[github-actions]

This issue has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

[jiridanek]

According to the relevant design docs, --action_env was intended to be an exhaustive list of environment variables passed into actions with use_default_shell_env=True.

In every action executed with use_default_shell_env being true, precisely the environment variables specified by --action_env options are set as the default environment. (Note that, therefore, by default, the environment for actions is empty.)

https://bazel.build/designs/2016/06/21/environment.html#new-flag---action_env

[nagelp-bosch]

Is this still being considered? IMO defaulting to true would make more sense.

We had issues with false positive cache hits because someone used actions.run_shell() and hadn't set use_default_shell_env=True. He was not aware that an environment variable his shell script depended on wouldn't be available/considered because it's available in pretty much any other action, and he assumed it would be in run/run_shell, too. So other actions reacted to a changed value of that environment variable, but his shell script didn't, because Bazel arrived at the same action hash and returned (unexpected) cached data.

Until this is implemented we need to catch the use of actions.run_shell() or actions.run() without use_default_shell_env=True to avoid people shooting themselves in the foot again. Any advice on how such a check could be implemented? Is there something easier than extending buildifier?

[jbedard]

I don't think this is still applicable for rules_nodejs since the run_node call no longer exists in this repo.

This is being considered as opt-in in rules_js (aspect-build/rules_js#1303) and has initial use with lifecycle hooks (aspect-build/rules_js#1489). However it should never be enabled by default due to the cache-busting effects it has.