From 1e405417457210191a4f4b3a7ce7950720a16cc7 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 12 May 2022 01:26:15 -0700 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. Fixes #7364 Fixes #15224 Fixes #15225 Closes #15232. PiperOrigin-RevId: 448185352 --- .../google/devtools/build/lib/analysis/BUILD | 27 +++-- .../analysis/RuleConfiguredTargetBuilder.java | 6 +- .../lib/analysis/RunEnvironmentInfo.java | 106 ++++++++++++++++++ .../build/lib/analysis/RunfilesSupport.java | 4 +- .../analysis/starlark/StarlarkModules.java | 2 + .../StarlarkRuleConfiguredTargetUtil.java | 12 ++ .../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 | 18 ++- .../RunEnvironmentInfoApi.java | 103 +++++++++++++++++ .../test/TestEnvironmentInfoApi.java | 38 ------- .../test/TestingModuleApi.java | 9 +- .../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 +++++++++++++- 19 files changed, 440 insertions(+), 142 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/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 721a422eba3e78..97b8b8cfd84108 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", @@ -390,7 +389,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", "//src/main/java/com/google/devtools/build/lib/causes", "//src/main/java/com/google/devtools/build/lib/cmdline", - "//src/main/java/com/google/devtools/build/lib/cmdline:LabelValidator", "//src/main/java/com/google/devtools/build/lib/collect", "//src/main/java/com/google/devtools/build/lib/collect/compacthashset", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", @@ -1003,6 +1001,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"], @@ -2412,17 +2422,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 244a575a1ca5f2..fed227896f0ccb 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,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 0166b2d5e971d2..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(b/176554800): 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..3a18f0afb86a14 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/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 15337da99ff737..2763e8412795e2 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..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 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..d398b1e0c00abf 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( @@ -523,7 +533,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/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 3f237073bfb5f7..0ab20ce6319a18 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/BUILD +++ b/src/test/java/com/google/devtools/build/lib/starlark/BUILD @@ -45,6 +45,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_test_result_info", 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 c7ffc30df3adfb..52fbac82d2ef1c 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.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; @@ -29,6 +30,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; @@ -42,6 +44,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; @@ -3570,4 +3573,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"