From 988bca1cc54f2ec9dda156fe54c531b23a2ba457 Mon Sep 17 00:00:00 2001 From: Ivo List Date: Wed, 23 Aug 2023 12:21:27 -0700 Subject: [PATCH 1/2] 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 --- .../lib/analysis/actions/SpawnAction.java | 46 ++++++++++++-- .../starlark/StarlarkActionFactory.java | 19 ++++-- .../semantics/BuildLanguageOptions.java | 18 ++++++ src/test/shell/integration/action_env_test.sh | 61 ++++++++++++++++++- 4 files changed, 132 insertions(+), 12 deletions(-) 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..10623d80ef46e6 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; @@ -762,12 +763,33 @@ 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 (actionEnvironment != null) { + env = actionEnvironment; + } else 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); } @@ -1075,6 +1097,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 cb43c7cf6a773a..9174991ec2dfdb 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; + /** * 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 @@ -754,6 +767,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) .build(); return INTERNER.intern(semantics); } @@ -844,6 +860,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"; // non-booleans public static final StarlarkSemantics.Key EXPERIMENTAL_BUILTINS_BZL_PATH = 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. From 45c53e229c4c21bb923b8a07d98fef9653ba8ef1 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 6 Sep 2023 13:04:21 +0200 Subject: [PATCH 2/2] Remove `actionEnvironment` --- .../build/lib/analysis/actions/SpawnAction.java | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) 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 10623d80ef46e6..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 @@ -684,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(); @@ -718,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; @@ -764,9 +762,7 @@ public SpawnAction build(ActionOwner owner, BuildConfigurationValue configuratio } CommandLines commandLines = result.build(); ActionEnvironment env; - if (actionEnvironment != null) { - env = actionEnvironment; - } else if (useDefaultShellEnvironment && environment != null) { + 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. @@ -1006,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.