Skip to content

Commit dbdfa07

Browse files
fmeumfmeum
authored
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); ^ ```
1 parent e4bc370 commit dbdfa07

24 files changed

+519
-160
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java‎

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,8 @@
4646
public final class ActionEnvironment {
4747

4848
/**
49-
* A map of environment variables together with a list of variables to inherit from the shell
50-
* environment.
49+
* A map of environment variables and their values together with a list of variables whose values
50+
* should be inherited from the client environment.
5151
*/
5252
public interface EnvironmentVariables {
5353

@@ -78,14 +78,14 @@ default int size() {
7878

7979
/**
8080
* An {@link EnvironmentVariables} that combines variables from two different environments without
81-
* allocation a new map.
81+
* allocating a new map.
8282
*/
8383
static class CompoundEnvironmentVariables implements EnvironmentVariables {
8484

8585
static EnvironmentVariables create(
8686
Map<String, String> fixedVars, Set<String> inheritedVars, EnvironmentVariables base) {
87-
if (fixedVars.isEmpty() && inheritedVars.isEmpty() && base.isEmpty()) {
88-
return EMPTY_ENVIRONMENT_VARIABLES;
87+
if (fixedVars.isEmpty() && inheritedVars.isEmpty()) {
88+
return base;
8989
}
9090
return new CompoundEnvironmentVariables(fixedVars, inheritedVars, base);
9191
}
@@ -168,8 +168,6 @@ public ImmutableSet<String> getInheritedEnvironment() {
168168
* given map. Returns these two parts as a new {@link ActionEnvironment} instance.
169169
*/
170170
public static ActionEnvironment split(Map<String, String> env) {
171-
// Care needs to be taken that the two sets don't overlap - the order in which the two parts are
172-
// combined later is undefined.
173171
Map<String, String> fixedEnv = new TreeMap<>();
174172
Set<String> inheritedEnv = new TreeSet<>();
175173
for (Map.Entry<String, String> entry : env.entrySet()) {
@@ -190,9 +188,11 @@ private ActionEnvironment(EnvironmentVariables vars) {
190188
}
191189

192190
/**
193-
* Creates a new action environment. The order in which the environments are combined is
194-
* undefined, so callers need to take care that the key set of the {@code fixedEnv} map and the
195-
* set of {@code inheritedEnv} elements are disjoint.
191+
* Creates a new action environment. If an environment variable is contained in both {@link
192+
* EnvironmentVariables#getFixedEnvironment()} and {@link
193+
* EnvironmentVariables#getInheritedEnvironment()}, the result of {@link
194+
* ActionEnvironment#resolve(Map, Map)} will contain the value inherited from the client
195+
* environment.
196196
*/
197197
private static ActionEnvironment create(EnvironmentVariables vars) {
198198
if (vars.isEmpty()) {
@@ -201,6 +201,12 @@ private static ActionEnvironment create(EnvironmentVariables vars) {
201201
return new ActionEnvironment(vars);
202202
}
203203

204+
/**
205+
* Creates a new action environment. If an environment variable is contained both as a key in
206+
* {@code fixedEnv} and in {@code inheritedEnv}, the result of {@link
207+
* ActionEnvironment#resolve(Map, Map)} will contain the value inherited from the client
208+
* environment.
209+
*/
204210
public static ActionEnvironment create(
205211
Map<String, String> fixedEnv, ImmutableSet<String> inheritedEnv) {
206212
return ActionEnvironment.create(SimpleEnvironmentVariables.create(fixedEnv, inheritedEnv));
@@ -214,21 +220,29 @@ public static ActionEnvironment create(Map<String, String> fixedEnv) {
214220
* Returns a copy of the environment with the given fixed variables added to it, <em>overwriting
215221
* any existing occurrences of those variables</em>.
216222
*/
217-
public ActionEnvironment addFixedVariables(Map<String, String> fixedVars) {
218-
return ActionEnvironment.create(
219-
CompoundEnvironmentVariables.create(fixedVars, ImmutableSet.of(), vars));
223+
public ActionEnvironment withAdditionalFixedVariables(Map<String, String> fixedVars) {
224+
return withAdditionalVariables(fixedVars, ImmutableSet.of());
220225
}
221226

222227
/**
223228
* Returns a copy of the environment with the given fixed and inherited variables added to it,
224229
* <em>overwriting any existing occurrences of those variables</em>.
225230
*/
226-
public ActionEnvironment addVariables(Map<String, String> fixedVars, Set<String> inheritedVars) {
227-
return ActionEnvironment.create(
228-
CompoundEnvironmentVariables.create(fixedVars, inheritedVars, vars));
231+
public ActionEnvironment withAdditionalVariables(
232+
Map<String, String> fixedVars, Set<String> inheritedVars) {
233+
EnvironmentVariables newVars =
234+
CompoundEnvironmentVariables.create(fixedVars, inheritedVars, vars);
235+
if (newVars == vars) {
236+
return this;
237+
}
238+
return ActionEnvironment.create(newVars);
229239
}
230240

231-
/** Returns the combined size of the fixed and inherited environments. */
241+
/**
242+
* Returns an upper bound on the combined size of the fixed and inherited environments. A call to
243+
* {@link ActionEnvironment#resolve(Map, Map)} may add less entries than this number if
244+
* environment variables are contained in both the fixed and the inherited environment.
245+
*/
232246
public int size() {
233247
return vars.size();
234248
}

‎src/main/java/com/google/devtools/build/lib/analysis/BUILD‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,6 @@ java_library(
134134
":test/execution_info",
135135
":test/instrumented_files_info",
136136
":test/test_configuration",
137-
":test/test_environment_info",
138137
":test/test_sharding_strategy",
139138
":test/test_trimming_transition_factory",
140139
":toolchain_collection",
@@ -339,6 +338,7 @@ java_library(
339338
":resolved_toolchain_context",
340339
":rule_configured_object_value",
341340
":rule_definition_environment",
341+
":run_environment_info",
342342
":starlark/args",
343343
":starlark/bazel_build_api_globals",
344344
":starlark/function_transition_util",
@@ -356,7 +356,6 @@ java_library(
356356
":test/execution_info",
357357
":test/instrumented_files_info",
358358
":test/test_configuration",
359-
":test/test_environment_info",
360359
":test/test_sharding_strategy",
361360
":toolchain_collection",
362361
":toolchain_context",
@@ -999,6 +998,18 @@ java_library(
999998
],
1000999
)
10011000

1001+
java_library(
1002+
name = "run_environment_info",
1003+
srcs = ["RunEnvironmentInfo.java"],
1004+
deps = [
1005+
"//src/main/java/com/google/devtools/build/lib/concurrent",
1006+
"//src/main/java/com/google/devtools/build/lib/packages",
1007+
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
1008+
"//src/main/java/net/starlark/java/eval",
1009+
"//third_party:guava",
1010+
],
1011+
)
1012+
10021013
java_library(
10031014
name = "rule_definition_environment",
10041015
srcs = ["RuleDefinitionEnvironment.java"],
@@ -2365,17 +2376,6 @@ java_library(
23652376
],
23662377
)
23672378

2368-
java_library(
2369-
name = "test/test_environment_info",
2370-
srcs = ["test/TestEnvironmentInfo.java"],
2371-
deps = [
2372-
"//src/main/java/com/google/devtools/build/lib/concurrent",
2373-
"//src/main/java/com/google/devtools/build/lib/packages",
2374-
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test",
2375-
"//third_party:guava",
2376-
],
2377-
)
2378-
23792379
java_library(
23802380
name = "test/test_sharding_strategy",
23812381
srcs = ["test/TestShardingStrategy.java"],

‎src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@
4040
import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo;
4141
import com.google.devtools.build.lib.analysis.test.TestActionBuilder;
4242
import com.google.devtools.build.lib.analysis.test.TestConfiguration;
43-
import com.google.devtools.build.lib.analysis.test.TestEnvironmentInfo;
4443
import com.google.devtools.build.lib.analysis.test.TestProvider;
4544
import com.google.devtools.build.lib.analysis.test.TestProvider.TestParams;
4645
import com.google.devtools.build.lib.analysis.test.TestTagsProvider;
@@ -465,9 +464,8 @@ private TestProvider initializeTestProvider(FilesToRunProvider filesToRunProvide
465464
providersBuilder.getProvider(
466465
InstrumentedFilesInfo.STARLARK_CONSTRUCTOR.getKey()));
467466

468-
TestEnvironmentInfo environmentProvider =
469-
(TestEnvironmentInfo)
470-
providersBuilder.getProvider(TestEnvironmentInfo.PROVIDER.getKey());
467+
RunEnvironmentInfo environmentProvider =
468+
(RunEnvironmentInfo) providersBuilder.getProvider(RunEnvironmentInfo.PROVIDER.getKey());
471469
if (environmentProvider != null) {
472470
testActionBuilder.addExtraEnv(environmentProvider.getEnvironment());
473471
testActionBuilder.addExtraInheritedEnv(environmentProvider.getInheritedEnvironment());

‎src/main/java/com/google/devtools/build/lib/analysis/RunEnvironmentInfo.java‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
// Copyright 2022 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package com.google.devtools.build.lib.analysis;
16+
17+
import com.google.common.base.Preconditions;
18+
import com.google.common.collect.ImmutableList;
19+
import com.google.common.collect.ImmutableMap;
20+
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
21+
import com.google.devtools.build.lib.packages.BuiltinProvider;
22+
import com.google.devtools.build.lib.packages.NativeInfo;
23+
import com.google.devtools.build.lib.starlarkbuildapi.RunEnvironmentInfoApi;
24+
import java.util.List;
25+
import java.util.Map;
26+
import net.starlark.java.eval.Dict;
27+
import net.starlark.java.eval.EvalException;
28+
import net.starlark.java.eval.Sequence;
29+
import net.starlark.java.eval.StarlarkList;
30+
31+
/**
32+
* Provider containing any additional environment variables for use in the executable or test
33+
* action.
34+
*/
35+
@Immutable
36+
public final class RunEnvironmentInfo extends NativeInfo implements RunEnvironmentInfoApi {
37+
38+
/** Singleton instance of the provider type for {@link DefaultInfo}. */
39+
public static final RunEnvironmentInfoProvider PROVIDER = new RunEnvironmentInfoProvider();
40+
41+
private final ImmutableMap<String, String> environment;
42+
private final ImmutableList<String> inheritedEnvironment;
43+
private final boolean shouldErrorOnNonExecutableRule;
44+
45+
/** Constructs a new provider with the given fixed and inherited environment variables. */
46+
public RunEnvironmentInfo(
47+
Map<String, String> environment,
48+
List<String> inheritedEnvironment,
49+
boolean shouldErrorOnNonExecutableRule) {
50+
this.environment = ImmutableMap.copyOf(Preconditions.checkNotNull(environment));
51+
this.inheritedEnvironment =
52+
ImmutableList.copyOf(Preconditions.checkNotNull(inheritedEnvironment));
53+
this.shouldErrorOnNonExecutableRule = shouldErrorOnNonExecutableRule;
54+
}
55+
56+
@Override
57+
public RunEnvironmentInfoProvider getProvider() {
58+
return PROVIDER;
59+
}
60+
61+
/**
62+
* Returns environment variables which should be set when the target advertising this provider is
63+
* executed.
64+
*/
65+
@Override
66+
public ImmutableMap<String, String> getEnvironment() {
67+
return environment;
68+
}
69+
70+
/**
71+
* Returns names of environment variables whose value should be inherited from the shell
72+
* environment when the target advertising this provider is executed.
73+
*/
74+
@Override
75+
public ImmutableList<String> getInheritedEnvironment() {
76+
return inheritedEnvironment;
77+
}
78+
79+
/**
80+
* Returns whether advertising this provider on a non-executable (and thus non-test) rule should
81+
* result in an error or a warning. The latter is required to not break testing.TestEnvironment,
82+
* which is now implemented via RunEnvironmentInfo.
83+
*/
84+
public boolean shouldErrorOnNonExecutableRule() {
85+
return shouldErrorOnNonExecutableRule;
86+
}
87+
88+
/** Provider implementation for {@link RunEnvironmentInfoApi}. */
89+
public static class RunEnvironmentInfoProvider extends BuiltinProvider<RunEnvironmentInfo>
90+
implements RunEnvironmentInfoApi.RunEnvironmentInfoApiProvider {
91+
92+
private RunEnvironmentInfoProvider() {
93+
super("RunEnvironmentInfo", RunEnvironmentInfo.class);
94+
}
95+
96+
@Override
97+
public RunEnvironmentInfoApi constructor(
98+
Dict<?, ?> environment, Sequence<?> inheritedEnvironment) throws EvalException {
99+
return new RunEnvironmentInfo(
100+
Dict.cast(environment, String.class, String.class, "environment"),
101+
StarlarkList.immutableCopyOf(
102+
Sequence.cast(inheritedEnvironment, String.class, "inherited_environment")),
103+
/* shouldErrorOnNonExecutableRule */ true);
104+
}
105+
}
106+
}

‎src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -475,9 +475,7 @@ private static CommandLine computeArgs(RuleContext ruleContext, CommandLine addi
475475
}
476476

477477
private static ActionEnvironment computeActionEnvironment(RuleContext ruleContext) {
478-
// Currently, "env" and "env_inherit" are not added to Starlark-defined rules (unlike "args"),
479-
// in order to avoid breaking existing Starlark rules that use those attribute names.
480-
// TODO(brandjon): Support "env" and "env_inherit" for Starlark-defined rules.
478+
// Executable Starlark rules can use RunEnvironmentInfo to specify environment variables.
481479
boolean isNativeRule =
482480
ruleContext.getRule().getRuleClassObject().getRuleDefinitionEnvironmentLabel() == null;
483481
if (!isNativeRule

‎src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkModules.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import com.google.devtools.build.lib.analysis.ActionsProvider;
1919
import com.google.devtools.build.lib.analysis.DefaultInfo;
2020
import com.google.devtools.build.lib.analysis.OutputGroupInfo;
21+
import com.google.devtools.build.lib.analysis.RunEnvironmentInfo;
2122
import com.google.devtools.build.lib.packages.StarlarkLibrary;
2223
import com.google.devtools.build.lib.packages.StructProvider;
2324
import net.starlark.java.eval.Starlark;
@@ -44,5 +45,6 @@ public static void addPredeclared(ImmutableMap.Builder<String, Object> predeclar
4445
predeclared.put("OutputGroupInfo", OutputGroupInfo.STARLARK_CONSTRUCTOR);
4546
predeclared.put("Actions", ActionsProvider.INSTANCE);
4647
predeclared.put("DefaultInfo", DefaultInfo.PROVIDER);
48+
predeclared.put("RunEnvironmentInfo", RunEnvironmentInfo.PROVIDER);
4749
}
4850
}

‎src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import com.google.devtools.build.lib.analysis.DefaultInfo;
2525
import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
2626
import com.google.devtools.build.lib.analysis.RuleContext;
27+
import com.google.devtools.build.lib.analysis.RunEnvironmentInfo;
2728
import com.google.devtools.build.lib.analysis.Runfiles;
2829
import com.google.devtools.build.lib.analysis.RunfilesProvider;
2930
import com.google.devtools.build.lib.analysis.RunfilesSupport;
@@ -348,6 +349,17 @@ private static void addProviders(
348349
if (getProviderKey(declaredProvider).equals(DefaultInfo.PROVIDER.getKey())) {
349350
parseDefaultProviderFields((DefaultInfo) declaredProvider, context, builder);
350351
defaultProviderProvidedExplicitly = true;
352+
} else if (getProviderKey(declaredProvider).equals(RunEnvironmentInfo.PROVIDER.getKey())
353+
&& !(context.isExecutable() || context.getRuleContext().isTestTarget())) {
354+
String message =
355+
"Returning RunEnvironmentInfo from a non-executable, non-test target has no effect";
356+
RunEnvironmentInfo runEnvironmentInfo = (RunEnvironmentInfo) declaredProvider;
357+
if (runEnvironmentInfo.shouldErrorOnNonExecutableRule()) {
358+
context.getRuleContext().ruleError(message);
359+
} else {
360+
context.getRuleContext().ruleWarning(message);
361+
builder.addStarlarkDeclaredProvider(declaredProvider);
362+
}
351363
} else {
352364
builder.addStarlarkDeclaredProvider(declaredProvider);
353365
}

‎src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,7 @@ private TestParams createTestAction(int shards)
453453
testProperties,
454454
runfilesSupport
455455
.getActionEnvironment()
456-
.addVariables(extraTestEnv, extraInheritedEnv),
456+
.withAdditionalVariables(extraTestEnv, extraInheritedEnv),
457457
executionSettings,
458458
shard,
459459
run,

0 commit comments

Comments
 (0)