Fix Cpp{Compile,Link}Action environment and cache key computation

These were previously ignoring the inhertied environment, i.e.,
--action_env=PATH did _not_ result in the PATH variable being forwarded from
the client environment.

Fixes bazelbuild#5142.

PiperOrigin-RevId: 196966822

Author: ulfjack

src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java

@@ -106,10 +106,31 @@ public abstract class AbstractAction implements Action, ActionApi {
   /**
    * Construct an abstract action with the specified inputs and outputs;
    */
-  protected AbstractAction(ActionOwner owner,
-                           Iterable<Artifact> inputs,
-                           Iterable<Artifact> outputs) {
-    this(owner, ImmutableList.<Artifact>of(), inputs, EmptyRunfilesSupplier.INSTANCE, outputs);
+  protected AbstractAction(
+      ActionOwner owner,
+      Iterable<Artifact> inputs,
+      Iterable<Artifact> outputs) {
+    this(
+        owner,
+        /*tools = */ImmutableList.of(),
+        inputs,
+        EmptyRunfilesSupplier.INSTANCE,
+        outputs,
+        ActionEnvironment.EMPTY);
+  }
+
+  protected AbstractAction(
+      ActionOwner owner,
+      Iterable<Artifact> inputs,
+      Iterable<Artifact> outputs,
+      ActionEnvironment env) {
+    this(
+        owner,
+        /*tools = */ImmutableList.of(),
+        inputs,
+        EmptyRunfilesSupplier.INSTANCE,
+        outputs,
+        env);
   }
 
   /**
@@ -120,15 +141,21 @@ protected AbstractAction(
       Iterable<Artifact> tools,
       Iterable<Artifact> inputs,
       Iterable<Artifact> outputs) {
-    this(owner, tools, inputs, EmptyRunfilesSupplier.INSTANCE, outputs);
+    this(owner, tools, inputs, EmptyRunfilesSupplier.INSTANCE, outputs, ActionEnvironment.EMPTY);
   }
 
   protected AbstractAction(
       ActionOwner owner,
       Iterable<Artifact> inputs,
       RunfilesSupplier runfilesSupplier,
       Iterable<Artifact> outputs) {
-    this(owner, ImmutableList.<Artifact>of(), inputs, runfilesSupplier, outputs);
+    this(
+        owner,
+        /*tools=*/ImmutableList.of(),
+        inputs,
+        runfilesSupplier,
+        outputs,
+        ActionEnvironment.EMPTY);
   }
 
   protected AbstractAction(
@@ -148,14 +175,12 @@ protected AbstractAction(
       Iterable<Artifact> outputs,
       ActionEnvironment env) {
     Preconditions.checkNotNull(owner);
-    // TODO(bazel-team): Use RuleContext.actionOwner here instead
     this.owner = owner;
     this.tools = CollectionUtils.makeImmutable(tools);
     this.inputs = CollectionUtils.makeImmutable(inputs);
-    this.env = env;
+    this.env = Preconditions.checkNotNull(env);
     this.outputs = ImmutableSet.copyOf(outputs);
-    this.runfilesSupplier = Preconditions.checkNotNull(runfilesSupplier,
-        "runfilesSupplier may not be null");
+    this.runfilesSupplier = Preconditions.checkNotNull(runfilesSupplier);
     Preconditions.checkArgument(!this.outputs.isEmpty(), "action outputs may not be empty");
   }
 

src/main/java/com/google/devtools/build/lib/actions/CommandAction.java

@@ -29,8 +29,10 @@ public interface CommandAction extends Action, ExecutionInfoSpecifier {
 
   /** 
    * Returns a map of command line variables to their values that constitute the environment
-   * in which this action should be run.
+   * in which this action should be run. This excludes any inherited environment variables, as this
+   * method does not provide access to the client environment.
    */
+  @VisibleForTesting
   ImmutableMap<String, String> getEnvironment();
 
   /** Returns inputs to this action, including inputs that may be pruned. */

src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java

@@ -478,6 +478,7 @@ protected SpawnInfo getExtraActionSpawnInfo() throws CommandLineExpansionExcepti
   }
 
   @Override
+  @VisibleForTesting
   public final ImmutableMap<String, String> getEnvironment() {
     // TODO(ulfjack): AbstractAction should declare getEnvironment with a return value of type
     // ActionEnvironment to avoid developers misunderstanding the purpose of this method. That

src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java

@@ -24,6 +24,7 @@
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.AbstractAction;
+import com.google.devtools.build.lib.actions.ActionEnvironment;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
@@ -155,7 +156,6 @@ public class CppCompileAction extends AbstractAction
    */
   public static final String CLIF_MATCH = "clif-match";
 
-  private final ImmutableMap<String, String> localShellEnvironment;
   protected final Artifact outputFile;
   private final Artifact sourceFile;
   private final Artifact optionalSourceFile;
@@ -278,7 +278,7 @@ public class CppCompileAction extends AbstractAction
       @Nullable Artifact dwoFile,
       @Nullable Artifact ltoIndexingFile,
       Artifact optionalSourceFile,
-      ImmutableMap<String, String> localShellEnvironment,
+      ActionEnvironment env,
       CcCompilationContext ccCompilationContext,
       CoptsFilter coptsFilter,
       Iterable<IncludeScannable> lipoScannables,
@@ -298,7 +298,7 @@ public class CppCompileAction extends AbstractAction
             gcnoFile,
             dwoFile,
             ltoIndexingFile),
-        localShellEnvironment,
+        env,
         Preconditions.checkNotNull(outputFile),
         sourceFile,
         optionalSourceFile,
@@ -346,7 +346,7 @@ public class CppCompileAction extends AbstractAction
       ActionOwner owner,
       NestedSet<Artifact> inputs,
       ImmutableSet<Artifact> outputs,
-      ImmutableMap<String, String> localShellEnvironment,
+      ActionEnvironment env,
       Artifact outputFile,
       Artifact sourceFile,
       Artifact optionalSourceFile,
@@ -377,8 +377,7 @@ public class CppCompileAction extends AbstractAction
       boolean needsIncludeValidation,
       IncludeProcessing includeProcessing,
       @Nullable Artifact grepIncludes) {
-    super(owner, inputs, outputs);
-    this.localShellEnvironment = localShellEnvironment;
+    super(owner, inputs, outputs, env);
     this.outputFile = outputFile;
     this.sourceFile = sourceFile;
     this.optionalSourceFile = optionalSourceFile;
@@ -746,8 +745,15 @@ public ImmutableCollection<String> getDefines() {
   }
 
   @Override
+  @VisibleForTesting
   public ImmutableMap<String, String> getEnvironment() {
-    Map<String, String> environment = new LinkedHashMap<>(localShellEnvironment);
+    return getEnvironment(ImmutableMap.of());
+  }
+
+  public ImmutableMap<String, String> getEnvironment(Map<String, String> clientEnv) {
+    Map<String, String> environment = new LinkedHashMap<>(env.size());
+    env.resolve(environment, clientEnv);
+
     if (!getExecutionInfo().containsKey(ExecutionRequirements.REQUIRES_DARWIN)) {
       // Linux: this prevents gcc/clang from writing the unpredictable (and often irrelevant) value
       // of getcwd() into the debug info. Not applicable to Darwin or Windows, which have no /proc.
@@ -786,6 +792,7 @@ public ExtraActionInfo.Builder getExtraActionInfo(ActionKeyContext actionKeyCont
       info.addAllSourcesAndHeaders(
           Artifact.toExecPaths(ccCompilationContext.getDeclaredIncludeSrcs()));
     }
+    // TODO(ulfjack): Extra actions currently ignore the client environment.
     for (Map.Entry<String, String> envVariable : getEnvironment().entrySet()) {
       info.addVariable(
           EnvironmentVariable.newBuilder()
@@ -1105,7 +1112,9 @@ public ResourceSet estimateResourceConsumptionLocal() {
   @Override
   public void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) {
     fp.addUUID(actionClassId);
-    fp.addStringMap(getEnvironment());
+    fp.addStringMap(env.getFixedEnv());
+    fp.addStrings(env.getInheritedEnv());
+    fp.addStringMap(compileCommandLine.getEnvironment());
     fp.addStringMap(executionInfo);
 
     // For the argv part of the cache key, ignore all compiler flags that explicitly denote module

src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java

@@ -14,13 +14,15 @@
 
 package com.google.devtools.build.lib.rules.cpp;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Functions;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicates;
 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.ActionEnvironment;
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.RuleContext;
@@ -78,7 +80,7 @@ public class CppCompileActionBuilder {
   private CppSemantics cppSemantics;
   private CcToolchainProvider ccToolchain;
   @Nullable private final Artifact grepIncludes;
-  private final ImmutableMap<String, String> localShellEnvironment;
+  private ActionEnvironment env;
   private final boolean codeCoverageEnabled;
   @Nullable private String actionName;
   private ImmutableList<Artifact> builtinIncludeFiles;
@@ -122,7 +124,7 @@ private CppCompileActionBuilder(
     this.mandatoryInputsBuilder = NestedSetBuilder.stableOrder();
     this.additionalIncludeScanningRoots = new ImmutableList.Builder<>();
     this.allowUsingHeaderModules = true;
-    this.localShellEnvironment = configuration.getLocalShellEnvironment();
+    this.env = configuration.getActionEnvironment();
     this.codeCoverageEnabled = configuration.isCodeCoverageEnabled();
     this.ccToolchain = ccToolchain;
     this.grepIncludes = grepIncludes;
@@ -159,7 +161,7 @@ public CppCompileActionBuilder(CppCompileActionBuilder other) {
     this.lipoScannableMap = other.lipoScannableMap;
     this.shouldScanIncludes = other.shouldScanIncludes;
     this.executionInfo = new LinkedHashMap<>(other.executionInfo);
-    this.localShellEnvironment = other.localShellEnvironment;
+    this.env = other.env;
     this.codeCoverageEnabled = other.codeCoverageEnabled;
     this.cppSemantics = other.cppSemantics;
     this.ccToolchain = other.ccToolchain;
@@ -378,7 +380,7 @@ public CppCompileAction buildAndVerify(Consumer<String> errorCollector) {
               outputFile,
               tempOutputFile,
               dotdFile,
-              localShellEnvironment,
+              env,
               ccCompilationContext,
               coptsFilter,
               getLipoScannables(realMandatoryInputs),
@@ -409,7 +411,7 @@ public CppCompileAction buildAndVerify(Consumer<String> errorCollector) {
               dwoFile,
               ltoIndexingFile,
               optionalSourceFile,
-              localShellEnvironment,
+              env,
               ccCompilationContext,
               coptsFilter,
               getLipoScannables(realMandatoryInputs),
@@ -719,4 +721,13 @@ public PathFragment getRealOutputFilePath() {
       return getOutputFile().getExecPath();
     }
   }
+
+  /**
+   * Do not use! This method is only intended for testing.
+   */
+  @VisibleForTesting
+  public CppCompileActionBuilder setActionEnvironment(ActionEnvironment env) {
+    this.env = env;
+    return this;
+  }
 }

src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java

@@ -22,7 +22,9 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.actions.AbstractAction;
+import com.google.devtools.build.lib.actions.ActionEnvironment;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
@@ -63,6 +65,7 @@
 import java.io.IOException;
 import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
 import javax.annotation.Nullable;
 
 /** Action that represents a linking step. */
@@ -107,8 +110,6 @@ public Artifact create(RuleContext ruleContext, BuildConfiguration configuration
   private final LibraryToLink outputLibrary;
   private final Artifact linkOutput;
   private final LibraryToLink interfaceOutputLibrary;
-  private final ImmutableSet<String> clientEnvironmentVariables;
-  private final ImmutableMap<String, String> actionEnv;
   private final ImmutableMap<String, String> toolchainEnv;
   private final ImmutableSet<String> executionRequirements;
   private final ImmutableList<Artifact> linkstampObjects;
@@ -164,14 +165,13 @@ public Artifact create(RuleContext ruleContext, BuildConfiguration configuration
       boolean isLtoIndexing,
       ImmutableList<Artifact> linkstampObjects,
       LinkCommandLine linkCommandLine,
-      ImmutableSet<String> clientEnvironmentVariables,
-      ImmutableMap<String, String> actionEnv,
+      ActionEnvironment env,
       ImmutableMap<String, String> toolchainEnv,
       ImmutableSet<String> executionRequirements,
       PathFragment ldExecutable,
       String hostSystemName,
       String targetCpu) {
-    super(owner, inputs, outputs);
+    super(owner, inputs, outputs, env);
     if (mnemonic == null) {
       this.mnemonic = (isLtoIndexing) ? "CppLTOIndexing" : "CppLink";
     } else {
@@ -186,8 +186,6 @@ public Artifact create(RuleContext ruleContext, BuildConfiguration configuration
     this.isLtoIndexing = isLtoIndexing;
     this.linkstampObjects = linkstampObjects;
     this.linkCommandLine = linkCommandLine;
-    this.clientEnvironmentVariables = clientEnvironmentVariables;
-    this.actionEnv = actionEnv;
     this.toolchainEnv = toolchainEnv;
     this.executionRequirements = executionRequirements;
     this.ldExecutable = ldExecutable;
@@ -211,15 +209,15 @@ public Iterable<Artifact> getPossibleInputsForTesting() {
   }
 
   @Override
-  public Iterable<String> getClientEnvironmentVariables() {
-    return clientEnvironmentVariables;
+  @VisibleForTesting
+  public ImmutableMap<String, String> getEnvironment() {
+    return getEnvironment(ImmutableMap.of());
   }
 
-  @Override
-  public ImmutableMap<String, String> getEnvironment() {
-    LinkedHashMap<String, String> result = new LinkedHashMap<>();
+  public ImmutableMap<String, String> getEnvironment(Map<String, String> clientEnv) {
+    LinkedHashMap<String, String> result = Maps.newLinkedHashMapWithExpectedSize(env.size());
+    env.resolve(result, clientEnv);
 
-    result.putAll(actionEnv);
     result.putAll(toolchainEnv);
 
     if (!executionRequirements.contains(ExecutionRequirements.REQUIRES_DARWIN)) {
@@ -313,7 +311,7 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext)
             new SimpleSpawn(
                 this,
                 ImmutableList.copyOf(getCommandLine(actionExecutionContext.getArtifactExpander())),
-                getEnvironment(),
+                getEnvironment(actionExecutionContext.getClientEnv()),
                 getExecutionInfo(),
                 ImmutableList.copyOf(getMandatoryInputs()),
                 getOutputs().asList(),

src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java

@@ -1204,8 +1204,7 @@ public CppLinkAction build() throws InterruptedException {
             .map(Linkstamp::getArtifact)
             .collect(ImmutableList.toImmutableList()),
         linkCommandLine,
-        configuration.getVariableShellEnvironment(),
-        configuration.getLocalShellEnvironment(),
+        configuration.getActionEnvironment(),
         toolchainEnv,
         executionRequirements.build(),
         toolchain.getToolPathFragment(Tool.LD),

src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java

@@ -20,6 +20,7 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.ActionEnvironment;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.ActionOwner;
@@ -73,7 +74,7 @@ public class FakeCppCompileAction extends CppCompileAction {
       Artifact outputFile,
       PathFragment tempOutputFile,
       DotdFile dotdFile,
-      ImmutableMap<String, String> localShellEnvironment,
+      ActionEnvironment env,
       CcCompilationContext ccCompilationContext,
       CoptsFilter nocopts,
       Iterable<IncludeScannable> lipoScannables,
@@ -102,7 +103,7 @@ public class FakeCppCompileAction extends CppCompileAction {
         /* dwoFile=*/ null,
         /* ltoIndexingFile=*/ null,
         /* optionalSourceFile=*/ null,
-        localShellEnvironment,
+        env,
         // We only allow inclusion of header files explicitly declared in
         // "srcs", so we only use declaredIncludeSrcs, not declaredIncludeDirs.
         // (Disallowing use of undeclared headers for cc_fake_binary is needed

src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java

@@ -52,7 +52,7 @@ public CppCompileActionResult execWithReply(
         new SimpleSpawn(
             action,
             ImmutableList.copyOf(action.getArguments()),
-            ImmutableMap.copyOf(action.getEnvironment()),
+            ImmutableMap.copyOf(action.getEnvironment(actionExecutionContext.getClientEnv())),
             ImmutableMap.copyOf(action.getExecutionInfo()),
             EmptyRunfilesSupplier.INSTANCE,
             ImmutableMap.of(),