Specify fixedEnv/inheritedEnv interaction in ActionEnvironment

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<String> getInheritedEnvironment() {
    * given map. Returns these two parts as a new {@link ActionEnvironment} instance.
    */
   public static ActionEnvironment split(Map<String, String> 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<String, String> fixedEnv = new TreeMap<>();
     Set<String> inheritedEnv = new TreeSet<>();
     for (Map.Entry<String, String> 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<String, String> fixedEnv, ImmutableSet<String> inheritedEnv) {
     return ActionEnvironment.create(SimpleEnvironmentVariables.create(fixedEnv, inheritedEnv));
@@ -227,7 +232,11 @@ public ActionEnvironment addVariables(Map<String, String> fixedVars, Set<String>
         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();
   }

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<String, String> clientEnv = ImmutableMap.of("INHERITED_ONLY", "inherited",
+        "FIXED_AND_INHERITED", "inherited");
+    Map<String, String> result = new HashMap<>();
+    env.resolve(result, clientEnv);
+
+    assertThat(result).containsExactly("FIXED_ONLY", "fixed", "FIXED_AND_INHERITED", "inherited",
+        "INHERITED_ONLY", "inherited");
+  }
 }