Add --incompatible_merge_fixed_and_default_shell_env

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

@@ -48,6 +48,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;
@@ -777,15 +778,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

@@ -679,6 +679,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;
+
  @Option(
  name = "incompatible_objc_provider_remove_linking_info",
  defaultValue = "false",
@@ -781,6 +793,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 +885,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.