Add --incompatible_merge_fixed_and_default_shell_env

With the new flag, Starlark actions that specify both `env` and
`use_default_shell_env` will no longer have `env` ignored. Instead, the
values of `env` will override the default shell environment.

This allows Starlark actions to both pick up user-configured variables
such as `PATH` from the shell environment as well as set variables to
fixed values required for the action, e.g., variables provided by the
C++ toolchain.

Rationale for having `env` override the default shell env: The rules
know best which values they have to set specific environment variables
to in order to successfully execute an action, so a situation where
users break an action by a globally applied `--action_env` is prevented.
If users really do have to be able to modify an environment variables
fixed by the rule, the rule can always make this configurable via an
attribute.

Work towards #5980

Author: fmeum

src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java

@@ -681,12 +681,35 @@ public SpawnAction build(ActionOwner owner, BuildConfigurationValue configuratio
         result.addCommandLine(pair);
       }
       CommandLines commandLines = result.build();
-      ActionEnvironment env =
-          actionEnvironment != null
-              ? actionEnvironment
-              : useDefaultShellEnvironment
-                  ? configuration.getActionEnvironment()
-                  : ActionEnvironment.create(environment, inheritedEnvironment);
+      ActionEnvironment env;
+      if (actionEnvironment != null) {
+        env = actionEnvironment;
+      } else if (!useDefaultShellEnvironment) {
+        env = ActionEnvironment.create(environment, inheritedEnvironment);
+      } else {
+        // Inherited variables override fixed variables in ActionEnvironment. Since we want the
+        // fixed part of the action-provided environment to override the inherited part of the
+        // user-provided environment, we have to explicitly filter the inherited part.
+        ImmutableMap<String, String> actionFixedEnv =
+            environment != null ? environment : ImmutableMap.of();
+        ImmutableSet<String> actionInheritedEnv =
+            inheritedEnvironment != null ? inheritedEnvironment : ImmutableSet.of();
+        ImmutableSet<String> userFilteredInheritedEnv = configuration.getActionEnvironment()
+            .getInheritedEnv()
+            .stream()
+            .filter(key -> !actionFixedEnv.containsKey(key))
+            .collect(toImmutableSet());
+        // Do not create a new ActionEnvironment in the common case where no vars have been filtered
+        // out.
+        if (userFilteredInheritedEnv.equals(
+            configuration.getActionEnvironment().getInheritedEnv())) {
+          env = configuration.getActionEnvironment();
+        } else {
+          env = ActionEnvironment.create(configuration.getActionEnvironment().getFixedEnv(),
+              userFilteredInheritedEnv);
+        }
+        env = env.withAdditionalVariables(actionFixedEnv, actionInheritedEnv);
+      }
       return buildSpawnAction(
           owner, commandLines, configuration.getCommandLineLimits(), configuration, env);
     }
@@ -962,6 +985,19 @@ public Builder useDefaultShellEnvironment() {
       return this;
     }
 
