From dbdfa07e92f99497be9c14265611ad2920161483 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 29 Jun 2022 23:57:57 +0200 Subject: [PATCH] Let Starlark executable rules specify their environment (#15766) * Specify fixedEnv/inheritedEnv interaction in ActionEnvironment Previously, ActionEnvironment did not publicly document how fixed and inherited environment variables interact, but still cautioned users to keep the two sets disjoint without enforcing this. As a result, neither could users rely on the interaction nor could ActionEnvironment benefit from the additional freedom of not specifying the behavior. With this commit, ActionEnvironment explicitly specifies that the values of environment variable inherited from the client environment take precedence over fixed values and codifies this behavior in a test. This has been the effective behavior all along and has the advantage that users can provide overrideable defaults for environment variables. Closes #15170. PiperOrigin-RevId: 439315634 * Intern trivial ActionEnvironment and EnvironmentVariables instances When an ActionEnvironment is constructed out of an existing one, the ActionEnvironment and EnvironmentVariables instances, which are immutable, can be reused if no variables are added. Also renames addVariables and addFixedVariables to better reflect that they are operating on an immutable type. Closes #15171. PiperOrigin-RevId: 440312159 * Let Starlark executable rules specify their environment The new RunEnvironmentInfo provider allows any executable or test Starlark rule to specify the environment for when it is executed, either as part of a test action or via the run command. Refactors testing.TestEnvironment to construct a RunEnvironmentInfo and adds a warning (but not an error) if the provider constructed in this way is returned from a non-executable non-test rule. If a RunEnvironmentInfo is constructed directly via the Starlark constructor, this warning becomes an error. Fixes #7364 Fixes #15224 Fixes #15225 Closes #15232. PiperOrigin-RevId: 448185352 * Fix strict deps violation ``` src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java:990: error: [strict] Using type com.google.devtools.build.lib.cmdline.LabelValidator from an indirect dependency (TOOL_INFO: "//src/main/java/com/google/devtools/build/lib/cmdline:LabelValidator"). See command below ** LabelValidator.parseAbsoluteLabel(labelString); ^ ``` --- .../build/lib/actions/ActionEnvironment.java | 48 +++++--- .../google/devtools/build/lib/analysis/BUILD | 26 ++--- .../analysis/RuleConfiguredTargetBuilder.java | 6 +- .../lib/analysis/RunEnvironmentInfo.java | 106 ++++++++++++++++++ .../build/lib/analysis/RunfilesSupport.java | 4 +- .../analysis/starlark/StarlarkModules.java | 2 + .../StarlarkRuleConfiguredTargetUtil.java | 12 ++ .../lib/analysis/test/TestActionBuilder.java | 2 +- .../analysis/test/TestEnvironmentInfo.java | 63 ----------- .../com/google/devtools/build/lib/rules/BUILD | 2 +- .../rules/java/JavaCompileActionBuilder.java | 5 +- .../java/JavaHeaderCompileActionBuilder.java | 5 +- .../lib/rules/test/StarlarkTestingModule.java | 9 +- .../devtools/build/lib/runtime/commands/BUILD | 1 + .../lib/runtime/commands/RunCommand.java | 16 ++- .../RunEnvironmentInfoApi.java | 103 +++++++++++++++++ .../test/TestEnvironmentInfoApi.java | 38 ------- .../test/TestingModuleApi.java | 9 +- .../lib/actions/ActionEnvironmentTest.java | 40 ++++++- .../devtools/build/lib/rules/test/BUILD | 2 +- .../rules/test/StarlarkTestingModuleTest.java | 7 +- .../google/devtools/build/lib/starlark/BUILD | 1 + .../lib/starlark/StarlarkIntegrationTest.java | 87 ++++++++++++++ src/test/shell/bazel/bazel_rules_test.sh | 85 +++++++++++++- 24 files changed, 519 insertions(+), 160 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/RunEnvironmentInfo.java delete mode 100644 src/main/java/com/google/devtools/build/lib/analysis/test/TestEnvironmentInfo.java create mode 100644 src/main/java/com/google/devtools/build/lib/starlarkbuildapi/RunEnvironmentInfoApi.java delete mode 100644 src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/TestEnvironmentInfoApi.java diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java b/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java index 8a9e3d18f081ae..95e2e26462103f 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java @@ -46,8 +46,8 @@ public final class ActionEnvironment { /** - * A map of environment variables together with a list of variables to inherit from the shell - * environment. + * A map of environment variables and their values together with a list of variables whose values + * should be inherited from the client environment. */ public interface EnvironmentVariables { @@ -78,14 +78,14 @@ default int size() { /** * An {@link EnvironmentVariables} that combines variables from two different environments without - * allocation a new map. + * allocating a new map. */ static class CompoundEnvironmentVariables implements EnvironmentVariables { static EnvironmentVariables create( Map fixedVars, Set inheritedVars, EnvironmentVariables base) { - if (fixedVars.isEmpty() && inheritedVars.isEmpty() && base.isEmpty()) { - return EMPTY_ENVIRONMENT_VARIABLES; + if (fixedVars.isEmpty() && inheritedVars.isEmpty()) { + return base; } return new CompoundEnvironmentVariables(fixedVars, inheritedVars, base); } @@ -168,8 +168,6 @@ public ImmutableSet getInheritedEnvironment() { * given map. Returns these two parts as a new {@link ActionEnvironment} instance. */ public static ActionEnvironment split(Map env) { - // Care needs to be taken that the two sets don't overlap - the order in which the two parts are - // combined later is undefined. Map fixedEnv = new TreeMap<>(); Set inheritedEnv = new TreeSet<>(); for (Map.Entry entry : env.entrySet()) { @@ -190,9 +188,11 @@ private ActionEnvironment(EnvironmentVariables vars) { } /** - * Creates a new action environment. The order in which the environments are combined is - * undefined, so callers need to take care that the key set of the {@code fixedEnv} map and the - * set of {@code inheritedEnv} elements are disjoint. + * Creates a new action environment. If an environment variable is contained in both {@link + * EnvironmentVariables#getFixedEnvironment()} and {@link + * EnvironmentVariables#getInheritedEnvironment()}, the result of {@link + * ActionEnvironment#resolve(Map, Map)} will contain the value inherited from the client + * environment. */ private static ActionEnvironment create(EnvironmentVariables vars) { if (vars.isEmpty()) { @@ -201,6 +201,12 @@ private static ActionEnvironment create(EnvironmentVariables vars) { return new ActionEnvironment(vars); } + /** + * Creates a new action environment. If an environment variable is contained both as a key in + * {@code fixedEnv} and in {@code inheritedEnv}, the result of {@link + * ActionEnvironment#resolve(Map, Map)} will contain the value inherited from the client + * environment. + */ public static ActionEnvironment create( Map fixedEnv, ImmutableSet inheritedEnv) { return ActionEnvironment.create(SimpleEnvironmentVariables.create(fixedEnv, inheritedEnv)); @@ -214,21 +220,29 @@ public static ActionEnvironment create(Map fixedEnv) { * Returns a copy of the environment with the given fixed variables added to it, overwriting * any existing occurrences of those variables. */ - public ActionEnvironment addFixedVariables(Map fixedVars) { - return ActionEnvironment.create( - CompoundEnvironmentVariables.create(fixedVars, ImmutableSet.of(), vars)); + public ActionEnvironment withAdditionalFixedVariables(Map fixedVars) { + return withAdditionalVariables(fixedVars, ImmutableSet.of()); } /** * Returns a copy of the environment with the given fixed and inherited variables added to it, * overwriting any existing occurrences of those variables. */ - public ActionEnvironment addVariables(Map fixedVars, Set inheritedVars) { - return ActionEnvironment.create( - CompoundEnvironmentVariables.create(fixedVars, inheritedVars, vars)); + public ActionEnvironment withAdditionalVariables( + Map fixedVars, Set inheritedVars) { + EnvironmentVariables newVars = + CompoundEnvironmentVariables.create(fixedVars, inheritedVars, vars); + if (newVars == vars) { + return this; + } + return ActionEnvironment.create(newVars); } - /** Returns the combined size of the fixed and inherited environments. */ + /** + * Returns an upper bound on the combined size of the fixed and inherited environments. A call to + * {@link ActionEnvironment#resolve(Map, Map)} may add less entries than this number if + * environment variables are contained in both the fixed and the inherited environment. + */ public int size() { return vars.size(); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 4cfeb659323e79..24fab7ad3d96ee 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -134,7 +134,6 @@ java_library( ":test/execution_info", ":test/instrumented_files_info", ":test/test_configuration", - ":test/test_environment_info", ":test/test_sharding_strategy", ":test/test_trimming_transition_factory", ":toolchain_collection", @@ -339,6 +338,7 @@ java_library( ":resolved_toolchain_context", ":rule_configured_object_value", ":rule_definition_environment", + ":run_environment_info", ":starlark/args", ":starlark/bazel_build_api_globals", ":starlark/function_transition_util", @@ -356,7 +356,6 @@ java_library( ":test/execution_info", ":test/instrumented_files_info", ":test/test_configuration", - ":test/test_environment_info", ":test/test_sharding_strategy", ":toolchain_collection", ":toolchain_context", @@ -999,6 +998,18 @@ java_library( ], ) +java_library( + name = "run_environment_info", + srcs = ["RunEnvironmentInfo.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/concurrent", + "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi", + "//src/main/java/net/starlark/java/eval", + "//third_party:guava", + ], +) + java_library( name = "rule_definition_environment", srcs = ["RuleDefinitionEnvironment.java"], @@ -2365,17 +2376,6 @@ java_library( ], ) -java_library( - name = "test/test_environment_info", - srcs = ["test/TestEnvironmentInfo.java"], - deps = [ - "//src/main/java/com/google/devtools/build/lib/concurrent", - "//src/main/java/com/google/devtools/build/lib/packages", - "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test", - "//third_party:guava", - ], -) - java_library( name = "test/test_sharding_strategy", srcs = ["test/TestShardingStrategy.java"], diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java index 59cde31fd2b0a6..490ac0c70b83eb 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java @@ -40,7 +40,6 @@ import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo; import com.google.devtools.build.lib.analysis.test.TestActionBuilder; import com.google.devtools.build.lib.analysis.test.TestConfiguration; -import com.google.devtools.build.lib.analysis.test.TestEnvironmentInfo; import com.google.devtools.build.lib.analysis.test.TestProvider; import com.google.devtools.build.lib.analysis.test.TestProvider.TestParams; import com.google.devtools.build.lib.analysis.test.TestTagsProvider; @@ -465,9 +464,8 @@ private TestProvider initializeTestProvider(FilesToRunProvider filesToRunProvide providersBuilder.getProvider( InstrumentedFilesInfo.STARLARK_CONSTRUCTOR.getKey())); - TestEnvironmentInfo environmentProvider = - (TestEnvironmentInfo) - providersBuilder.getProvider(TestEnvironmentInfo.PROVIDER.getKey()); + RunEnvironmentInfo environmentProvider = + (RunEnvironmentInfo) providersBuilder.getProvider(RunEnvironmentInfo.PROVIDER.getKey()); if (environmentProvider != null) { testActionBuilder.addExtraEnv(environmentProvider.getEnvironment()); testActionBuilder.addExtraInheritedEnv(environmentProvider.getInheritedEnvironment()); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunEnvironmentInfo.java b/src/main/java/com/google/devtools/build/lib/analysis/RunEnvironmentInfo.java new file mode 100644 index 00000000000000..cb40eceefe43aa --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/RunEnvironmentInfo.java @@ -0,0 +1,106 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.analysis; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; +import com.google.devtools.build.lib.packages.BuiltinProvider; +import com.google.devtools.build.lib.packages.NativeInfo; +import com.google.devtools.build.lib.starlarkbuildapi.RunEnvironmentInfoApi; +import java.util.List; +import java.util.Map; +import net.starlark.java.eval.Dict; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.Sequence; +import net.starlark.java.eval.StarlarkList; + +/** + * Provider containing any additional environment variables for use in the executable or test + * action. + */ +@Immutable +public final class RunEnvironmentInfo extends NativeInfo implements RunEnvironmentInfoApi { + + /** Singleton instance of the provider type for {@link DefaultInfo}. */ + public static final RunEnvironmentInfoProvider PROVIDER = new RunEnvironmentInfoProvider(); + + private final ImmutableMap environment; + private final ImmutableList inheritedEnvironment; + private final boolean shouldErrorOnNonExecutableRule; + + /** Constructs a new provider with the given fixed and inherited environment variables. */ + public RunEnvironmentInfo( + Map environment, + List inheritedEnvironment, + boolean shouldErrorOnNonExecutableRule) { + this.environment = ImmutableMap.copyOf(Preconditions.checkNotNull(environment)); + this.inheritedEnvironment = + ImmutableList.copyOf(Preconditions.checkNotNull(inheritedEnvironment)); + this.shouldErrorOnNonExecutableRule = shouldErrorOnNonExecutableRule; + } + + @Override + public RunEnvironmentInfoProvider getProvider() { + return PROVIDER; + } + + /** + * Returns environment variables which should be set when the target advertising this provider is + * executed. + */ + @Override + public ImmutableMap getEnvironment() { + return environment; + } + + /** + * Returns names of environment variables whose value should be inherited from the shell + * environment when the target advertising this provider is executed. + */ + @Override + public ImmutableList getInheritedEnvironment() { + return inheritedEnvironment; + } + + /** + * Returns whether advertising this provider on a non-executable (and thus non-test) rule should + * result in an error or a warning. The latter is required to not break testing.TestEnvironment, + * which is now implemented via RunEnvironmentInfo. + */ + public boolean shouldErrorOnNonExecutableRule() { + return shouldErrorOnNonExecutableRule; + } + + /** Provider implementation for {@link RunEnvironmentInfoApi}. */ + public static class RunEnvironmentInfoProvider extends BuiltinProvider + implements RunEnvironmentInfoApi.RunEnvironmentInfoApiProvider { + + private RunEnvironmentInfoProvider() { + super("RunEnvironmentInfo", RunEnvironmentInfo.class); + } + + @Override + public RunEnvironmentInfoApi constructor( + Dict environment, Sequence inheritedEnvironment) throws EvalException { + return new RunEnvironmentInfo( + Dict.cast(environment, String.class, String.class, "environment"), + StarlarkList.immutableCopyOf( + Sequence.cast(inheritedEnvironment, String.class, "inherited_environment")), + /* shouldErrorOnNonExecutableRule */ true); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java index d0fe247bccfb05..ca20a2e3c982dd 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java @@ -475,9 +475,7 @@ private static CommandLine computeArgs(RuleContext ruleContext, CommandLine addi } private static ActionEnvironment computeActionEnvironment(RuleContext ruleContext) { - // Currently, "env" and "env_inherit" are not added to Starlark-defined rules (unlike "args"), - // in order to avoid breaking existing Starlark rules that use those attribute names. - // TODO(brandjon): Support "env" and "env_inherit" for Starlark-defined rules. + // Executable Starlark rules can use RunEnvironmentInfo to specify environment variables. boolean isNativeRule = ruleContext.getRule().getRuleClassObject().getRuleDefinitionEnvironmentLabel() == null; if (!isNativeRule diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkModules.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkModules.java index a3105a713c1b6c..c44ae4c4a9a2bb 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkModules.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkModules.java @@ -18,6 +18,7 @@ import com.google.devtools.build.lib.analysis.ActionsProvider; import com.google.devtools.build.lib.analysis.DefaultInfo; import com.google.devtools.build.lib.analysis.OutputGroupInfo; +import com.google.devtools.build.lib.analysis.RunEnvironmentInfo; import com.google.devtools.build.lib.packages.StarlarkLibrary; import com.google.devtools.build.lib.packages.StructProvider; import net.starlark.java.eval.Starlark; @@ -44,5 +45,6 @@ public static void addPredeclared(ImmutableMap.Builder predeclar predeclared.put("OutputGroupInfo", OutputGroupInfo.STARLARK_CONSTRUCTOR); predeclared.put("Actions", ActionsProvider.INSTANCE); predeclared.put("DefaultInfo", DefaultInfo.PROVIDER); + predeclared.put("RunEnvironmentInfo", RunEnvironmentInfo.PROVIDER); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java index 5b1a22e33d7ecf..aae3da4adf4236 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.analysis.DefaultInfo; import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder; import com.google.devtools.build.lib.analysis.RuleContext; +import com.google.devtools.build.lib.analysis.RunEnvironmentInfo; import com.google.devtools.build.lib.analysis.Runfiles; import com.google.devtools.build.lib.analysis.RunfilesProvider; import com.google.devtools.build.lib.analysis.RunfilesSupport; @@ -348,6 +349,17 @@ private static void addProviders( if (getProviderKey(declaredProvider).equals(DefaultInfo.PROVIDER.getKey())) { parseDefaultProviderFields((DefaultInfo) declaredProvider, context, builder); defaultProviderProvidedExplicitly = true; + } else if (getProviderKey(declaredProvider).equals(RunEnvironmentInfo.PROVIDER.getKey()) + && !(context.isExecutable() || context.getRuleContext().isTestTarget())) { + String message = + "Returning RunEnvironmentInfo from a non-executable, non-test target has no effect"; + RunEnvironmentInfo runEnvironmentInfo = (RunEnvironmentInfo) declaredProvider; + if (runEnvironmentInfo.shouldErrorOnNonExecutableRule()) { + context.getRuleContext().ruleError(message); + } else { + context.getRuleContext().ruleWarning(message); + builder.addStarlarkDeclaredProvider(declaredProvider); + } } else { builder.addStarlarkDeclaredProvider(declaredProvider); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java index d545812330ad09..0a35c3b32bb708 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java @@ -453,7 +453,7 @@ private TestParams createTestAction(int shards) testProperties, runfilesSupport .getActionEnvironment() - .addVariables(extraTestEnv, extraInheritedEnv), + .withAdditionalVariables(extraTestEnv, extraInheritedEnv), executionSettings, shard, run, diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestEnvironmentInfo.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestEnvironmentInfo.java deleted file mode 100644 index d5245338342d2d..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestEnvironmentInfo.java +++ /dev/null @@ -1,63 +0,0 @@ -// Copyright 2015 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.analysis.test; - -import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; -import com.google.devtools.build.lib.packages.BuiltinProvider; -import com.google.devtools.build.lib.packages.NativeInfo; -import com.google.devtools.build.lib.starlarkbuildapi.test.TestEnvironmentInfoApi; -import java.util.List; -import java.util.Map; - -/** Provider containing any additional environment variables for use in the test action. */ -@Immutable -public final class TestEnvironmentInfo extends NativeInfo implements TestEnvironmentInfoApi { - - /** Starlark constructor and identifier for TestEnvironmentInfo. */ - public static final BuiltinProvider PROVIDER = - new BuiltinProvider("TestEnvironment", TestEnvironmentInfo.class) {}; - - private final ImmutableMap environment; - private final ImmutableList inheritedEnvironment; - - /** Constructs a new provider with the given fixed and inherited environment variables. */ - public TestEnvironmentInfo(Map environment, List inheritedEnvironment) { - this.environment = ImmutableMap.copyOf(Preconditions.checkNotNull(environment)); - this.inheritedEnvironment = - ImmutableList.copyOf(Preconditions.checkNotNull(inheritedEnvironment)); - } - - @Override - public BuiltinProvider getProvider() { - return PROVIDER; - } - - /** - * Returns environment variables which should be set on the test action. - */ - @Override - public Map getEnvironment() { - return environment; - } - - /** Returns names of environment variables whose value should be obtained from the environment. */ - @Override - public ImmutableList getInheritedEnvironment() { - return inheritedEnvironment; - } -} diff --git a/src/main/java/com/google/devtools/build/lib/rules/BUILD b/src/main/java/com/google/devtools/build/lib/rules/BUILD index cbc39276b35b22..cf54a4361d4704 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -122,12 +122,12 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment", + "//src/main/java/com/google/devtools/build/lib/analysis:run_environment_info", "//src/main/java/com/google/devtools/build/lib/analysis:test/analysis_failure_info", "//src/main/java/com/google/devtools/build/lib/analysis:test/analysis_test_result_info", "//src/main/java/com/google/devtools/build/lib/analysis:test/execution_info", "//src/main/java/com/google/devtools/build/lib/analysis:test/instrumented_files_info", "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration", - "//src/main/java/com/google/devtools/build/lib/analysis:test/test_environment_info", "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection", "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_provider", "//src/main/java/com/google/devtools/build/lib/concurrent", diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java index 492deef8d7792e..c4c8fe5236fb95 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java @@ -206,7 +206,10 @@ public JavaCompileAction build() { toolsBuilder.addTransitive(toolsJars); ActionEnvironment actionEnvironment = - ruleContext.getConfiguration().getActionEnvironment().addFixedVariables(UTF8_ENVIRONMENT); + ruleContext + .getConfiguration() + .getActionEnvironment() + .withAdditionalFixedVariables(UTF8_ENVIRONMENT); NestedSetBuilder mandatoryInputs = NestedSetBuilder.stableOrder(); mandatoryInputs diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java index 73731bdd803ee4..6757895433e872 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java @@ -295,7 +295,10 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti } ActionEnvironment actionEnvironment = - ruleContext.getConfiguration().getActionEnvironment().addFixedVariables(UTF8_ENVIRONMENT); + ruleContext + .getConfiguration() + .getActionEnvironment() + .withAdditionalFixedVariables(UTF8_ENVIRONMENT); OnDemandString progressMessage = new ProgressMessage( diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java b/src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java index 1c6fcb6c23dad9..5373dd0eb6dedf 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java @@ -13,8 +13,8 @@ // limitations under the License. package com.google.devtools.build.lib.rules.test; +import com.google.devtools.build.lib.analysis.RunEnvironmentInfo; import com.google.devtools.build.lib.analysis.test.ExecutionInfo; -import com.google.devtools.build.lib.analysis.test.TestEnvironmentInfo; import com.google.devtools.build.lib.starlarkbuildapi.test.TestingModuleApi; import net.starlark.java.eval.Dict; import net.starlark.java.eval.EvalException; @@ -31,13 +31,14 @@ public ExecutionInfo executionInfo(Dict requirements /* * } @Override - public TestEnvironmentInfo testEnvironment( + public RunEnvironmentInfo testEnvironment( Dict environment /* */, Sequence inheritedEnvironment /* */) throws EvalException { - return new TestEnvironmentInfo( + return new RunEnvironmentInfo( Dict.cast(environment, String.class, String.class, "environment"), StarlarkList.immutableCopyOf( - Sequence.cast(inheritedEnvironment, String.class, "inherited_environment"))); + Sequence.cast(inheritedEnvironment, String.class, "inherited_environment")), + /* shouldErrorOnNonExecutableRule */ false); } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD b/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD index 626bf683f1305d..6a404df1f82cbd 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD @@ -46,6 +46,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:no_build_event", "//src/main/java/com/google/devtools/build/lib/analysis:no_build_request_finished_event", "//src/main/java/com/google/devtools/build/lib/analysis:print_action_visitor", + "//src/main/java/com/google/devtools/build/lib/analysis:run_environment_info", "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration", "//src/main/java/com/google/devtools/build/lib/buildeventstream", "//src/main/java/com/google/devtools/build/lib/cmdline", diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java index b125acd48b1a6f..1d9600bb826d5b 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java @@ -20,9 +20,11 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; +import com.google.devtools.build.lib.actions.ActionEnvironment; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.CommandLine; @@ -31,6 +33,7 @@ import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.FilesToRunProvider; +import com.google.devtools.build.lib.analysis.RunEnvironmentInfo; import com.google.devtools.build.lib.analysis.RunfilesSupport; import com.google.devtools.build.lib.analysis.ShToolchain; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; @@ -472,9 +475,18 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti } } else { workingDir = runfilesDir; + ActionEnvironment actionEnvironment = ActionEnvironment.EMPTY; if (runfilesSupport != null) { - runfilesSupport.getActionEnvironment().resolve(runEnvironment, env.getClientEnv()); + actionEnvironment = runfilesSupport.getActionEnvironment(); } + RunEnvironmentInfo environmentProvider = targetToRun.get(RunEnvironmentInfo.PROVIDER); + if (environmentProvider != null) { + actionEnvironment = + actionEnvironment.withAdditionalVariables( + environmentProvider.getEnvironment(), + ImmutableSet.copyOf(environmentProvider.getInheritedEnvironment())); + } + actionEnvironment.resolve(runEnvironment, env.getClientEnv()); try { List args = computeArgs(targetToRun, commandLineArgs); constructCommandLine( @@ -506,7 +518,7 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti runEnvironment, workingDir.getPathString(), configuration.checksum(), - /* executionPlatform= */ null); + /* executionPlatformAsLabelString= */ null); PathFragment shExecutable = ShToolchain.getPath(configuration); if (shExecutable.isEmpty()) { diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/RunEnvironmentInfoApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/RunEnvironmentInfoApi.java new file mode 100644 index 00000000000000..3de4fca081a126 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/RunEnvironmentInfoApi.java @@ -0,0 +1,103 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.starlarkbuildapi; + +import com.google.devtools.build.docgen.annot.DocCategory; +import com.google.devtools.build.docgen.annot.StarlarkConstructor; +import com.google.devtools.build.lib.starlarkbuildapi.core.ProviderApi; +import com.google.devtools.build.lib.starlarkbuildapi.core.StructApi; +import java.util.List; +import java.util.Map; +import net.starlark.java.annot.Param; +import net.starlark.java.annot.ParamType; +import net.starlark.java.annot.StarlarkBuiltin; +import net.starlark.java.annot.StarlarkMethod; +import net.starlark.java.eval.Dict; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.Sequence; + +/** + * Provider containing any additional environment variables for use when running executables, either + * in test actions or when executed via the run command. + */ +@StarlarkBuiltin( + name = "RunEnvironmentInfo", + category = DocCategory.PROVIDER, + doc = + "A provider that can be returned from executable rules to control the environment in" + + " which their executable is executed.") +public interface RunEnvironmentInfoApi extends StructApi { + + @StarlarkMethod( + name = "environment", + doc = + "A map of string keys and values that represent environment variables and their values." + + " These will be made available when the target that returns this provider is" + + " executed, either as a test or via the run command.", + structField = true) + Map getEnvironment(); + + @StarlarkMethod( + name = "inherited_environment", + doc = + "A sequence of names of environment variables. These variables are made available with" + + " their current value taken from the shell environment when the target that returns" + + " this provider is executed, either as a test or via the run command. If a variable" + + " is contained in both environment and" + + " inherited_environment, the value inherited from the shell" + + " environment will take precedence if set.", + structField = true) + List getInheritedEnvironment(); + + /** Provider for {@link RunEnvironmentInfoApi}. */ + @StarlarkBuiltin(name = "Provider", category = DocCategory.PROVIDER, documented = false, doc = "") + interface RunEnvironmentInfoApiProvider extends ProviderApi { + + @StarlarkMethod( + name = "RunEnvironmentInfo", + doc = "", + documented = false, + parameters = { + @Param( + name = "environment", + defaultValue = "{}", + named = true, + positional = true, + doc = + "A map of string keys and values that represent environment variables and their" + + " values. These will be made available when the target that returns this" + + " provider is executed, either as a test or via the run command."), + @Param( + name = "inherited_environment", + allowedTypes = {@ParamType(type = Sequence.class, generic1 = String.class)}, + defaultValue = "[]", + named = true, + positional = true, + doc = + "A sequence of names of environment variables. These variables are made " + + " available with their current value taken from the shell environment" + + " when the target that returns this provider is executed, either as a" + + " test or via the run command. If a variable is contained in both " + + "environment and inherited_environment, the value" + + " inherited from the shell environment will take precedence if set.") + }, + selfCall = true) + @StarlarkConstructor + RunEnvironmentInfoApi constructor( + Dict environment, // expected + Sequence inheritedEnvironment /* expected */) + throws EvalException; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/TestEnvironmentInfoApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/TestEnvironmentInfoApi.java deleted file mode 100644 index ed11b24af45ddf..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/TestEnvironmentInfoApi.java +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright 2018 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.devtools.build.lib.starlarkbuildapi.test; - -import com.google.devtools.build.lib.starlarkbuildapi.core.StructApi; -import java.util.List; -import java.util.Map; -import net.starlark.java.annot.StarlarkBuiltin; -import net.starlark.java.annot.StarlarkMethod; - -/** Provider containing any additional environment variables for use in the test action. */ -@StarlarkBuiltin(name = "TestEnvironmentInfo", doc = "", documented = false) -public interface TestEnvironmentInfoApi extends StructApi { - - @StarlarkMethod( - name = "environment", - doc = "A dict containing environment variables which should be set on the test action.", - structField = true) - Map getEnvironment(); - - @StarlarkMethod( - name = "inherited_environment", - doc = "A list of variables that the test action should inherit from the shell environment.", - structField = true) - List getInheritedEnvironment(); -} diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/TestingModuleApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/TestingModuleApi.java index 1570f8039bbfbe..dd91fdc7739a1b 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/TestingModuleApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/TestingModuleApi.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.starlarkbuildapi.test; +import com.google.devtools.build.lib.starlarkbuildapi.RunEnvironmentInfoApi; import net.starlark.java.annot.Param; import net.starlark.java.annot.ParamType; import net.starlark.java.annot.StarlarkBuiltin; @@ -49,12 +50,12 @@ public interface TestingModuleApi extends StarlarkValue { ExecutionInfoApi executionInfo(Dict requirements // expected ) throws EvalException; - // TODO(bazel-team): Change this function to be the actual TestEnvironmentInfo.PROVIDER. @StarlarkMethod( name = "TestEnvironment", doc = - "Creates a new test environment provider. Use this provider to specify extra" - + "environment variables to be made available during test execution.", + "Deprecated: Use RunEnvironmentInfo instead. Creates a new test environment " + + "provider. Use this provider to specify extra environment variables to be made " + + "available during test execution.", parameters = { @Param( name = "environment", @@ -76,7 +77,7 @@ ExecutionInfoApi executionInfo(Dict requirements // expec + " and inherited_environment, the value inherited from the" + " shell environment will take precedence if set.") }) - TestEnvironmentInfoApi testEnvironment( + RunEnvironmentInfoApi testEnvironment( Dict environment, // expected Sequence inheritedEnvironment /* expected */) throws EvalException; diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java index d48d266db2f373..8ff56fbc18f40d 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java @@ -18,6 +18,8 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import java.util.HashMap; +import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -32,7 +34,7 @@ public void compoundEnvOrdering() { ActionEnvironment.create( ImmutableMap.of("FOO", "foo1", "BAR", "bar"), ImmutableSet.of("baz")); // entries added by env2 override the existing entries - ActionEnvironment env2 = env1.addFixedVariables(ImmutableMap.of("FOO", "foo2")); + ActionEnvironment env2 = env1.withAdditionalFixedVariables(ImmutableMap.of("FOO", "foo2")); assertThat(env1.getFixedEnv()).containsExactly("FOO", "foo1", "BAR", "bar"); assertThat(env1.getInheritedEnv()).containsExactly("baz"); @@ -40,4 +42,40 @@ public void compoundEnvOrdering() { assertThat(env2.getFixedEnv()).containsExactly("FOO", "foo2", "BAR", "bar"); assertThat(env2.getInheritedEnv()).containsExactly("baz"); } + + @Test + public void fixedInheritedInteraction() { + ActionEnvironment env = + ActionEnvironment.create( + ImmutableMap.of("FIXED_ONLY", "fixed"), ImmutableSet.of("INHERITED_ONLY")) + .withAdditionalVariables( + ImmutableMap.of("FIXED_AND_INHERITED", "fixed"), + ImmutableSet.of("FIXED_AND_INHERITED")); + Map clientEnv = + ImmutableMap.of("INHERITED_ONLY", "inherited", "FIXED_AND_INHERITED", "inherited"); + Map result = new HashMap<>(); + env.resolve(result, clientEnv); + + assertThat(result) + .containsExactly( + "FIXED_ONLY", + "fixed", + "FIXED_AND_INHERITED", + "inherited", + "INHERITED_ONLY", + "inherited"); + } + + @Test + public void emptyEnvironmentInterning() { + ActionEnvironment emptyEnvironment = + ActionEnvironment.create(ImmutableMap.of(), ImmutableSet.of()); + assertThat(emptyEnvironment).isSameInstanceAs(ActionEnvironment.EMPTY); + + ActionEnvironment base = + ActionEnvironment.create(ImmutableMap.of("FOO", "foo1"), ImmutableSet.of("baz")); + assertThat(base.withAdditionalFixedVariables(ImmutableMap.of())).isSameInstanceAs(base); + assertThat(base.withAdditionalVariables(ImmutableMap.of(), ImmutableSet.of())) + .isSameInstanceAs(base); + } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/test/BUILD b/src/test/java/com/google/devtools/build/lib/rules/test/BUILD index 8f34480f06425c..764210c5b4484f 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/test/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/test/BUILD @@ -20,8 +20,8 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", + "//src/main/java/com/google/devtools/build/lib/analysis:run_environment_info", "//src/main/java/com/google/devtools/build/lib/analysis:test/execution_info", - "//src/main/java/com/google/devtools/build/lib/analysis:test/test_environment_info", "//src/test/java/com/google/devtools/build/lib/analysis/util", "//third_party:junit4", "//third_party:truth", diff --git a/src/test/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModuleTest.java b/src/test/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModuleTest.java index 77943fef1eaaf7..4a63b185ab9f2e 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModuleTest.java @@ -16,8 +16,8 @@ import static com.google.common.truth.Truth.assertThat; import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.RunEnvironmentInfo; import com.google.devtools.build.lib.analysis.test.ExecutionInfo; -import com.google.devtools.build.lib.analysis.test.TestEnvironmentInfo; import com.google.devtools.build.lib.analysis.test.TestProvider; import com.google.devtools.build.lib.analysis.test.TestRunnerAction; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; @@ -74,7 +74,7 @@ public void testStarlarkRulePropagatesTestEnvironmentProvider() throws Exception ")"); ConfiguredTarget starlarkTarget = getConfiguredTarget("//examples/apple_starlark:my_target"); - TestEnvironmentInfo provider = starlarkTarget.get(TestEnvironmentInfo.PROVIDER); + RunEnvironmentInfo provider = starlarkTarget.get(RunEnvironmentInfo.PROVIDER); assertThat(provider.getEnvironment().get("XCODE_VERSION_OVERRIDE")).isEqualTo("7.3.1"); } @@ -105,7 +105,8 @@ public void testStarlarkRulePropagatesTestEnvironmentProviderWithInheritedEnv() ")"); ConfiguredTarget starlarkTarget = getConfiguredTarget("//examples/apple_starlark:my_target"); - TestEnvironmentInfo provider = starlarkTarget.get(TestEnvironmentInfo.PROVIDER); + RunEnvironmentInfo provider = + (RunEnvironmentInfo) starlarkTarget.get(RunEnvironmentInfo.PROVIDER.getKey()); assertThat(provider.getEnvironment()).containsEntry("XCODE_VERSION_OVERRIDE", "7.3.1"); assertThat(provider.getInheritedEnvironment()).contains("DEVELOPER_DIR"); diff --git a/src/test/java/com/google/devtools/build/lib/starlark/BUILD b/src/test/java/com/google/devtools/build/lib/starlark/BUILD index 4313647f7a72fe..0f8a78aecbf428 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/BUILD +++ b/src/test/java/com/google/devtools/build/lib/starlark/BUILD @@ -41,6 +41,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:file_provider", "//src/main/java/com/google/devtools/build/lib/analysis:resolved_toolchain_context", + "//src/main/java/com/google/devtools/build/lib/analysis:run_environment_info", "//src/main/java/com/google/devtools/build/lib/analysis:starlark/args", "//src/main/java/com/google/devtools/build/lib/analysis:starlark/starlark_exec_group_collection", "//src/main/java/com/google/devtools/build/lib/analysis:test/analysis_failure", diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java index cc186b0d942af2..c782feff365f67 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java @@ -22,6 +22,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.truth.Correspondence; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; @@ -30,6 +31,7 @@ import com.google.devtools.build.lib.analysis.FileProvider; import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.OutputGroupInfo; +import com.google.devtools.build.lib.analysis.RunEnvironmentInfo; import com.google.devtools.build.lib.analysis.RunfilesProvider; import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition; import com.google.devtools.build.lib.analysis.configuredtargets.FileConfiguredTarget; @@ -45,6 +47,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.Depset; import com.google.devtools.build.lib.collect.nestedset.NestedSet; +import com.google.devtools.build.lib.events.EventKind; import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; import com.google.devtools.build.lib.packages.BuildSetting; import com.google.devtools.build.lib.packages.FunctionSplitTransitionAllowlist; @@ -3766,4 +3769,88 @@ public void bzlFileWithErrorsLoadedThroughMultipleLoadPathsWithTheLatterOneNotHa .hasMessageThat() .contains("compilation of module 'test/starlark/error.bzl' failed"); } + + @Test + public void testStarlarkRulePropagatesRunEnvironmentProvider() throws Exception { + scratch.file( + "examples/rules.bzl", + "def my_rule_impl(ctx):", + " script = ctx.actions.declare_file(ctx.attr.name)", + " ctx.actions.write(script, '', is_executable = True)", + " run_env = RunEnvironmentInfo(", + " {'FIXED': 'fixed'},", + " ['INHERITED']", + " )", + " return [", + " DefaultInfo(executable = script),", + " run_env,", + " ]", + "my_rule = rule(", + " implementation = my_rule_impl,", + " attrs = {},", + " executable = True,", + ")"); + scratch.file( + "examples/BUILD", + "load(':rules.bzl', 'my_rule')", + "my_rule(", + " name = 'my_target',", + ")"); + + ConfiguredTarget starlarkTarget = getConfiguredTarget("//examples:my_target"); + RunEnvironmentInfo provider = starlarkTarget.get(RunEnvironmentInfo.PROVIDER); + + assertThat(provider.getEnvironment()).containsExactly("FIXED", "fixed"); + assertThat(provider.getInheritedEnvironment()).containsExactly("INHERITED"); + } + + @Test + public void nonExecutableStarlarkRuleReturningRunEnvironmentInfoErrors() throws Exception { + scratch.file( + "examples/rules.bzl", + "def my_rule_impl(ctx):", + " return [RunEnvironmentInfo()]", + "my_rule = rule(", + " implementation = my_rule_impl,", + " attrs = {},", + ")"); + scratch.file( + "examples/BUILD", + "load(':rules.bzl', 'my_rule')", + "my_rule(", + " name = 'my_target',", + ")"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//examples:my_target"); + assertContainsEvent( + "in my_rule rule //examples:my_target: Returning RunEnvironmentInfo from a non-executable," + + " non-test target has no effect", + ImmutableSet.of(EventKind.ERROR)); + } + + @Test + public void nonExecutableStarlarkRuleReturningTestEnvironmentProducesAWarning() throws Exception { + scratch.file( + "examples/rules.bzl", + "def my_rule_impl(ctx):", + " return [testing.TestEnvironment(environment = {})]", + "my_rule = rule(", + " implementation = my_rule_impl,", + " attrs = {},", + ")"); + scratch.file( + "examples/BUILD", + "load(':rules.bzl', 'my_rule')", + "my_rule(", + " name = 'my_target',", + ")"); + + reporter.removeHandler(failFastHandler); + getConfiguredTarget("//examples:my_target"); + assertContainsEvent( + "in my_rule rule //examples:my_target: Returning RunEnvironmentInfo from a non-executable," + + " non-test target has no effect", + ImmutableSet.of(EventKind.WARNING)); + } } diff --git a/src/test/shell/bazel/bazel_rules_test.sh b/src/test/shell/bazel/bazel_rules_test.sh index 5650f0d458e283..5807d4fd45085e 100755 --- a/src/test/shell/bazel/bazel_rules_test.sh +++ b/src/test/shell/bazel/bazel_rules_test.sh @@ -612,6 +612,7 @@ if not "%FIXED_ONLY%" == "fixed" exit /B 1 if not "%FIXED_AND_INHERITED%" == "inherited" exit /B 1 if not "%FIXED_AND_INHERITED_BUT_NOT_SET%" == "fixed" exit /B 1 if not "%INHERITED_ONLY%" == "inherited" exit /B 1 +if defined INHERITED_BUT_UNSET exit /B 1 """ EOF else @@ -621,7 +622,8 @@ _SCRIPT_CONTENT = """#!/bin/bash [[ "$FIXED_ONLY" == "fixed" \ && "$FIXED_AND_INHERITED" == "inherited" \ && "$FIXED_AND_INHERITED_BUT_NOT_SET" == "fixed" \ - && "$INHERITED_ONLY" == "inherited" ]] + && "$INHERITED_ONLY" == "inherited" \ + && -z "${INHERITED_BUT_UNSET+default}" ]] """ EOF fi @@ -644,13 +646,14 @@ def _my_test_impl(ctx): "FIXED_AND_INHERITED", "FIXED_AND_INHERITED_BUT_NOT_SET", "INHERITED_ONLY", + "INHERITED_BUT_UNSET", ] ) return [ DefaultInfo( executable = test_sh, ), - test_env + test_env, ] my_test = rule( @@ -661,7 +664,83 @@ my_test = rule( EOF FIXED_AND_INHERITED=inherited INHERITED_ONLY=inherited \ - bazel test //pkg:my_test &> /dev/null || fail "Test should pass" + bazel test //pkg:my_test >$TEST_log 2>&1 || fail "Test should pass" +} + +function test_starlark_rule_with_run_environment() { + mkdir pkg + cat >pkg/BUILD <<'EOF' +load(":rules.bzl", "my_executable") +my_executable( + name = "my_executable", +) +EOF + + # On Windows this file needs to be acceptable by CreateProcessW(), rather + # than a Bourne script. + if "$is_windows"; then + cat >pkg/rules.bzl <<'EOF' +_SCRIPT_EXT = ".bat" +_SCRIPT_CONTENT = """@ECHO OFF +if not "%FIXED_ONLY%" == "fixed" exit /B 1 +if not "%FIXED_AND_INHERITED%" == "inherited" exit /B 1 +if not "%FIXED_AND_INHERITED_BUT_NOT_SET%" == "fixed" exit /B 1 +if not "%INHERITED_ONLY%" == "inherited" exit /B 1 +if defined INHERITED_BUT_UNSET exit /B 1 +""" +EOF + else + cat >pkg/rules.bzl <<'EOF' +_SCRIPT_EXT = ".sh" +_SCRIPT_CONTENT = """#!/bin/bash +set -x +env +[[ "$FIXED_ONLY" == "fixed" \ + && "$FIXED_AND_INHERITED" == "inherited" \ + && "$FIXED_AND_INHERITED_BUT_NOT_SET" == "fixed" \ + && "$INHERITED_ONLY" == "inherited" \ + && -z "${INHERITED_BUT_UNSET+default}" ]] +""" +EOF + fi + + cat >>pkg/rules.bzl <<'EOF' +def _my_executable_impl(ctx): + executable_sh = ctx.actions.declare_file(ctx.attr.name + _SCRIPT_EXT) + ctx.actions.write( + output = executable_sh, + content = _SCRIPT_CONTENT, + is_executable = True, + ) + run_env = RunEnvironmentInfo( + { + "FIXED_AND_INHERITED": "fixed", + "FIXED_AND_INHERITED_BUT_NOT_SET": "fixed", + "FIXED_ONLY": "fixed", + }, + [ + "FIXED_AND_INHERITED", + "FIXED_AND_INHERITED_BUT_NOT_SET", + "INHERITED_ONLY", + "INHERITED_BUT_UNSET", + ] + ) + return [ + DefaultInfo( + executable = executable_sh, + ), + run_env, + ] + +my_executable = rule( + implementation = _my_executable_impl, + attrs = {}, + executable = True, +) +EOF + + FIXED_AND_INHERITED=inherited INHERITED_ONLY=inherited \ + bazel run //pkg:my_executable >$TEST_log 2>&1 || fail "Binary should have exit code 0" } run_suite "rules test"