Flip --experimental_strict_action_env on by default. #6263

benjaminp · 2018-09-27T05:16:41Z

See #2574. For a while, many actions did not respect the --action_env command line option, but those have been fixed now. So, I think it's time to flip on this flag for greater hermeticity by default.

buchgr · 2018-09-28T12:24:32Z

src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java

@@ -111,7 +111,7 @@
  public static class StrictActionEnvOptions extends FragmentOptions {
    @Option(
        name = "experimental_strict_action_env",


if enabled by default, please remove the "experimental prefix" and set the oldName to experimental_strict_action_env.

I think we just want this flag to go away. If you want something like the old behavior, the correct thing to do is to pass --action_env=PATH and --action_env=LD_LIBRARY_PATH. --noexperimental_strict_action_env does some universally bad things like reading from the server environment rather than the client environment.

You are right. I was under the impression that without --experimental_strict_action_env all shell environment variables are inherited by the action environment but I was wrong there.

buchgr · 2018-09-28T12:26:13Z

Overall looks good. Can you please add a RELNOTES: tag describing the impact of this change too? That way it will show up in our release notes!

@laurentlb for guidance on whether this is compatible with the incompatible flags policy. We want to flip the default value of a flag. Is that ok?

laurentlb · 2018-09-28T13:28:35Z

Is it a breaking change? Is it likely to break users?
If it's a breaking change, it should have ideally be named with --incompatible_. But renaming the flag now can be annoying for people using it.

Having RELNOTES is definitely useful.

buchgr · 2018-09-28T13:31:56Z

Is it a breaking change? Is it likely to break users?

The change is that we no longer pass all environment variables to command execution. So it will break people who depend on this. Such users can change the --strict_action_env flag back to false though. We are not planning on removing this flag any time soon, but just to switch it on by default.

laurentlb · 2018-09-28T14:41:59Z

Can we discuss this on bazel-dev@googlegroups.com?

jin · 2018-09-28T14:42:31Z

This seems like a pretty big breaking change, and we don't know the scope of breakage flipping this flag will bring. Even though it's reversible, it's still adding maintenance burden to our end users. Could we discuss this on bazel-discuss or bazel-dev for a start?

buchgr · 2018-10-01T11:32:01Z

https://mail.google.com/mail/u/0/#sent/QgrcJHrnwhDnndGnMTVVHtBTxlfTdghGTZg

benjaminp · 2018-10-01T20:29:55Z

Thank you for the reviews and sending that email, Jakob. Question about RELNOTES: Does that have to be in a commit message or is editing the PR description here sufficient?

buchgr · 2018-10-02T14:35:25Z

@benjaminp it needs to be in the commit message. Thanks!

See bazelbuild#2574. For a while, many actions did not respect the --action_env command line option, but those have been fixed now. So, I think it's time to flip on this flag for greater hermeticity by default. RELNOTES: The --experimental_strict_action_env option is now on by default. This means Bazel will no longer use the client's PATH and LD_LIBRARY_PATH environmental variables in the default action environment. If the old behavior is desired, pass --action_env=PATH and --action_env=LD_LIBRARY_PATH. --noexperimental_strict_action_env will also temporarily restore the old behavior. However, as --action_env is a more general and explicit way to pass client environmental variables into actions, --noexperimental_strict_action_env will eventually be deprecated and removed.

benjaminp · 2018-10-04T02:24:54Z

I've added RELNOTES to the commit message.

buchgr · 2018-10-04T17:21:42Z

LGTM!

ulfjack · 2018-10-11T03:12:41Z

This only affects two environment variables (but they are admittedly pretty important).

EdSchouten · 2018-10-11T07:55:49Z

Could it be the case that this option still makes odd decisions when doing cross remote builds? We currently have a remote build cluster for which we use the following ~/.bazelrc options:

# From bazel-toolchains/configs/debian8_clang/0.4.0/toolchain.bazelrc.
build:bbbdev-debian8 --action_env=BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1
build:bbbdev-debian8 --crosstool_top=@bazel_toolchains//configs/debian8_clang/0.4.0/bazel_0.17.1/default:toolchain
build:bbbdev-debian8 --extra_execution_platforms=@bazel_toolchains//configs/debian8_clang/0.4.0:rbe_debian8
build:bbbdev-debian8 --extra_toolchains=@bazel_toolchains//configs/debian8_clang/0.4.0/bazel_0.17.1/cpp:cc-toolchain-clang-x86_64-default
build:bbbdev-debian8 --host_javabase=@bazel_toolchains//configs/debian8_clang/0.4.0:jdk8
build:bbbdev-debian8 --host_java_toolchain=@bazel_tools//tools/jdk:toolchain_hostjdk8
build:bbbdev-debian8 --host_platform=@bazel_toolchains//configs/debian8_clang/0.4.0:rbe_debian8
build:bbbdev-debian8 --javabase=@bazel_toolchains//configs/debian8_clang/0.4.0:jdk8
build:bbbdev-debian8 --java_toolchain=@bazel_tools//tools/jdk:toolchain_hostjdk8
build:bbbdev-debian8 --platforms=@bazel_toolchains//configs/debian8_clang/0.4.0:rbe_debian8
# Specific to our company.
build:bbbdev-debian8 --cpu=k8
build:bbbdev-debian8 --experimental_strict_action_env
build:bbbdev-debian8 --genrule_strategy=remote
build:bbbdev-debian8 --host_cpu=k8
build:bbbdev-debian8 --jobs=8
build:bbbdev-debian8 --remote_executor=<redacted>
build:bbbdev-debian8 --remote_instance_name=debian8
build:bbbdev-debian8 --spawn_strategy=remote
build:bbbdev-debian8 --strategy=Closure=remote
build:bbbdev-debian8 --strategy=Javac=remote

This configuration seems to be sufficient when doing builds from a Linux client on this Debian 8 based cluster. However, when doing builds from a Windows client, we see that PATH is set to something specific to Windows, which ends up causing our builds to fail. This is why we currently need the following flag in addition to --experimental_strict_action_env:

build:bbbdev-debian8 --action_env=PATH=/bin:/usr/bin

EdSchouten · 2018-10-11T09:13:50Z

Ah. Looks like this is already documented as a TODO in BazelRuleClassProvider.java:

// TODO(ulfjack): The default PATH should be set from the exec platform, which may be different
// from the local machine. For now, this can be overridden with --action_env=PATH=<value>, so
// at least there's a workaround.

Carry on!

benjaminp · 2018-10-11T14:14:34Z

It's probably worth filing a formal issue for that problem, too.

Globegitter · 2018-10-27T06:26:55Z

Anything blocking this from getting merged now? Would be nice to get this into the 0.20 release.

buchgr · 2018-10-31T23:28:55Z

I ll merge it when I am back in the office on Monday!

buchgr · 2018-11-08T16:17:17Z

merging in progress!

The `SassCompiler` action calls a Bash script which in turn assumes `grep`, `cut`, `dirname` and other commands to be available in the `PATH`. But we can't rely on that to be true without `use_default_shell_env = True`. That's because without this flag, shell scripts use Bash's default `PATH` (since `/usr/bin/env -` clears the entire environment). That default `PATH` will be different on different platforms, and is not guaranteed to contain all the shell commands this script expects to be able to call. Indeed, on NixOS the default `PATH` for Bash is `/no-such-path`. If turning on `use_default_shell_env` makes your nervous, it shouldn't. According to bazelbuild/bazel#6263, `--experimental_strict_action_env` will become the default in Bazel v0.20. This means that the default env will be restricted, standard and small. (So this shouldn't hurt hermeticity - actually it should help it.)

benjaminp requested a review from tualeron as a code owner September 27, 2018 05:16

googlebot added the cla: yes label Sep 27, 2018

benjaminp force-pushed the strict-action-env-by-default branch from 2ab5e0a to d744b39 Compare September 27, 2018 06:37

iirina added the team-Execution label Sep 27, 2018

iirina requested a review from buchgr September 27, 2018 14:55

benjaminp force-pushed the strict-action-env-by-default branch 4 times, most recently from d81673c to e6b4c9d Compare September 27, 2018 19:09

buchgr reviewed Sep 28, 2018

View reviewed changes

benjaminp force-pushed the strict-action-env-by-default branch from e6b4c9d to 7337e5c Compare October 2, 2018 15:42

EdSchouten mentioned this pull request Oct 15, 2018

experimental_strict_action_env doesn't do the right thing with remote builds #6392

Open

buchgr mentioned this pull request Oct 31, 2018

Bazel doesn't store java tests in remote cache #6457

Closed

This was referenced Nov 4, 2018

Set use_default_shell_env in rollup example bazel-contrib/rules_nodejs#407

Closed

Set use_default_shell_env bazelbuild/rules_sass#59

Merged

buchgr mentioned this pull request Nov 10, 2018

--incompatible_strict_action_env #6648

Closed

bazel-io closed this in 3bf6857 Nov 19, 2018

benjaminp deleted the strict-action-env-by-default branch November 20, 2018 03:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flip --experimental_strict_action_env on by default. #6263

Flip --experimental_strict_action_env on by default. #6263

benjaminp commented Sep 27, 2018

buchgr Sep 28, 2018

benjaminp Oct 1, 2018

buchgr Oct 2, 2018

buchgr commented Sep 28, 2018

laurentlb commented Sep 28, 2018

buchgr commented Sep 28, 2018

laurentlb commented Sep 28, 2018

jin commented Sep 28, 2018

buchgr commented Oct 1, 2018

benjaminp commented Oct 1, 2018

buchgr commented Oct 2, 2018

benjaminp commented Oct 4, 2018

buchgr commented Oct 4, 2018

ulfjack commented Oct 11, 2018

EdSchouten commented Oct 11, 2018 •

edited

Loading

EdSchouten commented Oct 11, 2018 •

edited

Loading

benjaminp commented Oct 11, 2018

Globegitter commented Oct 27, 2018

buchgr commented Oct 31, 2018

buchgr commented Nov 8, 2018 •

edited

Loading

Flip --experimental_strict_action_env on by default. #6263

Flip --experimental_strict_action_env on by default. #6263

Conversation

benjaminp commented Sep 27, 2018

buchgr Sep 28, 2018

Choose a reason for hiding this comment

benjaminp Oct 1, 2018

Choose a reason for hiding this comment

buchgr Oct 2, 2018

Choose a reason for hiding this comment

buchgr commented Sep 28, 2018

laurentlb commented Sep 28, 2018

buchgr commented Sep 28, 2018

laurentlb commented Sep 28, 2018

jin commented Sep 28, 2018

buchgr commented Oct 1, 2018

benjaminp commented Oct 1, 2018

buchgr commented Oct 2, 2018

benjaminp commented Oct 4, 2018

buchgr commented Oct 4, 2018

ulfjack commented Oct 11, 2018

EdSchouten commented Oct 11, 2018 • edited Loading

EdSchouten commented Oct 11, 2018 • edited Loading

benjaminp commented Oct 11, 2018

Globegitter commented Oct 27, 2018

buchgr commented Oct 31, 2018

buchgr commented Nov 8, 2018 • edited Loading

EdSchouten commented Oct 11, 2018 •

edited

Loading

EdSchouten commented Oct 11, 2018 •

edited

Loading

buchgr commented Nov 8, 2018 •

edited

Loading