Starts threading ActionExecutionContext to sites calling getPath() only within

Action execution.

PiperOrigin-RevId: 192488641

5 files changed, 114 insertions, 76 deletions

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 ed68eb5f0e..09dfaa660b 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 @@ public class SymlinkAction extends AbstractAction {
     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 @@ public class SymlinkAction extends AbstractAction {
       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 4448eafd0e..66ad1c603c 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.ActionOwner;
 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.NestedSetBuilder;
 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 @@ public class CppCompileAction extends AbstractAction
       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 @@ public class CppCompileAction extends AbstractAction
   @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 @@ public class CppCompileAction extends AbstractAction
   }
 
   /**
-   * 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 @@ public class CppCompileAction extends AbstractAction
       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 @@ public class CppCompileAction extends AbstractAction
     }
 
     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 @@ public class CppCompileAction extends AbstractAction
   }
 
   /**
-   * 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 @@ public class CppCompileAction extends AbstractAction
     }
     // 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 @@ public class CppCompileAction extends AbstractAction
           actionExecutionContext.getVerboseFailures(),
           this);
     }
-    ensureCoverageNotesFilesExist();
+    ensureCoverageNotesFilesExist(actionExecutionContext);
 
     // This is the .d file scanning part.
     CppIncludeExtractionContext scanningContext =
@@ -1200,7 +1195,8 @@ public class CppCompileAction extends AbstractAction
               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 @@ public class CppCompileAction extends AbstractAction
     // 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 @@ public class CppCompileAction extends AbstractAction
 
   @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 @@ public class CppCompileAction extends AbstractAction
         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 @@ public class CppCompileAction extends AbstractAction
     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 @@ public class CppCompileAction extends AbstractAction
       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 @@ public class CppCompileAction extends AbstractAction
   }
 
   /**
-   * 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 @@ public class CppCompileAction extends AbstractAction
       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 0a35544599..5528aac3bd 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 @@ public class FakeCppCompileAction extends CppCompileAction {
           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 @@ public class FakeCppCompileAction extends CppCompileAction {
     // 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 14157c260f..7129779d58 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.collect.Sets;
 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.MutableActionGraph;
 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.Collections;
 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 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
   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 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
     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 abstract class BuildViewTestCase extends FoundationTestCase {
   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 8226bc1bc8..061eac34e4 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 @@ public class ObjcLibraryTest extends ObjcRuleTestCase {
         .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 @@ public class ObjcLibraryTest extends ObjcRuleTestCase {
     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");