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 a3baf58296a030..5841e381cd92a4 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 @@ -579,7 +579,7 @@ public SkylarkDict getSkylarkSubstitutions() { @Override public SkylarkDict getEnv() { - return SkylarkDict.copyOf(null, env.getFixedEnv()); + return SkylarkDict.copyOf(null, env.getFixedEnv().toMap()); } @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 f49ff020943ce8..2378a00650ea42 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 @@ -16,8 +16,11 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.collect.CollectionUtils; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.util.Fingerprint; +import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; import java.util.TreeMap; @@ -44,6 +47,80 @@ */ @AutoCodec public final class ActionEnvironment { + + /** A map of environment variables. */ + public interface EnvironmentVariables { + + /** + * Returns the environment variables as a map. + * + *

WARNING: this allocations additional objects if the underlying implementation is a {@link + * CompoundEnvironmentVariables}; use sparingly. + */ + ImmutableMap toMap(); + + default boolean isEmpty() { + return toMap().isEmpty(); + } + + default int size() { + return toMap().size(); + } + } + + /** + * An {@link EnvironmentVariables} that combines variables from two different environments without + * allocation a new map. + */ + static class CompoundEnvironmentVariables implements EnvironmentVariables { + private final EnvironmentVariables current; + private final EnvironmentVariables base; + + CompoundEnvironmentVariables(Map vars, EnvironmentVariables base) { + this.current = new SimpleEnvironmentVariables(vars); + this.base = base; + } + + @Override + public boolean isEmpty() { + return current.isEmpty() && base.isEmpty(); + } + + @Override + public ImmutableMap toMap() { + Map result = new LinkedHashMap<>(); + result.putAll(base.toMap()); + result.putAll(current.toMap()); + return ImmutableMap.copyOf(result); + } + } + + /** A simple {@link EnvironmentVariables}. */ + static class SimpleEnvironmentVariables implements EnvironmentVariables { + + static EnvironmentVariables create(Map vars) { + if (vars.isEmpty()) { + return EMPTY_ENVIRONMENT_VARIABLES; + } + return new SimpleEnvironmentVariables(vars); + } + + private final ImmutableMap vars; + + private SimpleEnvironmentVariables(Map vars) { + this.vars = ImmutableMap.copyOf(vars); + } + + @Override + public ImmutableMap toMap() { + return vars; + } + } + + /** An empty {@link EnvironmentVariables}. */ + public static final EnvironmentVariables EMPTY_ENVIRONMENT_VARIABLES = + new SimpleEnvironmentVariables(ImmutableMap.of()); + /** * An empty environment, mainly for testing. Production code should never use this, but instead * get the proper environment from the current configuration. @@ -51,7 +128,7 @@ public final class ActionEnvironment { // 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(ImmutableMap.of(), ImmutableSet.of()); + new ActionEnvironment(EMPTY_ENVIRONMENT_VARIABLES, ImmutableSet.of()); /** * Splits the given map into a map of variables with a fixed value, and a set of variables that @@ -71,14 +148,14 @@ public static ActionEnvironment split(Map env) { inheritedEnv.add(key); } } - return create(ImmutableMap.copyOf(fixedEnv), ImmutableSet.copyOf(inheritedEnv)); + return create(new SimpleEnvironmentVariables(fixedEnv), ImmutableSet.copyOf(inheritedEnv)); } - private final ImmutableMap fixedEnv; - private final ImmutableSet inheritedEnv; + private final EnvironmentVariables fixedEnv; + private final Iterable inheritedEnv; - private ActionEnvironment( - ImmutableMap fixedEnv, ImmutableSet inheritedEnv) { + private ActionEnvironment(EnvironmentVariables fixedEnv, Iterable inheritedEnv) { + CollectionUtils.checkImmutable(inheritedEnv); this.fixedEnv = fixedEnv; this.inheritedEnv = inheritedEnv; } @@ -90,20 +167,33 @@ private ActionEnvironment( */ @AutoCodec.Instantiator public static ActionEnvironment create( - ImmutableMap fixedEnv, ImmutableSet inheritedEnv) { - if (fixedEnv.isEmpty() && inheritedEnv.isEmpty()) { + EnvironmentVariables fixedEnv, Iterable inheritedEnv) { + if (fixedEnv.isEmpty() && Iterables.isEmpty(inheritedEnv)) { return EMPTY; } return new ActionEnvironment(fixedEnv, inheritedEnv); } + public static ActionEnvironment create( + Map fixedEnv, Iterable inheritedEnv) { + return new ActionEnvironment(SimpleEnvironmentVariables.create(fixedEnv), inheritedEnv); + } + public static ActionEnvironment create(Map fixedEnv) { - return new ActionEnvironment(ImmutableMap.copyOf(fixedEnv), ImmutableSet.of()); + return new ActionEnvironment(new SimpleEnvironmentVariables(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); } /** Returns the combined size of the fixed and inherited environments. */ public int size() { - return fixedEnv.size() + inheritedEnv.size(); + return fixedEnv.size() + Iterables.size(inheritedEnv); } /** @@ -111,7 +201,7 @@ 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 ImmutableMap getFixedEnv() { + public EnvironmentVariables getFixedEnv() { return fixedEnv; } @@ -121,7 +211,7 @@ public ImmutableMap getFixedEnv() { * be used for testing and to compute the cache keys of actions. Use {@link #resolve} instead to * get the complete environment. */ - public ImmutableSet getInheritedEnv() { + public Iterable getInheritedEnv() { return inheritedEnv; } @@ -133,7 +223,7 @@ public ImmutableSet getInheritedEnv() { */ public void resolve(Map result, Map clientEnv) { Preconditions.checkNotNull(clientEnv); - result.putAll(fixedEnv); + result.putAll(fixedEnv.toMap()); for (String var : inheritedEnv) { String value = clientEnv.get(var); if (value != null) { @@ -143,6 +233,7 @@ public void resolve(Map result, Map clientEnv) { } public void addTo(Fingerprint f) { - f.addStringMap(fixedEnv); + f.addStringMap(fixedEnv.toMap()); + f.addStrings(inheritedEnv); } } 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 c3c268c9161f4e..8c423a5a7e3d4a 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 @@ -225,7 +225,6 @@ public CommandLines getCommandLines() { } @Override - @VisibleForTesting public List getArguments() throws CommandLineExpansionException { return ImmutableList.copyOf(commandLines.allArguments()); } @@ -385,7 +384,7 @@ protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) for (Artifact runfilesManifest : runfilesManifests) { fp.addPath(runfilesManifest.getExecPath()); } - fp.addStringMap(env.getFixedEnv()); + env.addTo(fp); fp.addStrings(getClientEnvironmentVariables()); fp.addStringMap(getExecutionInfo()); } @@ -395,7 +394,7 @@ public String describeKey() { StringBuilder message = new StringBuilder(); message.append(getProgressMessage()); message.append('\n'); - for (Map.Entry entry : env.getFixedEnv().entrySet()) { + for (Map.Entry entry : env.getFixedEnv().toMap().entrySet()) { message.append(" Environment variable: "); message.append(ShellEscaper.escapeString(entry.getKey())); message.append('='); @@ -482,7 +481,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(); + return env.getFixedEnv().toMap(); } /** @@ -561,6 +560,7 @@ public static class Builder { private final List inputRunfilesSuppliers = new ArrayList<>(); private final List toolRunfilesSuppliers = new ArrayList<>(); private ResourceSet resourceSet = AbstractAction.DEFAULT_RESOURCE_SET; + private ActionEnvironment actionEnvironment = null; private ImmutableMap environment = ImmutableMap.of(); private ImmutableSet inheritedEnvironment = ImmutableSet.of(); private ImmutableMap executionInfo = ImmutableMap.of(); @@ -590,6 +590,7 @@ public Builder(Builder other) { this.inputRunfilesSuppliers.addAll(other.inputRunfilesSuppliers); this.toolRunfilesSuppliers.addAll(other.toolRunfilesSuppliers); this.resourceSet = other.resourceSet; + this.actionEnvironment = other.actionEnvironment; this.environment = other.environment; this.executionInfo = other.executionInfo; this.isShellCommand = other.isShellCommand; @@ -632,9 +633,11 @@ public Action[] build(ActionOwner owner, BuildConfiguration configuration) { } CommandLines commandLines = result.build(); ActionEnvironment env = - useDefaultShellEnvironment - ? configuration.getActionEnvironment() - : ActionEnvironment.create(environment, inheritedEnvironment); + actionEnvironment != null + ? actionEnvironment + : useDefaultShellEnvironment + ? configuration.getActionEnvironment() + : ActionEnvironment.create(environment, inheritedEnvironment); Action spawnAction = buildSpawnAction( owner, commandLines, configuration.getCommandLineLimits(), configuration, env); @@ -825,6 +828,12 @@ public Builder setResources(ResourceSet resourceSet) { return this; } + /** Sets the action environment. */ + public Builder setEnvironment(ActionEnvironment actionEnvironment) { + this.actionEnvironment = actionEnvironment; + return this; + } + /** * Sets the map of environment variables. Do not use! This makes the builder ignore the * 'default shell environment', which is computed from the --action_env command line option. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java index 986388be34a0a2..b9dcfae3b3b333 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java @@ -99,8 +99,7 @@ protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) { fp.addString(GUID); fp.addBoolean(filesetTree); fp.addBoolean(enableRunfiles); - fp.addStringMap(env.getFixedEnv()); - fp.addStrings(env.getInheritedEnv()); + env.addTo(fp); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java index 2437f69a075adc..37e190f252fc66 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java @@ -1558,7 +1558,7 @@ public ActionEnvironment getActionEnvironment() { */ @Override public ImmutableMap getLocalShellEnvironment() { - return actionEnv.getFixedEnv(); + return actionEnv.getFixedEnv().toMap(); } /** @@ -1576,7 +1576,7 @@ public ImmutableMap getLocalShellEnvironment() { * client environment.) */ @Deprecated // Use getActionEnvironment instead. - public ImmutableSet getVariableShellEnvironment() { + public Iterable getVariableShellEnvironment() { return actionEnv.getInheritedEnv(); } @@ -1711,7 +1711,7 @@ public boolean legacyExternalRunfiles() { */ @Override public ImmutableMap getTestEnv() { - return testEnv.getFixedEnv(); + return testEnv.getFixedEnv().toMap(); } /** diff --git a/src/main/java/com/google/devtools/build/lib/query2/ActionGraphTextOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/ActionGraphTextOutputFormatterCallback.java index cf2061189d2f0c..0a51c25fa81d18 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/ActionGraphTextOutputFormatterCallback.java +++ b/src/main/java/com/google/devtools/build/lib/query2/ActionGraphTextOutputFormatterCallback.java @@ -16,7 +16,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.common.collect.Streams; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionExecutionMetadata; @@ -174,13 +174,13 @@ private void writeAction(ActionAnalysisMetadata action, PrintStream printStream) SpawnAction spawnAction = (SpawnAction) action; // TODO(twerth): This handles the fixed environment. We probably want to output the inherited // environment as well. - ImmutableSet> fixedEnvironment = - spawnAction.getEnvironment().getFixedEnv().entrySet(); - if (!fixedEnvironment.isEmpty()) { + Iterable> fixedEnvironment = + spawnAction.getEnvironment().getFixedEnv().toMap().entrySet(); + if (!Iterables.isEmpty(fixedEnvironment)) { stringBuilder .append(" Environment: [") .append( - fixedEnvironment.stream() + Streams.stream(fixedEnvironment) .map( environmentVariable -> environmentVariable.getKey() + "=" + environmentVariable.getValue()) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 36f934cfed32f6..4ce2df3583a7a7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -1027,8 +1027,7 @@ public boolean isShareable() { @Override public void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) { fp.addUUID(actionClassId); - fp.addStringMap(env.getFixedEnv()); - fp.addStrings(env.getInheritedEnv()); + env.addTo(fp); fp.addStringMap(compileCommandLine.getEnvironment()); fp.addStringMap(executionInfo); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java index 8c5e0561c822be..8a592d1fca43da 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java @@ -203,8 +203,7 @@ protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) { } fp.addPath(imports.getExecPath()); } - fp.addStringMap(env.getFixedEnv()); - fp.addStrings(env.getInheritedEnv()); + env.addTo(fp); fp.addStringMap(getExecutionInfo()); } 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 f276b06f6f8b0f..a314f3908efa1d 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 @@ -282,7 +282,8 @@ public void build(RuleContext ruleContext, JavaSemantics javaSemantics) { builder.setProgressMessage(getProgressMessage()); builder.setMnemonic(MNEMONIC); builder.setResources(LOCAL_RESOURCES); - builder.setEnvironment(UTF8_ENVIRONMENT); + builder.setEnvironment( + ruleContext.getConfiguration().getActionEnvironment().addFixedVariables(UTF8_ENVIRONMENT)); builder.setExecutionInfo(executionInfo); builder.setExtraActionInfo( 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 d89836be1257be..bf29c19bde3ad4 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 @@ -16,6 +16,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import static com.google.devtools.build.lib.rules.java.JavaCompileActionBuilder.UTF8_ENVIRONMENT; import static java.nio.charset.StandardCharsets.ISO_8859_1; import static java.util.stream.Collectors.joining; @@ -230,7 +231,8 @@ public void build(JavaToolchainProvider javaToolchain, JavaRuntimeInfo hostJavab SpawnAction.Builder builder = new SpawnAction.Builder(); - builder.setEnvironment(JavaCompileActionBuilder.UTF8_ENVIRONMENT); + builder.setEnvironment( + ruleContext.getConfiguration().getActionEnvironment().addFixedVariables(UTF8_ENVIRONMENT)); builder.setProgressMessage( new ProgressMessage( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/ActionGraphDump.java b/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/ActionGraphDump.java index 3c966b258ecc9e..be5a34210f40a7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/ActionGraphDump.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/ActionGraphDump.java @@ -15,7 +15,6 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Action; @@ -111,7 +110,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. - ImmutableMap fixedEnvironment = spawnAction.getEnvironment().getFixedEnv(); + Map fixedEnvironment = spawnAction.getEnvironment().getFixedEnv().toMap(); for (Map.Entry environmentVariable : fixedEnvironment.entrySet()) { AnalysisProtos.KeyValuePair.Builder keyValuePairBuilder = AnalysisProtos.KeyValuePair.newBuilder(); 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 new file mode 100644 index 00000000000000..8ff8e445cc7e97 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionEnvironmentTest.java @@ -0,0 +1,43 @@ +// 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.actions; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** {@link ActionEnvironment}Test */ +@RunWith(JUnit4.class) +public final class ActionEnvironmentTest { + + @Test + public void compoundEnvOrdering() { + ActionEnvironment env1 = + 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")); + + assertThat(env1.getFixedEnv().toMap()).containsExactly("FOO", "foo1", "BAR", "bar"); + assertThat(env1.getInheritedEnv()).containsExactly("baz"); + + assertThat(env2.getFixedEnv().toMap()).containsExactly("FOO", "foo2", "BAR", "bar"); + assertThat(env2.getInheritedEnv()).containsExactly("baz"); + } +} 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 619384c66df527..c6300cee8cbed3 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.getActionEnvironment(options); - assertThat(env.getFixedEnv()).containsEntry("PATH", "/bin:/usr/bin"); - assertThat(env.getFixedEnv()).containsEntry("FOO", "bar"); + assertThat(env.getFixedEnv().toMap()).containsEntry("PATH", "/bin:/usr/bin"); + assertThat(env.getFixedEnv().toMap()).containsEntry("FOO", "bar"); } @Test