From 8a870648c213273250f6995a984eef2e0e7dee14 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 27 Apr 2023 12:18:29 +0200 Subject: [PATCH] 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 --- .../lib/analysis/actions/SpawnAction.java | 42 +++++++++++-- .../starlark/StarlarkActionFactory.java | 19 ++++-- .../semantics/BuildLanguageOptions.java | 17 ++++++ src/test/shell/integration/action_env_test.sh | 61 ++++++++++++++++++- 4 files changed, 129 insertions(+), 10 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 8a38f2b4c257c9..8744c8f78e7a31 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 @@ -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); } @@ -879,6 +901,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 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 20b5fc98e1ac9a..643ac3ee1d88ac 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 @@ -49,6 +49,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; @@ -763,15 +764,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 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 executionInfo = 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 2641aa3fdee03c..1dfebd8ecb321c 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 @@ -658,6 +658,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; + /** * 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 @@ -750,6 +762,9 @@ public StarlarkSemantics toStarlarkSemantics() { .setBool( EXPERIMENTAL_GET_FIXED_CONFIGURED_ACTION_ENV, experimentalGetFixedConfiguredEnvironment) + .setBool( + INCOMPATIBLE_MERGE_FIXED_AND_DEFAULT_SHELL_ENV, + incompatibleMergeFixedAndDefaultShellEnv) .build(); return INTERNER.intern(semantics); } @@ -838,6 +853,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"; // 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.