diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java index d41e7d2dcbb3bc..6d0daf8f39390c 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java +++ b/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 inputs, - Iterable outputs) { - this(owner, ImmutableList.of(), inputs, EmptyRunfilesSupplier.INSTANCE, outputs); + protected AbstractAction( + ActionOwner owner, + Iterable inputs, + Iterable outputs) { + this( + owner, + /*tools = */ImmutableList.of(), + inputs, + EmptyRunfilesSupplier.INSTANCE, + outputs, + ActionEnvironment.EMPTY); + } + + protected AbstractAction( + ActionOwner owner, + Iterable inputs, + Iterable outputs, + ActionEnvironment env) { + this( + owner, + /*tools = */ImmutableList.of(), + inputs, + EmptyRunfilesSupplier.INSTANCE, + outputs, + env); } /** @@ -120,7 +141,7 @@ protected AbstractAction( Iterable tools, Iterable inputs, Iterable outputs) { - this(owner, tools, inputs, EmptyRunfilesSupplier.INSTANCE, outputs); + this(owner, tools, inputs, EmptyRunfilesSupplier.INSTANCE, outputs, ActionEnvironment.EMPTY); } protected AbstractAction( @@ -128,7 +149,13 @@ protected AbstractAction( Iterable inputs, RunfilesSupplier runfilesSupplier, Iterable outputs) { - this(owner, ImmutableList.of(), inputs, runfilesSupplier, outputs); + this( + owner, + /*tools=*/ImmutableList.of(), + inputs, + runfilesSupplier, + outputs, + ActionEnvironment.EMPTY); } protected AbstractAction( @@ -148,14 +175,12 @@ protected AbstractAction( Iterable 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"); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java b/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java index 1ac7da832d18e9..e76fd0723e9c4e 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java +++ b/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 getEnvironment(); /** Returns inputs to this action, including inputs that may be pruned. */ 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 9b826d09066184..1ea286db8d40c1 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 @@ -478,6 +478,7 @@ protected SpawnInfo getExtraActionSpawnInfo() throws CommandLineExpansionExcepti } @Override + @VisibleForTesting public final ImmutableMap getEnvironment() { // TODO(ulfjack): AbstractAction should declare getEnvironment with a return value of type // ActionEnvironment to avoid developers misunderstanding the purpose of this method. That diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index b338d5b1734457..3f8045a538fd1c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/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 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 localShellEnvironment, + ActionEnvironment env, CcCompilationContext ccCompilationContext, CoptsFilter coptsFilter, Iterable 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 inputs, ImmutableSet outputs, - ImmutableMap 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 getDefines() { } @Override + @VisibleForTesting public ImmutableMap getEnvironment() { - Map environment = new LinkedHashMap<>(localShellEnvironment); + return getEnvironment(ImmutableMap.of()); + } + + public ImmutableMap getEnvironment(Map clientEnv) { + Map 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 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 diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java index 866d1a542c4efd..11a09dba8fa685 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java @@ -14,6 +14,7 @@ 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; @@ -21,6 +22,7 @@ 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 localShellEnvironment; + private ActionEnvironment env; private final boolean codeCoverageEnabled; @Nullable private String actionName; private ImmutableList 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 errorCollector) { outputFile, tempOutputFile, dotdFile, - localShellEnvironment, + env, ccCompilationContext, coptsFilter, getLipoScannables(realMandatoryInputs), @@ -409,7 +411,7 @@ public CppCompileAction buildAndVerify(Consumer 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; + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java index 67d6b5bbeef07b..d18aca00e7910f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java +++ b/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 clientEnvironmentVariables; - private final ImmutableMap actionEnv; private final ImmutableMap toolchainEnv; private final ImmutableSet executionRequirements; private final ImmutableList linkstampObjects; @@ -164,14 +165,13 @@ public Artifact create(RuleContext ruleContext, BuildConfiguration configuration boolean isLtoIndexing, ImmutableList linkstampObjects, LinkCommandLine linkCommandLine, - ImmutableSet clientEnvironmentVariables, - ImmutableMap actionEnv, + ActionEnvironment env, ImmutableMap toolchainEnv, ImmutableSet 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 getPossibleInputsForTesting() { } @Override - public Iterable getClientEnvironmentVariables() { - return clientEnvironmentVariables; + @VisibleForTesting + public ImmutableMap getEnvironment() { + return getEnvironment(ImmutableMap.of()); } - @Override - public ImmutableMap getEnvironment() { - LinkedHashMap result = new LinkedHashMap<>(); + public ImmutableMap getEnvironment(Map clientEnv) { + LinkedHashMap 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(), diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java index 69294f04ef1bfb..63a97b7055ce89 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java +++ b/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), diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java index ab00b5c9ab219e..de6a916802a968 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java +++ b/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 localShellEnvironment, + ActionEnvironment env, CcCompilationContext ccCompilationContext, CoptsFilter nocopts, Iterable 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 diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java index fe35d84f2978ff..2ce7ec48ab6716 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java +++ b/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(),