diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java index 408371b8dcf84f..f54952b196c7e3 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java +++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java @@ -699,7 +699,7 @@ public Dict getExecutionInfoDict() { @Override public Dict getEnv() { - return Dict.immutableCopyOf(env.getFixedEnv().toMap()); + return Dict.immutableCopyOf(env.getFixedEnv()); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java b/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java index f8ea2b1c993940..512cebf6c409a9 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.util.Fingerprint; import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.Map; import java.util.Set; import java.util.TreeMap; @@ -45,36 +46,57 @@ */ public final class ActionEnvironment { - /** A map of environment variables. */ + /** + * A map of environment variables and their values together with a list of variables whose values + * should be inherited from the client environment. + */ public interface EnvironmentVariables { /** - * Returns the environment variables as a map. + * Returns the fixed environment variables as a map. + * + *

WARNING: this allocates additional objects if the underlying implementation is a {@link + * CompoundEnvironmentVariables}; use sparingly. + */ + ImmutableMap getFixedEnvironment(); + + /** + * Returns the inherited environment variables as a set. * - *

WARNING: this allocations additional objects if the underlying implementation is a {@link + *

WARNING: this allocates additional objects if the underlying implementation is a {@link * CompoundEnvironmentVariables}; use sparingly. */ - ImmutableMap toMap(); + ImmutableSet getInheritedEnvironment(); default boolean isEmpty() { - return toMap().isEmpty(); + return getFixedEnvironment().isEmpty() && getInheritedEnvironment().isEmpty(); } default int size() { - return toMap().size(); + return getFixedEnvironment().size() + getInheritedEnvironment().size(); } } /** * An {@link EnvironmentVariables} that combines variables from two different environments without - * allocation a new map. + * allocating a new map. */ static class CompoundEnvironmentVariables implements EnvironmentVariables { + + static EnvironmentVariables create(Map fixedVars, Set inheritedVars, + EnvironmentVariables base) { + if (fixedVars.isEmpty() && inheritedVars.isEmpty()) { + return base; + } + return new CompoundEnvironmentVariables(fixedVars, inheritedVars, base); + } + private final EnvironmentVariables current; private final EnvironmentVariables base; - CompoundEnvironmentVariables(Map vars, EnvironmentVariables base) { - this.current = new SimpleEnvironmentVariables(vars); + private CompoundEnvironmentVariables(Map fixedVars, Set inheritedVars, + EnvironmentVariables base) { + this.current = SimpleEnvironmentVariables.create(fixedVars, inheritedVars); this.base = base; } @@ -84,39 +106,58 @@ public boolean isEmpty() { } @Override - public ImmutableMap toMap() { + public ImmutableMap getFixedEnvironment() { Map result = new LinkedHashMap<>(); - result.putAll(base.toMap()); - result.putAll(current.toMap()); + result.putAll(base.getFixedEnvironment()); + result.putAll(current.getFixedEnvironment()); return ImmutableMap.copyOf(result); } + + @Override + public ImmutableSet getInheritedEnvironment() { + Set result = new LinkedHashSet<>(); + result.addAll(base.getInheritedEnvironment()); + result.addAll(current.getInheritedEnvironment()); + return ImmutableSet.copyOf(result); + } } - /** A simple {@link EnvironmentVariables}. */ + /** + * A simple {@link EnvironmentVariables}. + */ static class SimpleEnvironmentVariables implements EnvironmentVariables { - static EnvironmentVariables create(Map vars) { - if (vars.isEmpty()) { + static EnvironmentVariables create(Map fixedVars, Set inheritedVars) { + if (fixedVars.isEmpty() && inheritedVars.isEmpty()) { return EMPTY_ENVIRONMENT_VARIABLES; } - return new SimpleEnvironmentVariables(vars); + return new SimpleEnvironmentVariables(fixedVars, inheritedVars); } - private final ImmutableMap vars; + private final ImmutableMap fixedVars; + private final ImmutableSet inheritedVars; + + private SimpleEnvironmentVariables(Map fixedVars, Set inheritedVars) { + this.fixedVars = ImmutableMap.copyOf(fixedVars); + this.inheritedVars = ImmutableSet.copyOf(inheritedVars); + } - private SimpleEnvironmentVariables(Map vars) { - this.vars = ImmutableMap.copyOf(vars); + @Override + public ImmutableMap getFixedEnvironment() { + return fixedVars; } @Override - public ImmutableMap toMap() { - return vars; + public ImmutableSet getInheritedEnvironment() { + return inheritedVars; } } - /** An empty {@link EnvironmentVariables}. */ + /** + * An empty {@link EnvironmentVariables}. + */ public static final EnvironmentVariables EMPTY_ENVIRONMENT_VARIABLES = - new SimpleEnvironmentVariables(ImmutableMap.of()); + new SimpleEnvironmentVariables(ImmutableMap.of(), ImmutableSet.of()); /** * An empty environment, mainly for testing. Production code should never use this, but instead @@ -124,8 +165,7 @@ public ImmutableMap toMap() { */ // TODO(ulfjack): Migrate all production code to use the proper action environment, and then make // this @VisibleForTesting or rename it to clarify. - public static final ActionEnvironment EMPTY = - new ActionEnvironment(EMPTY_ENVIRONMENT_VARIABLES, ImmutableSet.of()); + public static final ActionEnvironment EMPTY = new ActionEnvironment(EMPTY_ENVIRONMENT_VARIABLES); /** * Splits the given map into a map of variables with a fixed value, and a set of variables that @@ -133,8 +173,6 @@ public ImmutableMap toMap() { * given map. Returns these two parts as a new {@link ActionEnvironment} instance. */ public static ActionEnvironment split(Map env) { - // Care needs to be taken that the two sets don't overlap - the order in which the two parts are - // combined later is undefined. Map fixedEnv = new TreeMap<>(); Set inheritedEnv = new TreeSet<>(); for (Map.Entry entry : env.entrySet()) { @@ -145,50 +183,72 @@ public static ActionEnvironment split(Map env) { inheritedEnv.add(key); } } - return create(new SimpleEnvironmentVariables(fixedEnv), ImmutableSet.copyOf(inheritedEnv)); + return create(SimpleEnvironmentVariables.create(fixedEnv, inheritedEnv)); } - private final EnvironmentVariables fixedEnv; - private final ImmutableSet inheritedEnv; + private final EnvironmentVariables vars; - private ActionEnvironment(EnvironmentVariables fixedEnv, ImmutableSet inheritedEnv) { - this.fixedEnv = fixedEnv; - this.inheritedEnv = inheritedEnv; + private ActionEnvironment(EnvironmentVariables vars) { + this.vars = vars; } /** - * Creates a new action environment. The order in which the environments are combined is - * undefined, so callers need to take care that the key set of the {@code fixedEnv} map and the - * set of {@code inheritedEnv} elements are disjoint. + * Creates a new action environment. If an environment variable is contained in both {@link + * EnvironmentVariables#getFixedEnvironment()} and {@link EnvironmentVariables#getInheritedEnvironment()}, + * the result of {@link ActionEnvironment#resolve(Map, Map)} will contain the value inherited from + * the client environment. */ - private static ActionEnvironment create( - EnvironmentVariables fixedEnv, ImmutableSet inheritedEnv) { - if (fixedEnv.isEmpty() && inheritedEnv.isEmpty()) { + private static ActionEnvironment create(EnvironmentVariables vars) { + if (vars.isEmpty()) { return EMPTY; } - return new ActionEnvironment(fixedEnv, inheritedEnv); + return new ActionEnvironment(vars); } + /** + * Creates a new action environment. If an environment variable is contained both as a key in + * {@code fixedEnv} and in {@code inheritedEnv}, the result of {@link + * ActionEnvironment#resolve(Map, Map)} will contain the value inherited from the client + * environment. + */ public static ActionEnvironment create( Map fixedEnv, ImmutableSet inheritedEnv) { - return new ActionEnvironment(SimpleEnvironmentVariables.create(fixedEnv), inheritedEnv); + return ActionEnvironment.create(SimpleEnvironmentVariables.create(fixedEnv, inheritedEnv)); } public static ActionEnvironment create(Map fixedEnv) { - return new ActionEnvironment(new SimpleEnvironmentVariables(fixedEnv), ImmutableSet.of()); + return ActionEnvironment.create(SimpleEnvironmentVariables.create(fixedEnv, ImmutableSet.of())); } /** * Returns a copy of the environment with the given fixed variables added to it, overwriting * any existing occurrences of those variables. */ - public ActionEnvironment addFixedVariables(Map vars) { - return new ActionEnvironment(new CompoundEnvironmentVariables(vars, fixedEnv), inheritedEnv); + public ActionEnvironment withAdditionalFixedVariables(Map fixedVars) { + return withAdditionalVariables(fixedVars, ImmutableSet.of()); } - /** Returns the combined size of the fixed and inherited environments. */ + /** + * Returns a copy of the environment with the given fixed and inherited variables added to it, + * overwriting any existing occurrences of those variables. + */ + public ActionEnvironment withAdditionalVariables(Map fixedVars, + Set inheritedVars) { + EnvironmentVariables newVars = CompoundEnvironmentVariables.create(fixedVars, inheritedVars, + vars); + if (newVars == vars) { + return this; + } + return ActionEnvironment.create(newVars); + } + + /** + * Returns an upper bound on the combined size of the fixed and inherited environments. A call to + * {@link ActionEnvironment#resolve(Map, Map)} may add less entries than this number if + * environment variables are contained in both the fixed and the inherited environment. + */ public int size() { - return fixedEnv.size() + inheritedEnv.size(); + return vars.size(); } /** @@ -196,8 +256,8 @@ public int size() { * fixed values and their values. This should only be used for testing and to compute the cache * keys of actions. Use {@link #resolve} instead to get the complete environment. */ - public EnvironmentVariables getFixedEnv() { - return fixedEnv; + public ImmutableMap getFixedEnv() { + return vars.getFixedEnvironment(); } /** @@ -207,7 +267,7 @@ public EnvironmentVariables getFixedEnv() { * get the complete environment. */ public ImmutableSet getInheritedEnv() { - return inheritedEnv; + return vars.getInheritedEnvironment(); } /** @@ -218,8 +278,8 @@ public ImmutableSet getInheritedEnv() { */ public void resolve(Map result, Map clientEnv) { checkNotNull(clientEnv); - result.putAll(fixedEnv.toMap()); - for (String var : inheritedEnv) { + result.putAll(vars.getFixedEnvironment()); + for (String var : vars.getInheritedEnvironment()) { String value = clientEnv.get(var); if (value != null) { result.put(var, value); @@ -228,7 +288,7 @@ public void resolve(Map result, Map clientEnv) { } public void addTo(Fingerprint f) { - f.addStringMap(fixedEnv.toMap()); - f.addStrings(inheritedEnv); + f.addStringMap(vars.getFixedEnvironment()); + f.addStrings(vars.getInheritedEnvironment()); } } 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 97258bf87fbe74..88eabf31f8a885 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 @@ -475,6 +475,7 @@ private TestProvider initializeTestProvider(FilesToRunProvider filesToRunProvide providersBuilder.getProvider(TestEnvironmentInfo.PROVIDER.getKey()); if (environmentProvider != null) { testActionBuilder.addExtraEnv(environmentProvider.getEnvironment()); + testActionBuilder.addExtraInheritedEnv(environmentProvider.getInheritedEnvironment()); } TestParams testParams = diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index 00690adcad19a6..2c7b93febc290a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java @@ -465,7 +465,7 @@ public String describeKey() { StringBuilder message = new StringBuilder(); message.append(getProgressMessage()); message.append('\n'); - for (Map.Entry entry : env.getFixedEnv().toMap().entrySet()) { + for (Map.Entry entry : env.getFixedEnv().entrySet()) { message.append(" Environment variable: "); message.append(ShellEscaper.escapeString(entry.getKey())); message.append('='); @@ -574,7 +574,7 @@ public final ImmutableMap getIncompleteEnvironmentForTesting() { // ActionEnvironment to avoid developers misunderstanding the purpose of this method. That // requires first updating all subclasses and callers to actually handle environments correctly, // so it's not a small change. - return env.getFixedEnv().toMap(); + return env.getFixedEnv(); } /** Returns the out-of-band execution data for this action. */ diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java index 1495aeed74b857..bcaff069ad8a9e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java @@ -485,7 +485,7 @@ public boolean isSiblingRepositoryLayout() { */ @Override public ImmutableMap getLocalShellEnvironment() { - return actionEnv.getFixedEnv().toMap(); + return actionEnv.getFixedEnv(); } /** @@ -629,7 +629,7 @@ public boolean legacyExternalRunfiles() { */ @Override public ImmutableMap getTestEnv() { - return testEnv.getFixedEnv().toMap(); + return testEnv.getFixedEnv(); } /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java index e559b9d755a80d..c2c43e3d62cc08 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java @@ -54,7 +54,9 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.TreeMap; +import java.util.TreeSet; import javax.annotation.Nullable; /** @@ -94,10 +96,12 @@ public NestedSet getPackageSpecifications() { private InstrumentedFilesInfo instrumentedFiles; private int explicitShardCount; private final Map extraEnv; + private final Set extraInheritedEnv; public TestActionBuilder(RuleContext ruleContext) { this.ruleContext = ruleContext; this.extraEnv = new TreeMap<>(); + this.extraInheritedEnv = new TreeSet<>(); this.additionalTools = new ImmutableList.Builder<>(); } @@ -147,6 +151,11 @@ public TestActionBuilder addExtraEnv(Map extraEnv) { return this; } + public TestActionBuilder addExtraInheritedEnv(List extraInheritedEnv) { + this.extraInheritedEnv.addAll(extraInheritedEnv); + return this; + } + /** * Set the explicit shard count. Note that this may be overridden by the sharding strategy. */ @@ -402,7 +411,7 @@ private TestParams createTestAction(int shards) coverageArtifact, coverageDirectory, testProperties, - runfilesSupport.getActionEnvironment().addFixedVariables(extraTestEnv), + runfilesSupport.getActionEnvironment().withAdditionalVariables(extraTestEnv, extraInheritedEnv), executionSettings, shard, run, diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestEnvironmentInfo.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestEnvironmentInfo.java index f7e41009fa3d39..160582f6e175f4 100644 --- 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 @@ -15,10 +15,13 @@ 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. */ @@ -29,11 +32,14 @@ public final class TestEnvironmentInfo extends NativeInfo implements TestEnviron public static final BuiltinProvider PROVIDER = new BuiltinProvider("TestEnvironment", TestEnvironmentInfo.class) {}; - private final Map environment; + private final ImmutableMap environment; + private final ImmutableList inheritedEnvironment; - /** Constructs a new provider with the given variable name to variable value mapping. */ - public TestEnvironmentInfo(Map environment) { - this.environment = Preconditions.checkNotNull(environment); + /** 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 @@ -48,4 +54,12 @@ public BuiltinProvider getProvider() { public Map getEnvironment() { return environment; } + + /** + * Returns names of environment variables whose value should be obtained from the environment. + */ + @Override + public List getInheritedEnvironment() { + return inheritedEnvironment; + } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index 2aabd2af5ee209..4edb8b1511e42e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java @@ -1045,7 +1045,7 @@ public List getArguments() throws CommandLineExpansionException, Interru @Override public ImmutableMap getIncompleteEnvironmentForTesting() throws ActionExecutionException { - return getEnvironment().getFixedEnv().toMap(); + return getEnvironment().getFixedEnv(); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java index 91ec8a19144911..201ec27f0d8425 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java +++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java @@ -221,7 +221,7 @@ private void writeAction(ActionAnalysisMetadata action, PrintStream printStream) // TODO(twerth): This handles the fixed environment. We probably want to output the inherited // environment as well. Iterable> fixedEnvironment = - abstractAction.getEnvironment().getFixedEnv().toMap().entrySet(); + abstractAction.getEnvironment().getFixedEnv().entrySet(); if (!Iterables.isEmpty(fixedEnvironment)) { stringBuilder .append(" Environment: [") diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java index 16e289b0b90b70..eea2bea665c89d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java @@ -614,7 +614,7 @@ public final ImmutableMap getIncompleteEnvironmentForTesting() { // ActionEnvironment to avoid developers misunderstanding the purpose of this method. That // requires first updating all subclasses and callers to actually handle environments correctly, // so it's not a small change. - return env.getFixedEnv().toMap(); + return env.getFixedEnv(); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java index 214cc1060c7b8e..4532a6d1028654 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileActionBuilder.java @@ -205,7 +205,7 @@ public JavaCompileAction build() { toolsBuilder.addTransitive(toolsJars); ActionEnvironment actionEnvironment = - ruleContext.getConfiguration().getActionEnvironment().addFixedVariables(UTF8_ENVIRONMENT); + ruleContext.getConfiguration().getActionEnvironment().withAdditionalFixedVariables(UTF8_ENVIRONMENT); NestedSetBuilder mandatoryInputsBuilder = NestedSetBuilder.stableOrder(); mandatoryInputsBuilder diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java index 0838f8667e1c75..a0d5aa8fb2e8cb 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileActionBuilder.java @@ -296,7 +296,7 @@ public void build(JavaToolchainProvider javaToolchain) throws InterruptedExcepti } ActionEnvironment actionEnvironment = - ruleContext.getConfiguration().getActionEnvironment().addFixedVariables(UTF8_ENVIRONMENT); + ruleContext.getConfiguration().getActionEnvironment().withAdditionalFixedVariables(UTF8_ENVIRONMENT); OnDemandString progressMessage = new ProgressMessage( diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java b/src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java index 08f938caf3268b..e661c4015f87b2 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 @@ -18,6 +18,8 @@ import com.google.devtools.build.lib.starlarkbuildapi.test.TestingModuleApi; import net.starlark.java.eval.Dict; import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.Sequence; +import net.starlark.java.eval.StarlarkList; /** A class that exposes testing infrastructure to Starlark. */ public class StarlarkTestingModule implements TestingModuleApi { @@ -29,9 +31,12 @@ public ExecutionInfo executionInfo(Dict requirements /* * } @Override - public TestEnvironmentInfo testEnvironment(Dict environment /* */) + public TestEnvironmentInfo testEnvironment(Dict environment /* */, + Sequence inheritedEnvironment /* */) throws EvalException { return new TestEnvironmentInfo( - Dict.cast(environment, String.class, String.class, "environment")); + Dict.cast(environment, String.class, String.class, "environment"), + StarlarkList.immutableCopyOf( + Sequence.cast(inheritedEnvironment, String.class, "inherited_environment"))); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java b/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java index 5b326c56a9ddf8..5ff47004347ebc 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2/ActionGraphDump.java @@ -161,7 +161,7 @@ private void dumpSingleAction(ConfiguredTarget configuredTarget, ActionAnalysisM SpawnAction spawnAction = (SpawnAction) action; // TODO(twerth): This handles the fixed environment. We probably want to output the inherited // environment as well. - Map fixedEnvironment = spawnAction.getEnvironment().getFixedEnv().toMap(); + Map fixedEnvironment = spawnAction.getEnvironment().getFixedEnv(); for (Map.Entry environmentVariable : fixedEnvironment.entrySet()) { actionBuilder.addEnvironmentVariables( AnalysisProtosV2.KeyValuePair.newBuilder() 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 c8430970530d8a..ed11b24af45ddf 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 @@ -15,6 +15,7 @@ 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; @@ -28,4 +29,10 @@ public interface TestEnvironmentInfoApi extends StructApi { 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 a6f243a6274be1..cd9e2d65ba3b87 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 @@ -15,10 +15,12 @@ package com.google.devtools.build.lib.starlarkbuildapi.test; 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; import net.starlark.java.eval.StarlarkValue; /** Helper module for accessing test infrastructure. */ @@ -56,12 +58,24 @@ ExecutionInfoApi executionInfo(Dict requirements // expec parameters = { @Param( name = "environment", - named = false, + named = true, positional = true, doc = "A map of string keys and values that represent environment variables and their" - + " values. These will be made available during the test execution.") + + " values. These will be made available during the test execution."), + @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" + + " during the test execution with their current value taken from the shell" + + " environment. If a variable is contained in both environment" + + " and inherited_environment, the value inherited from the" + + " shell environment will take precedence if set.") }) - TestEnvironmentInfoApi testEnvironment(Dict environment // expected - ) throws EvalException; + TestEnvironmentInfoApi testEnvironment(Dict environment, // expected + Sequence inheritedEnvironment /* expected */) throws EvalException; } diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java index 8ff8e445cc7e97..cb770773ec85fc 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java @@ -18,6 +18,8 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import java.util.HashMap; +import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -32,12 +34,41 @@ public void compoundEnvOrdering() { ActionEnvironment.create( ImmutableMap.of("FOO", "foo1", "BAR", "bar"), ImmutableSet.of("baz")); // entries added by env2 override the existing entries - ActionEnvironment env2 = env1.addFixedVariables(ImmutableMap.of("FOO", "foo2")); + ActionEnvironment env2 = env1.withAdditionalFixedVariables(ImmutableMap.of("FOO", "foo2")); - assertThat(env1.getFixedEnv().toMap()).containsExactly("FOO", "foo1", "BAR", "bar"); + assertThat(env1.getFixedEnv()).containsExactly("FOO", "foo1", "BAR", "bar"); assertThat(env1.getInheritedEnv()).containsExactly("baz"); - assertThat(env2.getFixedEnv().toMap()).containsExactly("FOO", "foo2", "BAR", "bar"); + assertThat(env2.getFixedEnv()).containsExactly("FOO", "foo2", "BAR", "bar"); assertThat(env2.getInheritedEnv()).containsExactly("baz"); } + + @Test + public void emptyEnvironmentInterning() { + ActionEnvironment emptyEnvironment = ActionEnvironment.create(ImmutableMap.of(), + ImmutableSet.of()); + assertThat(emptyEnvironment).isSameInstanceAs(ActionEnvironment.EMPTY); + + ActionEnvironment base = ActionEnvironment.create(ImmutableMap.of("FOO", "foo1"), + ImmutableSet.of("baz")); + assertThat(base.withAdditionalFixedVariables(ImmutableMap.of())).isSameInstanceAs(base); + assertThat(base.withAdditionalVariables(ImmutableMap.of(), ImmutableSet.of())).isSameInstanceAs( + base); + } + + @Test + public void fixedInheritedInteraction() { + ActionEnvironment env = ActionEnvironment.create( + ImmutableMap.of("FIXED_ONLY", "fixed"), + ImmutableSet.of("INHERITED_ONLY")) + .withAdditionalVariables(ImmutableMap.of("FIXED_AND_INHERITED", "fixed"), + ImmutableSet.of("FIXED_AND_INHERITED")); + Map clientEnv = ImmutableMap.of("INHERITED_ONLY", "inherited", + "FIXED_AND_INHERITED", "inherited"); + Map result = new HashMap<>(); + env.resolve(result, clientEnv); + + assertThat(result).containsExactly("FIXED_ONLY", "fixed", "FIXED_AND_INHERITED", "inherited", + "INHERITED_ONLY", "inherited"); + } } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProviderTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProviderTest.java index 769b5ae740e77b..18e1179a0b78e9 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProviderTest.java @@ -176,8 +176,8 @@ public void strictActionEnv() throws Exception { "--action_env=FOO=bar"); ActionEnvironment env = BazelRuleClassProvider.SHELL_ACTION_ENV.apply(options); - assertThat(env.getFixedEnv().toMap()).containsEntry("PATH", "/bin:/usr/bin:/usr/local/bin"); - assertThat(env.getFixedEnv().toMap()).containsEntry("FOO", "bar"); + assertThat(env.getFixedEnv()).containsEntry("PATH", "/bin:/usr/bin:/usr/local/bin"); + assertThat(env.getFixedEnv()).containsEntry("FOO", "bar"); } @Test 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 051c4ad816f98d..bc429c7438c765 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 @@ -79,6 +79,39 @@ public void testStarlarkRulePropagatesTestEnvironmentProvider() throws Exception assertThat(provider.getEnvironment().get("XCODE_VERSION_OVERRIDE")).isEqualTo("7.3.1"); } + @Test + public void testStarlarkRulePropagatesTestEnvironmentProviderWithInheritedEnv() throws Exception { + scratch.file("examples/rule/BUILD"); + scratch.file( + "examples/rule/apple_rules.bzl", + "def my_rule_impl(ctx):", + " test_env = testing.TestEnvironment(", + " {'XCODE_VERSION_OVERRIDE': '7.3.1'},", + " [", + " 'DEVELOPER_DIR',", + " 'XCODE_VERSION_OVERRIDE',", + " ]", + ")", + " return [test_env]", + "my_rule = rule(implementation = my_rule_impl,", + " attrs = {},", + ")"); + scratch.file( + "examples/apple_starlark/BUILD", + "package(default_visibility = ['//visibility:public'])", + "load('//examples/rule:apple_rules.bzl', 'my_rule')", + "my_rule(", + " name = 'my_target',", + ")"); + + ConfiguredTarget starlarkTarget = getConfiguredTarget("//examples/apple_starlark:my_target"); + TestEnvironmentInfo provider = starlarkTarget.get(TestEnvironmentInfo.PROVIDER); + + assertThat(provider.getEnvironment().get("XCODE_VERSION_OVERRIDE")).isEqualTo("7.3.1"); + assertThat(provider.getInheritedEnvironment()).contains("DEVELOPER_DIR"); + assertThat(provider.getInheritedEnvironment()).contains("XCODE_VERSION_OVERRIDE"); + } + @Test public void testExecutionInfoProviderCanMarkTestAsLocal() throws Exception { scratch.file("examples/rule/BUILD"); diff --git a/src/test/shell/bazel/bazel_rules_test.sh b/src/test/shell/bazel/bazel_rules_test.sh index ef70918a41992d..5650f0d458e283 100755 --- a/src/test/shell/bazel/bazel_rules_test.sh +++ b/src/test/shell/bazel/bazel_rules_test.sh @@ -593,4 +593,75 @@ EOF assert_contains "hello world" bazel-bin/pkg/hello_gen.txt } +function test_starlark_test_with_test_environment() { + mkdir pkg + cat >pkg/BUILD <<'EOF' +load(":rules.bzl", "my_test") +my_test( + name = "my_test", +) +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 +""" +EOF + else + cat >pkg/rules.bzl <<'EOF' +_SCRIPT_EXT = ".sh" +_SCRIPT_CONTENT = """#!/bin/bash +[[ "$FIXED_ONLY" == "fixed" \ + && "$FIXED_AND_INHERITED" == "inherited" \ + && "$FIXED_AND_INHERITED_BUT_NOT_SET" == "fixed" \ + && "$INHERITED_ONLY" == "inherited" ]] +""" +EOF + fi + + cat >>pkg/rules.bzl <<'EOF' +def _my_test_impl(ctx): + test_sh = ctx.actions.declare_file(ctx.attr.name + _SCRIPT_EXT) + ctx.actions.write( + output = test_sh, + content = _SCRIPT_CONTENT, + is_executable = True, + ) + test_env = testing.TestEnvironment( + { + "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", + ] + ) + return [ + DefaultInfo( + executable = test_sh, + ), + test_env + ] + +my_test = rule( + implementation = _my_test_impl, + attrs = {}, + test = True, +) +EOF + + FIXED_AND_INHERITED=inherited INHERITED_ONLY=inherited \ + bazel test //pkg:my_test &> /dev/null || fail "Test should pass" +} + run_suite "rules test"