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
Fixes #12049

Closes #18235.

PiperOrigin-RevId: 559506535
Change-Id: I7ec6ae17b076bbca72fab8394f3a8b3e4f9ea9d8

Author: comius

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);
     }
 
@@ -894,6 +916,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

@@ -777,15 +777,24 @@ 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

@@ -679,6 +679,19 @@ 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;
+
   @Option(
       name = "incompatible_objc_provider_remove_linking_info",
       defaultValue = "false",
@@ -781,6 +794,9 @@ public StarlarkSemantics toStarlarkSemantics() {
             .setBool(
                 EXPERIMENTAL_GET_FIXED_CONFIGURED_ACTION_ENV,
                 experimentalGetFixedConfiguredEnvironment)
+            .setBool(
+                INCOMPATIBLE_MERGE_FIXED_AND_DEFAULT_SHELL_ENV,
+                incompatibleMergeFixedAndDefaultShellEnv)
             .setBool(
                 INCOMPATIBLE_OBJC_PROVIDER_REMOVE_LINKING_INFO,
                 incompatibleObjcProviderRemoveLinkingInfo)
@@ -870,6 +886,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";
   public static final String INCOMPATIBLE_OBJC_PROVIDER_REMOVE_LINKING_INFO =
       "-incompatible_objc_provider_remove_linking_info";
 

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",
@@ -65,12 +74,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
@@ -215,6 +228,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.