From 93ac9a66d37fb91efeb5e28cfd312ef3cfcf9f0b Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 13 Apr 2022 11:47:20 +0200 Subject: [PATCH] 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. --- .../google/devtools/build/lib/analysis/BUILD | 27 +++--- .../analysis/RuleConfiguredTargetBuilder.java | 7 +- .../lib/analysis/RunEnvironmentInfo.java | 94 ++++++++++++++++++ .../build/lib/analysis/RunfilesSupport.java | 4 +- .../analysis/starlark/StarlarkModules.java | 2 + .../StarlarkRuleConfiguredTargetUtil.java | 10 ++ .../analysis/test/TestEnvironmentInfo.java | 63 ------------ .../com/google/devtools/build/lib/rules/BUILD | 2 +- .../lib/rules/test/StarlarkTestingModule.java | 9 +- .../devtools/build/lib/runtime/commands/BUILD | 1 + .../lib/runtime/commands/RunCommand.java | 16 +++- .../RunEnvironmentInfoApi.java | 96 +++++++++++++++++++ .../test/TestEnvironmentInfoApi.java | 20 +--- .../test/TestingModuleApi.java | 6 +- .../devtools/build/lib/rules/test/BUILD | 2 +- .../rules/test/StarlarkTestingModuleTest.java | 7 +- .../google/devtools/build/lib/starlark/BUILD | 1 + .../lib/starlark/StarlarkIntegrationTest.java | 85 ++++++++++++++++ src/test/shell/bazel/bazel_rules_test.sh | 85 +++++++++++++++- 19 files changed, 418 insertions(+), 119 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 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 ff8ac3729d5a90..7781ab2b3646a7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -133,7 +133,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", @@ -340,6 +339,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", @@ -357,7 +357,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", @@ -1002,6 +1001,19 @@ 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/com/google/devtools/build/lib/starlarkbuildapi/test", + "//src/main/java/net/starlark/java/eval", + "//third_party:guava", + ], +) + java_library( name = "rule_definition_environment", srcs = ["RuleDefinitionEnvironment.java"], @@ -2407,17 +2419,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 88eabf31f8a885..5c9873d1841010 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; @@ -470,9 +469,9 @@ 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..6038acd6b6e0d5 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/RunEnvironmentInfo.java @@ -0,0 +1,94 @@ +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 com.google.devtools.build.lib.starlarkbuildapi.test.TestEnvironmentInfoApi; +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; + +@Immutable +public final class RunEnvironmentInfo extends NativeInfo implements RunEnvironmentInfoApi, + TestEnvironmentInfoApi { + + /** + * 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 Map 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")), + 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 e4d3e24e3a244a..4c4d96cb14bf5a 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 9a74ec1ef0d6c0..b8b839b565dcf3 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,15 @@ 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/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 b27e82580d793f..494fe2dadd62a7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -123,12 +123,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/test/StarlarkTestingModule.java b/src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java index 1c6fcb6c23dad9..61dbc457a60479 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")), + 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 df7249438877ba..2de67618a3c8c0 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 @@ -47,6 +47,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 442fd4477eb69b..090814b1761b71 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; @@ -32,6 +34,7 @@ import com.google.devtools.build.lib.analysis.AliasProvider; 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.BuildConfigurationValue; @@ -487,11 +490,18 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti } else { workingDir = targetToRunRunfilesDir != null ? targetToRunRunfilesDir : env.getWorkingDirectory(); + ActionEnvironment actionEnvironment = ActionEnvironment.EMPTY; if (targetToRunRunfilesSupport != null) { - targetToRunRunfilesSupport - .getActionEnvironment() - .resolve(runEnvironment, env.getClientEnv()); + actionEnvironment = targetToRunRunfilesSupport.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( 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..3293c382fed0f5 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/RunEnvironmentInfoApi.java @@ -0,0 +1,96 @@ +// 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; + +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 dict containing environment variables which should be set when executing the rule" + + " that returns this provider.", + structField = true) + Map getEnvironment(); + + @StarlarkMethod( + name = "inherited_environment", + doc = "A list of variables that should be inherited from the shell environment when executing" + + " the rule that returns this provider.", + 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 index ed11b24af45ddf..a7b999e6a96832 100644 --- 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 @@ -14,25 +14,9 @@ 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 com.google.devtools.build.lib.starlarkbuildapi.RunEnvironmentInfoApi; 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(); -} +public interface TestEnvironmentInfoApi extends RunEnvironmentInfoApi {} 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..45e8ad662f2bd7 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 @@ -49,12 +49,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", 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..ded75f02d904ec 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 f6c662584a2e76..36ac1ee50f3fae 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/BUILD +++ b/src/test/java/com/google/devtools/build/lib/starlark/BUILD @@ -46,6 +46,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 78ebdfd9d7b08d..56541d502fb6e4 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; @@ -3806,4 +3809,86 @@ 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"