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

@@ -29,6 +29,7 @@
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.Interner;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.AbstractAction;
 import com.google.devtools.build.lib.actions.ActionEnvironment;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
@@ -649,10 +650,31 @@ public SpawnAction build(ActionOwner owner, BuildConfigurationValue configuratio
         result.addCommandLine(pair);
       }
       CommandLines commandLines = result.build();
-      ActionEnvironment env =
-          useDefaultShellEnvironment
-              ? configuration.getActionEnvironment()
-              : ActionEnvironment.create(environment);
+      ActionEnvironment env;
+      if (useDefaultShellEnvironment && environment != null) {
+        // 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.
+        var userFilteredInheritedEnv =
+            ImmutableSet.copyOf(
+                Sets.difference(
+                    configuration.getActionEnvironment().getInheritedEnv(), environment.keySet()));
+        // 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.withAdditionalFixedVariables(environment);
+      } else if (useDefaultShellEnvironment) {
+        env = configuration.getActionEnvironment();
+      } else {
+        env = ActionEnvironment.create(environment);
+      }
       return buildSpawnAction(owner, commandLines, configuration, env);
     }
 
@@ -879,6 +901,18 @@ public Builder useDefaultShellEnvironment() {
       return this;
     }
 
+    /**
+     * Same as {@link #useDefaultShellEnvironment()}, but additionally sets the provided fixed
+     * environment variables, which take precedence over the variables contained in the default
+     * shell environment.
+     */
+    @CanIgnoreReturnValue
+    public Builder useDefaultShellEnvironmentWithOverrides(Map<String, String> environment) {
+      this.environment = ImmutableMap.copyOf(environment);
+      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

@@ -49,6 +49,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;
@@ -763,15 +764,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

@@ -658,6 +658,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
@@ -750,6 +762,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);
   }
@@ -838,6 +853,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.