Make SpawnActions honor the client environment

...for the variables that supposed to be inherited from it. Note
That with this patch, we take the correct variables, but do not
yet track the dependency on changes to the client environment; this
will happen in a follow up patches.

Also add a test that demonstrates that the client environment rather
than that at startup is taken.

--
Change-Id: I4d33efa8eaf4f8b689c9b7f2130f71309f3343f0
Reviewed-on: https://bazel-review.googlesource.com/#/c/5392
MOS_MIGRATED_REVID=131406356

4 files changed, 52 insertions, 7 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
index 3b02e119b6..09cf17600e 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
@@ -112,6 +112,10 @@ public class ActionExecutionContext {
     return executor;
   }
 
+  public ImmutableMap<String, String> getClientEnv() {
+    return clientEnv;
+  }
+
   public ArtifactExpander getArtifactExpander() {
     return artifactExpander;
   }
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 684abcdcc7..2a797237ff 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
@@ -244,7 +244,8 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
    */
   protected void internalExecute(
       ActionExecutionContext actionExecutionContext) throws ExecException, InterruptedException {
-    getContext(actionExecutionContext.getExecutor()).exec(getSpawn(), actionExecutionContext);
+    getContext(actionExecutionContext.getExecutor())
+        .exec(getSpawn(actionExecutionContext.getClientEnv()), actionExecutionContext);
   }
 
   @Override
@@ -287,9 +288,21 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
   /**
    * Returns a Spawn that is representative of the command that this Action
    * will execute. This function must not modify any state.
+   *
+   * This method is final, as it is merely a shorthand use of the generic way to obtain a spawn,
+   * which also depends on the client environment. Subclasses that which to override the way to get
+   * a spawn should override {@link getSpawn(Map<String, String>)} instead.
+   */
+  public final Spawn getSpawn() {
+    return getSpawn(null);
+  }
+
+  /**
+   * Return a spawn that is representative of the command that this Action will execute in the given
+   * client environment.
    */
-  public Spawn getSpawn() {
-    return new ActionSpawn();
+  public Spawn getSpawn(Map<String, String> clientEnv) {
+    return new ActionSpawn(clientEnv);
   }
 
   @Override
@@ -403,7 +416,9 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
 
     private final List<Artifact> filesets = new ArrayList<>();
 
-    public ActionSpawn() {
+    private final ImmutableMap<String, String> effectiveEnvironment;
+
+    public ActionSpawn(Map<String, String> clientEnv) {
       super(ImmutableList.copyOf(argv.arguments()),
           ImmutableMap.<String, String>of(),
           executionInfo,
@@ -415,11 +430,27 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
           filesets.add(input);
         }
       }
+      LinkedHashMap<String, String> env = new LinkedHashMap<>(SpawnAction.this.getEnvironment());
+      if (clientEnv != null) {
+        for (String var : SpawnAction.this.getClientEnvironmentVariables()) {
+          String value = clientEnv.get(var);
+          if (value == null) {
+            env.remove(var);
+          } else {
+            env.put(var, value);
+          }
+        }
+      }
+      effectiveEnvironment = ImmutableMap.copyOf(env);
+    }
+
+    public ActionSpawn() {
+      this(null);
     }
 
     @Override
     public ImmutableMap<String, String> getEnvironment() {
-      return ImmutableMap.copyOf(SpawnAction.this.getEnvironment());
+      return effectiveEnvironment;
     }
 
     @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java
index 937e1ae3cf..d80c5812e4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java
@@ -218,8 +218,8 @@ public final class ExtraAction extends SpawnAction {
    */
   // TODO(bazel-team): Add more tests that execute this code path!
   @Override
-  public Spawn getSpawn() {
-    final Spawn base = super.getSpawn();
+  public Spawn getSpawn(Map<String, String> clientEnv) {
+    final Spawn base = super.getSpawn(clientEnv);
     return new DelegateSpawn(base) {
       @Override public ImmutableMap<PathFragment, Artifact> getRunfilesManifests() {
         ImmutableMap.Builder<PathFragment, Artifact> builder = ImmutableMap.builder();
diff --git a/src/test/shell/integration/action_env_test.sh b/src/test/shell/integration/action_env_test.sh
index 2074c83d27..24e87ee48e 100755
--- a/src/test/shell/integration/action_env_test.sh
+++ b/src/test/shell/integration/action_env_test.sh
@@ -62,4 +62,14 @@ function test_simple_latest_wins() {
   expect_log "BAR=bar"
 }
 
+function test_client_env() {
+  export FOO=startup_foo
+  bazel clean --expunge
+  bazel help build > /dev/null || fail "bazel help failed"
+  export FOO=client_foo
+  bazel build --action_env=FOO pkg:showenv || fail "bazel build showenv failed"
+  cat `bazel info bazel-genfiles`/pkg/env.txt > $TEST_log
+  expect_log "FOO=client_foo"
+}
+
 run_suite "Tests for bazel's handling of environment variables in actions"