diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index 43d34f565f99ff..c2c1cfe9b46736 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/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 outputs = new ArrayList<>(); private final List inputRunfilesSuppliers = new ArrayList<>(); private ResourceSetOrBuilder resourceSetOrBuilder = AbstractAction.DEFAULT_RESOURCE_SET; - private ActionEnvironment actionEnvironment = null; private ImmutableMap environment = ImmutableMap.of(); private ImmutableSet inheritedEnvironment = ImmutableSet.of(); private ImmutableMap 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 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. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java index ca8d46a9ce1671..b6058da30c971b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java +++ b/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 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(); diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index 2259ab5a68e7d5..469427b6f32856 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/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"; diff --git a/src/test/shell/integration/action_env_test.sh b/src/test/shell/integration/action_env_test.sh index 08af2abd059630..6feaf075740bb0 100755 --- a/src/test/shell/integration/action_env_test.sh +++ b/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.