+    /**
+     * Same as {@link #useDefaultShellEnvironment()}, but additionally sets the provided fixed
+     * environment variables, which take precendence over the variables contained in the default
+     * shell environment.
+     */
+    @CanIgnoreReturnValue
+    public Builder useDefaultShellEnvironmentWithOverrides(Map<String, String> environment) {
+      this.environment = ImmutableMap.copyOf(environment);
+      this.inheritedEnvironment = null;
+      this.useDefaultShellEnvironment = true;
+      return this;
+    }
+
     /**
      * Sets the executable path; the path is interpreted relative to the execution root, unless it's
      * a bare file name.

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java

@@ -47,6 +47,7 @@
 import com.google.devtools.build.lib.analysis.actions.ParameterFileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.actions.StarlarkAction;
+import com.google.devtools.build.lib.analysis.actions.StarlarkAction.Builder;
 import com.google.devtools.build.lib.analysis.actions.Substitution;
 import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
 import com.google.devtools.build.lib.analysis.actions.TemplateExpansionAction;
@@ -712,15 +713,23 @@ private void registerStarlarkAction(
     } catch (IllegalArgumentException e) {
       throw Starlark.errorf("%s", e.getMessage());
     }
-    if (envUnchecked != Starlark.NONE) {
-      builder.setEnvironment(
-          ImmutableMap.copyOf(Dict.cast(envUnchecked, String.class, String.class, "env")));
-    }
     if (progressMessage != Starlark.NONE) {
       builder.setProgressMessageFromStarlark((String) progressMessage);
     }
+
+    ImmutableMap<String, String> env = null;
+    if (envUnchecked != Starlark.NONE) {
+      env = ImmutableMap.copyOf(Dict.cast(envUnchecked, String.class, String.class, "env"));
+    }
     if (Starlark.truth(useDefaultShellEnv)) {
-      builder.useDefaultShellEnvironment();
+      if (env != null && getSemantics().getBool(
+          BuildLanguageOptions.INCOMPATIBLE_MERGE_FIXED_AND_DEFAULT_SHELL_ENV)) {
+        builder.useDefaultShellEnvironmentWithOverrides(env);
+      } else {
+        builder.useDefaultShellEnvironment();
+      }
+    } else if (env != null) {
+      builder.setEnvironment(env);
     }
 
     ImmutableMap<String, String> executionInfo =

src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java

@@ -649,6 +649,18 @@ public final class BuildLanguageOptions extends OptionsBase {
               + " specified through features configuration.")
   public boolean experimentalGetFixedConfiguredEnvironment;
 
+  @Option(
+      name = "incompatible_merge_fixed_and_default_shell_env",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+      help = "If enabled, actions registered with ctx.actions.run and ctx.actions.run_shell with"
+          + " both 'env' and 'use_default_shell_env = True' specified will use an environment"
+          + " obtained from the default shell environment by overriding with the values passed in"
+          + " to 'env'. If disabled, the value of 'env' is completely ignored in this case.")
+  public boolean incompatibleMergeFixedAndDefaultShellEnv;
+
   /**
    * An interner to reduce the number of StarlarkSemantics instances. A single Blaze instance should
    * never accumulate a large number of these and being able to shortcut on object identity makes a
@@ -740,6 +752,9 @@ public StarlarkSemantics toStarlarkSemantics() {
             .setBool(
                 EXPERIMENTAL_GET_FIXED_CONFIGURED_ACTION_ENV,
                 experimentalGetFixedConfiguredEnvironment)
+            .setBool(
+                INCOMPATIBLE_MERGE_FIXED_AND_DEFAULT_SHELL_ENV,
+                incompatibleMergeFixedAndDefaultShellEnv)
             .build();
     return INTERNER.intern(semantics);
   }
@@ -827,6 +842,8 @@ public StarlarkSemantics toStarlarkSemantics() {
       "-incompatible_disable_starlark_host_transitions";
   public static final String EXPERIMENTAL_GET_FIXED_CONFIGURED_ACTION_ENV =
       "-experimental_get_fixed_configured_action_env";
+  public static final String INCOMPATIBLE_MERGE_FIXED_AND_DEFAULT_SHELL_ENV =
+      "-experimental_merge_fixed_and_default_shell_env";
 
   // non-booleans
   public static final StarlarkSemantics.Key<String> EXPERIMENTAL_BUILTINS_BZL_PATH =

src/test/shell/integration/action_env_test.sh

@@ -39,6 +39,15 @@ load("//pkg:build.bzl", "environ")
 
 environ(name = "no_default_env", env = 0)
 environ(name = "with_default_env", env = 1)
+environ(
+    name = "with_default_and_fixed_env",
+    env = 1,
+    fixed_env = {
+        "ACTION_FIXED": "action",
+        "ACTION_AND_CLIENT_FIXED": "action",
+        "ACTION_AND_CLIENT_INHERITED": "action",
+    },
+)
 
 sh_test(
     name = "test_env_foo",
@@ -72,12 +81,16 @@ def _impl(ctx):
   ctx.actions.run_shell(
       inputs=[],
       outputs=[output],
+      env = ctx.attr.fixed_env,
       use_default_shell_env = ctx.attr.env,
       command="env > %s" % output.path)
 
 environ = rule(
     implementation=_impl,
-    attrs={"env": attr.bool(default=True)},
+    attrs={
+        "env": attr.bool(default=True),
+        "fixed_env": attr.string_dict(),
+    },
     outputs={"out": "%{name}.env"},
 )
 EOF
@@ -222,6 +235,52 @@ function test_use_default_shell_env {
          && fail "dynamic action_env used, even though requested not to") || true
 }
 
+function test_use_default_shell_env_and_fixed_env {
+    ACTION_AND_CLIENT_INHERITED=client CLIENT_INHERITED=client \
+        bazel build \
+        --noincompatible_merge_fixed_and_default_shell_env \
+        --action_env=ACTION_AND_CLIENT_FIXED=client \
+        --action_env=ACTION_AND_CLIENT_INHERITED \
+        --action_env=CLIENT_FIXED=client \
+        --action_env=CLIENT_INHERITED \
+        //pkg:with_default_and_fixed_env
+    echo
+    cat bazel-bin/pkg/with_default_and_fixed_env.env
+    echo
+    grep -q ACTION_AND_CLIENT_FIXED=client bazel-bin/pkg/with_default_and_fixed_env.env \
+        || fail "static action environment not honored"
+    grep -q ACTION_AND_CLIENT_INHERITED=client bazel-bin/pkg/with_default_and_fixed_env.env \
+        || fail "dynamic action environment not honored"
+    grep -q ACTION_FIXED bazel-bin/pkg/with_default_and_fixed_env.env \
+        && fail "fixed env provided by action should have been ignored"
+    grep -q CLIENT_FIXED=client bazel-bin/pkg/with_default_and_fixed_env.env \
+        || fail "static action environment not honored"
+    grep -q CLIENT_INHERITED=client bazel-bin/pkg/with_default_and_fixed_env.env \
+        || fail "dynamic action environment not honored"
+
+    ACTION_AND_CLIENT_INHERITED=client CLIENT_INHERITED=client \
+        bazel build \
+        --incompatible_merge_fixed_and_default_shell_env \
+        --action_env=ACTION_AND_CLIENT_FIXED=client \
+        --action_env=ACTION_AND_CLIENT_INHERITED \
+        --action_env=CLIENT_FIXED=client \
+        --action_env=CLIENT_INHERITED \
+        //pkg:with_default_and_fixed_env
+    echo
+    cat bazel-bin/pkg/with_default_and_fixed_env.env
+    echo
+    grep -q ACTION_AND_CLIENT_FIXED=action bazel-bin/pkg/with_default_and_fixed_env.env \
+        || fail "action-provided env should have overridden static --action_env"
+    grep -q ACTION_AND_CLIENT_INHERITED=action bazel-bin/pkg/with_default_and_fixed_env.env \
+        || fail "action-provided env should have overridden dynamic --action_env"
+    grep -q ACTION_FIXED=action bazel-bin/pkg/with_default_and_fixed_env.env \
+        || fail "action-provided env should have been honored"
+    grep -q CLIENT_FIXED=client bazel-bin/pkg/with_default_and_fixed_env.env \
+        || fail "static action environment not honored"
+    grep -q CLIENT_INHERITED=client bazel-bin/pkg/with_default_and_fixed_env.env \
+        || fail "dynamic action environment not honored"
+}
+
 function test_action_env_changes_honored {
     # Verify that changes to the explicitly specified action_env in honored in
     # tests. Regression test for #3265.