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

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(),