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 d657c32f143cbc..f7bad5cf5529d0 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 @@ -45,8 +45,8 @@ public final class ActionEnvironment { /** - * A map of environment variables together with a list of variables to inherit from the shell - * environment. + * 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 { @@ -77,7 +77,7 @@ default int 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 { @@ -167,8 +167,6 @@ public ImmutableSet getInheritedEnvironment() { * 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()) { @@ -189,9 +187,10 @@ private ActionEnvironment(EnvironmentVariables 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 vars) { if (vars.isEmpty()) { @@ -200,6 +199,12 @@ private static ActionEnvironment create(EnvironmentVariables vars) { 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 ActionEnvironment.create(SimpleEnvironmentVariables.create(fixedEnv, inheritedEnv)); @@ -227,7 +232,11 @@ public ActionEnvironment addVariables(Map fixedVars, Set CompoundEnvironmentVariables.create(fixedVars, inheritedVars, vars)); } - /** Returns the combined size of the fixed and inherited environments. */ + /** + * 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 vars.size(); } 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 d48d266db2f373..5a49df579f8c37 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; @@ -40,4 +42,20 @@ public void compoundEnvOrdering() { assertThat(env2.getFixedEnv()).containsExactly("FOO", "foo2", "BAR", "bar"); assertThat(env2.getInheritedEnv()).containsExactly("baz"); } + + @Test + public void fixedInheritedInteraction() { + ActionEnvironment env = ActionEnvironment.create( + ImmutableMap.of("FIXED_ONLY", "fixed"), + ImmutableSet.of("INHERITED_ONLY")) + .addVariables(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"); + } }