[6.4.0] Add --incompatible_merge_fixed_and_default_shell_env (#19319)

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.

Commit
d1fdc53

PiperOrigin-RevId: 559506535
Change-Id: I7ec6ae17b076bbca72fab8394f3a8b3e4f9ea9d8

Fixes #19312

---------

Co-authored-by: Ivo List <[email protected]>
Co-authored-by: Ian (Hee) Cha <[email protected]>

Author: 3 people

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

@@ -28,6 +28,7 @@
 import com.google.common.collect.Interner;
 import com.google.common.collect.Iterables;
 import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.AbstractAction;
 import com.google.devtools.build.lib.actions.ActionContinuationOrResult;
 import com.google.devtools.build.lib.actions.ActionEnvironment;
@@ -683,7 +684,6 @@ public static class Builder {
     private final List<Artifact> outputs = new ArrayList<>();
     private final List<RunfilesSupplier> inputRunfilesSuppliers = new ArrayList<>();
     private ResourceSetOrBuilder resourceSetOrBuilder = AbstractAction.DEFAULT_RESOURCE_SET;
-    private ActionEnvironment actionEnvironment = null;
     private ImmutableMap<String, String> environment = ImmutableMap.of();
     private ImmutableSet<String> inheritedEnvironment = ImmutableSet.of();
     private ImmutableMap<String, String> executionInfo = ImmutableMap.of();
@@ -717,7 +717,6 @@ public Builder(Builder other) {
       this.outputs.addAll(other.outputs);
       this.inputRunfilesSuppliers.addAll(other.inputRunfilesSuppliers);
       this.resourceSetOrBuilder = other.resourceSetOrBuilder;
-      this.actionEnvironment = other.actionEnvironment;
       this.environment = other.environment;
       this.executionInfo = other.executionInfo;
       this.isShellCommand = other.isShellCommand;
@@ -762,12 +761,31 @@ 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 (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, inheritedEnvironment);
+      }
       return buildSpawnAction(
           owner, commandLines, configuration.getCommandLineLimits(), configuration, env);
     }
@@ -984,13 +1002,6 @@ public Builder setResources(ResourceSetOrBuilder resourceSetOrBuilder) {
       return this;
     }
 
-    /** Sets the action environment. */
-    @CanIgnoreReturnValue
-    public Builder setEnvironment(ActionEnvironment actionEnvironment) {
-      this.actionEnvironment = actionEnvironment;
-      return this;
-    }
-
     /**
      * Sets the map of environment variables. Do not use! This makes the builder ignore the 'default
      * shell environment', which is computed from the --action_env command line option.
@@ -1075,6 +1086,18 @@ public Builder executeUnconditionally() {
       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

@@ -671,15 +671,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);
     }
 
     RuleContext ruleContext = getRuleContext();

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

@@ -662,6 +662,19 @@ public final class BuildLanguageOptions extends OptionsBase {
       help = "If enabled, targets that have unknown attributes set to None fail.")
   public boolean incompatibleFailOnUnknownAttributes;
 
+  @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_disable_objc_library_transition",
       defaultValue = "false",
@@ -765,6 +778,9 @@ public StarlarkSemantics toStarlarkSemantics() {
                 EXPERIMENTAL_GET_FIXED_CONFIGURED_ACTION_ENV,
                 experimentalGetFixedConfiguredEnvironment)
             .setBool(INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES, incompatibleFailOnUnknownAttributes)
+            .setBool(
+                INCOMPATIBLE_MERGE_FIXED_AND_DEFAULT_SHELL_ENV,
+                incompatibleMergeFixedAndDefaultShellEnv)
             .setBool(
                 INCOMPATIBLE_DISABLE_OBJC_LIBRARY_TRANSITION,
                 incompatibleDisableObjcLibraryTransition)
@@ -858,6 +874,8 @@ public StarlarkSemantics toStarlarkSemantics() {
       "-experimental_get_fixed_configured_action_env";
   public static final String INCOMPATIBLE_FAIL_ON_UNKNOWN_ATTRIBUTES =
       "-incompatible_fail_on_unknown_attributes";
+  public static final String INCOMPATIBLE_MERGE_FIXED_AND_DEFAULT_SHELL_ENV =
+      "-experimental_merge_fixed_and_default_shell_env";
   public static final String INCOMPATIBLE_DISABLE_OBJC_LIBRARY_TRANSITION =
       "-incompatible_disable_objc_library_transition";
 

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.