Starts threading ActionExecutionContext to sites calling getPath() only within
Action execution.
PiperOrigin-RevId: 192488641
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java
index ed68eb5..09dfaa6 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java
@@ -106,8 +106,8 @@
return inputPath == null ? getPrimaryInput().getExecPath() : inputPath;
}
- public Path getOutputPath() {
- return getPrimaryOutput().getPath();
+ public Path getOutputPath(ActionExecutionContext actionExecutionContext) {
+ return actionExecutionContext.getInputPath(getPrimaryOutput());
}
@Override
@@ -120,7 +120,7 @@
srcPath = actionExecutionContext.getExecRoot().getRelative(inputPath);
}
try {
- getOutputPath().createSymbolicLink(srcPath);
+ getOutputPath(actionExecutionContext).createSymbolicLink(srcPath);
} catch (IOException e) {
throw new ActionExecutionException("failed to create symbolic link '"
+ Iterables.getOnlyElement(getOutputs()).prettyPrint()
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 4448eaf..66ad1c6 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
@@ -31,7 +31,6 @@
import com.google.devtools.build.lib.actions.ActionResult;
import com.google.devtools.build.lib.actions.ActionStatusMessage;
import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
import com.google.devtools.build.lib.actions.ArtifactResolver;
import com.google.devtools.build.lib.actions.CommandAction;
import com.google.devtools.build.lib.actions.CommandLineExpansionException;
@@ -50,7 +49,6 @@
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.rules.cpp.CcCommon.CoptsFilter;
@@ -492,8 +490,7 @@
initialResult =
actionExecutionContext
.getContext(CppIncludeScanningContext.class)
- .findAdditionalInputs(
- this, actionExecutionContext, includeProcessing);
+ .findAdditionalInputs(this, actionExecutionContext, includeProcessing);
} catch (ExecException e) {
throw e.toActionExecutionException(
"Include scanning of rule '" + getOwner().getLabel() + "'",
@@ -522,9 +519,7 @@
@Override
public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext)
throws ActionExecutionException, InterruptedException {
- actionExecutionContext
- .getEventBus()
- .post(ActionStatusMessage.analysisStrategy(this));
+ actionExecutionContext.getEventBus().post(ActionStatusMessage.analysisStrategy(this));
Iterable<Artifact> initialResult = findAdditionalInputs(actionExecutionContext);
@@ -826,36 +821,33 @@
}
/**
- * Enforce that the includes actually visited during the compile were properly
- * declared in the rules.
+ * Enforce that the includes actually visited during the compile were properly declared in the
+ * rules.
*
- * <p>The technique is to walk through all of the reported includes that gcc
- * emits into the .d file, and verify that they came from acceptable
- * relative include directories. This is done in two steps:
+ * <p>The technique is to walk through all of the reported includes that gcc emits into the .d
+ * file, and verify that they came from acceptable relative include directories. This is done in
+ * two steps:
*
- * <p>First, each included file is stripped of any include path prefix from
- * {@code quoteIncludeDirs} to produce an effective relative include dir+name.
+ * <p>First, each included file is stripped of any include path prefix from {@code
+ * quoteIncludeDirs} to produce an effective relative include dir+name.
*
- * <p>Second, the remaining directory is looked up in {@code declaredIncludeDirs},
- * a list of acceptable dirs. This list contains a set of dir fragments that
- * have been calculated by the configured target to be allowable for inclusion
- * by this source. If no match is found, an error is reported and an exception
- * is thrown.
+ * <p>Second, the remaining directory is looked up in {@code declaredIncludeDirs}, a list of
+ * acceptable dirs. This list contains a set of dir fragments that have been calculated by the
+ * configured target to be allowable for inclusion by this source. If no match is found, an error
+ * is reported and an exception is thrown.
*
* @throws ActionExecutionException iff there was an undeclared dependency
*/
@VisibleForTesting
public void validateInclusions(
- Iterable<Artifact> inputsForValidation,
- ArtifactExpander artifactExpander,
- EventHandler eventHandler)
+ ActionExecutionContext actionExecutionContext, Iterable<Artifact> inputsForValidation)
throws ActionExecutionException {
IncludeProblems errors = new IncludeProblems();
IncludeProblems warnings = new IncludeProblems();
Set<Artifact> allowedIncludes = new HashSet<>();
for (Artifact input : Iterables.concat(mandatoryInputs, prunableInputs)) {
if (input.isMiddlemanArtifact() || input.isTreeArtifact()) {
- artifactExpander.expand(input, allowedIncludes);
+ actionExecutionContext.getArtifactExpander().expand(input, allowedIncludes);
}
allowedIncludes.add(input);
}
@@ -887,10 +879,11 @@
if (FileSystemUtils.startsWithAny(input.getExecPath(), ignoreDirs)) {
continue;
}
- if (!isDeclaredIn(input, declaredIncludeDirs, declaredIncludeSrcs)) {
+ if (!isDeclaredIn(actionExecutionContext, input, declaredIncludeDirs, declaredIncludeSrcs)) {
// This call can never match the declared include sources (they would be matched above).
// There are no declared include sources we need to warn about, so use an empty set here.
- if (isDeclaredIn(input, warnIncludeDirs, ImmutableSet.<Artifact>of())) {
+ if (isDeclaredIn(
+ actionExecutionContext, input, warnIncludeDirs, ImmutableSet.<Artifact>of())) {
warnings.add(input.getExecPath().toString());
} else {
errors.add(input.getExecPath().toString());
@@ -930,11 +923,11 @@
}
if (warnings.hasProblems()) {
- eventHandler.handle(
- Event.warn(
- getOwner().getLocation(),
- warnings.getMessage(this, getSourceFile()))
- .withTag(Label.print(getOwner().getLabel())));
+ actionExecutionContext
+ .getEventHandler()
+ .handle(
+ Event.warn(getOwner().getLocation(), warnings.getMessage(this, getSourceFile()))
+ .withTag(Label.print(getOwner().getLabel())));
}
errors.assertProblemFree(this, getSourceFile());
}
@@ -945,18 +938,20 @@
}
/**
- * Returns true if an included artifact is declared in a set of allowed
- * include directories. The simple case is that the artifact's parent
- * directory is contained in the set, or is empty.
+ * Returns true if an included artifact is declared in a set of allowed include directories. The
+ * simple case is that the artifact's parent directory is contained in the set, or is empty.
*
- * <p>This check also supports a wildcard suffix of '**' for the cases where the
- * calculations are inexact.
+ * <p>This check also supports a wildcard suffix of '**' for the cases where the calculations are
+ * inexact.
*
- * <p>It also handles unseen non-nested-package subdirs by walking up the path looking
- * for matches.
+ * <p>It also handles unseen non-nested-package subdirs by walking up the path looking for
+ * matches.
*/
private static boolean isDeclaredIn(
- Artifact input, Set<PathFragment> declaredIncludeDirs, Set<Artifact> declaredIncludeSrcs) {
+ ActionExecutionContext actionExecutionContext,
+ Artifact input,
+ Set<PathFragment> declaredIncludeDirs,
+ Set<Artifact> declaredIncludeSrcs) {
// First check if it's listed in "srcs". If so, then its declared & OK.
if (declaredIncludeSrcs.contains(input)) {
return true;
@@ -982,7 +977,7 @@
}
// Still not found: see if it is in a subdir of a declared package.
Root root = input.getRoot().getRoot();
- for (Path dir = input.getPath().getParentDirectory();;) {
+ for (Path dir = actionExecutionContext.getInputPath(input).getParentDirectory(); ; ) {
if (dir.getRelative(BUILD_PATH_FRAGMENT).exists()) {
return false; // Bad: this is a sub-package, not a subdir of a declared package.
}
@@ -1183,7 +1178,7 @@
actionExecutionContext.getVerboseFailures(),
this);
}
- ensureCoverageNotesFilesExist();
+ ensureCoverageNotesFilesExist(actionExecutionContext);
// This is the .d file scanning part.
CppIncludeExtractionContext scanningContext =
@@ -1200,7 +1195,8 @@
showIncludesFilterForStderr);
} else {
discoveredInputs =
- discoverInputsFromDotdFiles(execRoot, scanningContext.getArtifactResolver(), reply);
+ discoverInputsFromDotdFiles(
+ actionExecutionContext, execRoot, scanningContext.getArtifactResolver(), reply);
}
reply = null; // Clear in-memory .d files early.
@@ -1212,10 +1208,7 @@
// because doing so would allow for incorrect builds.
// HeadersCheckingMode.NONE should only be used for ObjC build actions.
if (needsIncludeValidation) {
- validateInclusions(
- discoveredInputs,
- actionExecutionContext.getArtifactExpander(),
- actionExecutionContext.getEventHandler());
+ validateInclusions(actionExecutionContext, discoveredInputs);
}
return ActionResult.create(spawnResults);
}
@@ -1250,7 +1243,10 @@
@VisibleForTesting
public NestedSet<Artifact> discoverInputsFromDotdFiles(
- Path execRoot, ArtifactResolver artifactResolver, Reply reply)
+ ActionExecutionContext actionExecutionContext,
+ Path execRoot,
+ ArtifactResolver artifactResolver,
+ Reply reply)
throws ActionExecutionException {
if (!needsDotdInputPruning || getDotdFile() == null) {
return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
@@ -1259,7 +1255,8 @@
new HeaderDiscovery.Builder()
.setAction(this)
.setSourceFile(getSourceFile())
- .setDependencies(processDepset(execRoot, reply).getDependencies())
+ .setDependencies(
+ processDepset(actionExecutionContext, execRoot, reply).getDependencies())
.setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot))
.setAllowedDerivedinputsMap(getAllowedDerivedInputsMap());
@@ -1270,7 +1267,9 @@
return discoveryBuilder.build().discoverInputsFromDependencies(execRoot, artifactResolver);
}
- public DependencySet processDepset(Path execRoot, Reply reply) throws ActionExecutionException {
+ public DependencySet processDepset(
+ ActionExecutionContext actionExecutionContext, Path execRoot, Reply reply)
+ throws ActionExecutionException {
try {
DotdFile dotdFile = getDotdFile();
Preconditions.checkNotNull(dotdFile);
@@ -1281,7 +1280,7 @@
if (dotdFile.artifact() != null || reply == null) {
Path dotdPath;
if (dotdFile.artifact() != null) {
- dotdPath = dotdFile.getPath();
+ dotdPath = dotdFile.getPath(actionExecutionContext);
} else {
dotdPath = execRoot.getRelative(dotdFile.getSafeExecPath());
}
@@ -1307,20 +1306,23 @@
}
/**
- * Gcc only creates ".gcno" files if the compilation unit is non-empty.
- * To ensure that the set of outputs for a CppCompileAction remains consistent
- * and doesn't vary dynamically depending on the _contents_ of the input files,
- * we create empty ".gcno" files if gcc didn't create them.
+ * Gcc only creates ".gcno" files if the compilation unit is non-empty. To ensure that the set of
+ * outputs for a CppCompileAction remains consistent and doesn't vary dynamically depending on the
+ * _contents_ of the input files, we create empty ".gcno" files if gcc didn't create them.
*/
- private void ensureCoverageNotesFilesExist() throws ActionExecutionException {
+ private void ensureCoverageNotesFilesExist(ActionExecutionContext actionExecutionContext)
+ throws ActionExecutionException {
for (Artifact output : getOutputs()) {
- if (output.isFileType(CppFileTypes.COVERAGE_NOTES) // ".gcno"
- && !output.getPath().exists()) {
+ if (output.isFileType(CppFileTypes.COVERAGE_NOTES)) { // ".gcno"
+ Path outputPath = actionExecutionContext.getInputPath(output);
+ if (outputPath.exists()) {
+ continue;
+ }
try {
- FileSystemUtils.createEmptyFile(output.getPath());
+ FileSystemUtils.createEmptyFile(outputPath);
} catch (IOException e) {
throw new ActionExecutionException(
- "Error creating file '" + output.getPath() + "': " + e.getMessage(), e, this, false);
+ "Error creating file '" + outputPath + "': " + e.getMessage(), e, this, false);
}
}
}
@@ -1441,11 +1443,9 @@
return execPath == null ? artifact.getExecPath() : execPath;
}
- /**
- * @return the on-disk location of the .d file or null
- */
- public Path getPath() {
- return artifact.getPath();
+ /** @return the on-disk location of the .d file or null */
+ public Path getPath(ActionExecutionContext actionExecutionContext) {
+ return actionExecutionContext.getInputPath(artifact);
}
}
}
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 0a35544..5528aac 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
@@ -160,7 +160,8 @@
new HeaderDiscovery.Builder()
.setAction(this)
.setSourceFile(getSourceFile())
- .setDependencies(processDepset(execRoot, reply).getDependencies())
+ .setDependencies(
+ processDepset(actionExecutionContext, execRoot, reply).getDependencies())
.setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot))
.setAllowedDerivedinputsMap(getAllowedDerivedInputsMap());
@@ -183,16 +184,16 @@
// listed in the "srcs" of the cc_fake_binary or in the "srcs" of a cc_library that it
// depends on.
try {
- validateInclusions(
- discoveredInputs,
- actionExecutionContext.getArtifactExpander(),
- actionExecutionContext.getEventHandler());
+ validateInclusions(actionExecutionContext, discoveredInputs);
} catch (ActionExecutionException e) {
// TODO(bazel-team): (2009) make this into an error, once most of the current warnings
// are fixed.
- actionExecutionContext.getEventHandler().handle(Event.warn(
- getOwner().getLocation(),
- e.getMessage() + ";\n this warning may eventually become an error"));
+ actionExecutionContext
+ .getEventHandler()
+ .handle(
+ Event.warn(
+ getOwner().getLocation(),
+ e.getMessage() + ";\n this warning may eventually become an error"));
}
updateActionInputs(discoveredInputs);
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index 14157c2..7129779 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -30,10 +30,13 @@
import com.google.common.eventbus.EventBus;
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
+import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionGraph;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionKeyContext;
+import com.google.devtools.build.lib.actions.ActionLogBufferPathGenerator;
import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.ArtifactRoot;
@@ -44,6 +47,7 @@
import com.google.devtools.build.lib.actions.ResourceManager;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
+import com.google.devtools.build.lib.actions.util.DummyExecutor;
import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
import com.google.devtools.build.lib.analysis.AnalysisUtils;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
@@ -157,6 +161,7 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
+import java.util.TreeMap;
import java.util.UUID;
import javax.annotation.Nullable;
import org.junit.Before;
@@ -197,6 +202,8 @@
private LoadingOptions customLoadingOptions = null;
protected BuildConfigurationValue.Key targetConfigKey;
+ private ActionLogBufferPathGenerator actionLogBufferPathGenerator;
+
@Before
public final void initializeSkyframeExecutor() throws Exception {
initializeSkyframeExecutor(/*doPackageLoadingChecks=*/ true);
@@ -272,6 +279,9 @@
setUpSkyframe();
// Also initializes ResourceManager.
ResourceManager.instance().setAvailableResources(getStartingResources());
+ this.actionLogBufferPathGenerator =
+ new ActionLogBufferPathGenerator(
+ directories.getActionConsoleOutputDirectory(getExecRoot()));
}
public void initializeMockClient() throws IOException {
@@ -1989,4 +1999,27 @@
public Path getExecRoot() {
return directories.getExecRoot(ruleClassProvider.getRunfilesPrefix());
}
+
+ /** Creates instances of {@link ActionExecutionContext} consistent with test case. */
+ public class ActionExecutionContextBuilder {
+ private TreeMap<String, String> clientEnv = new TreeMap<>();
+ private ArtifactExpander artifactExpander = null;
+
+ public ActionExecutionContextBuilder setArtifactExpander(ArtifactExpander artifactExpander) {
+ this.artifactExpander = artifactExpander;
+ return this;
+ }
+
+ public ActionExecutionContext build() {
+ return new ActionExecutionContext(
+ new DummyExecutor(fileSystem, getExecRoot(), reporter),
+ /*actionInputFileCache=*/ null,
+ /*actionInputPrefetcher=*/ null,
+ actionKeyContext,
+ /*metadataHandler=*/ null,
+ actionLogBufferPathGenerator.generate(),
+ clientEnv,
+ artifactExpander);
+ }
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
index 8226bc1..061eac3 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
@@ -1067,7 +1067,10 @@
.setList("srcs", "a.m")
.write();
CppCompileAction compileAction = (CppCompileAction) compileAction("//lib:lib", "a.o");
- assertThat(compileAction.discoverInputsFromDotdFiles(null, null, null)).isEmpty();
+ assertThat(
+ compileAction.discoverInputsFromDotdFiles(
+ new ActionExecutionContextBuilder().build(), null, null, null))
+ .isEmpty();
}
@Test
@@ -1702,7 +1705,8 @@
createLibraryTargetWriter("//lib:lib").setList("srcs", "a.m").write();
CppCompileAction compileAction = (CppCompileAction) compileAction("//lib:lib", "a.o");
try {
- compileAction.discoverInputsFromDotdFiles(null, null, null);
+ compileAction.discoverInputsFromDotdFiles(
+ new ActionExecutionContextBuilder().build(), null, null, null);
fail("Expected ActionExecutionException");
} catch (ActionExecutionException expected) {
assertThat(expected).hasMessageThat().contains("error while parsing .d file");
