Make local TestStrategy bits rely less on runfiles manifests

TestStrategy#getLocalRunfilesDirectory(...) was using manifest names
to infer if the runfiles symlinks were created by comparing the name
of manifests. This is a bit confusing and adds unnecessary reliance
on manifests which we'd like to reduce. Instead get whether or not
we created the symlinks from the boolean that determines it.

--
MOS_MIGRATED_REVID=93414150

2 files changed, 28 insertions, 27 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestStrategy.java
index 98a2692331..93ce649aa2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestStrategy.java
@@ -296,16 +296,15 @@ public abstract class TestStrategy implements TestActionContext {
       InterruptedException {
     TestTargetExecutionSettings execSettings = testAction.getExecutionSettings();
 
-    // --nobuild_runfile_links disables runfiles generation only for C++ rules.
-    // In that case, getManifest returns the .runfiles_manifest (input) file,
-    // not the MANIFEST output file of the build-runfiles action. So the
-    // extension ".runfiles_manifest" indicates no runfiles tree.
-    if (!execSettings.getManifest().equals(execSettings.getInputManifest())) {
-      return execSettings.getManifest().getPath().getParentDirectory();
+    // If the symlink farm is already created then return the existing directory. If not we
+    // need to explicitly build it. This can happen when --nobuild_runfile_links is supplied
+    // as a flag to the build.
+    if (execSettings.getRunfilesSymlinksCreated()) {
+      return execSettings.getRunfilesDir();
     }
 
-    // We might need to build runfiles tree now, since it was not created yet
-    // local testing is needed.
+    // TODO(bazel-team): Should we be using TestTargetExecutionSettings#getRunfilesDir() here over
+    // generating the directory ourselves?
     Path program = execSettings.getExecutable().getPath();
     Path runfilesDir = program.getParentDirectory().getChild(program.getBaseName() + ".runfiles");
 
@@ -314,7 +313,7 @@ public abstract class TestStrategy implements TestActionContext {
     // trying to generate same runfiles tree in case of --runs_per_test > 1 or
     // local test sharding.
     long startTime = Profiler.nanoTimeMaybe();
-    synchronized (execSettings.getManifest()) {
+    synchronized (execSettings.getInputManifest()) {
       Profiler.instance().logSimpleTask(startTime, ProfilerTask.WAIT, testAction);
       updateLocalRunfilesDirectory(testAction, runfilesDir, actionExecutionContext, binTools);
     }
@@ -335,8 +334,10 @@ public abstract class TestStrategy implements TestActionContext {
 
     TestTargetExecutionSettings execSettings = testAction.getExecutionSettings();
     try {
+      // Avoid rebuilding the runfiles directory if the manifest in it matches the input manifest,
+      // implying the symlinks exist and are already up to date.
       if (Arrays.equals(runfilesDir.getRelative("MANIFEST").getMD5Digest(),
-          execSettings.getManifest().getPath().getMD5Digest())) {
+          execSettings.getInputManifest().getPath().getMD5Digest())) {
         return;
       }
     } catch (IOException e1) {
@@ -346,7 +347,7 @@ public abstract class TestStrategy implements TestActionContext {
     executor.getEventHandler().handle(Event.progress(
         "Building runfiles directory for '" + execSettings.getExecutable().prettyPrint() + "'."));
 
-    new SymlinkTreeHelper(execSettings.getManifest().getExecPath(),
+    new SymlinkTreeHelper(execSettings.getInputManifest().getExecPath(),
         runfilesDir.relativeTo(executor.getExecRoot()), /* filesetTree= */ false)
         .createSymlinks(testAction, actionExecutionContext, binTools);
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestTargetExecutionSettings.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestTargetExecutionSettings.java
index 20ad8af5ec..1948a3b330 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/TestTargetExecutionSettings.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestTargetExecutionSettings.java
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.RunUnder;
 import com.google.devtools.build.lib.packages.TargetUtils;
+import com.google.devtools.build.lib.vfs.Path;
 
 import java.util.List;
 
@@ -41,17 +42,18 @@ public final class TestTargetExecutionSettings {
   private final RunUnder runUnder;
   private final Artifact runUnderExecutable;
   private final Artifact executable;
-  private final Artifact runfilesManifest;
+  private final boolean runfilesSymlinksCreated;
+  private final Path runfilesDir;
   private final Artifact runfilesInputManifest;
   private final Artifact instrumentedFileManifest;
 
-  TestTargetExecutionSettings(RuleContext ruleContext, RunfilesSupport runfiles,
+  TestTargetExecutionSettings(RuleContext ruleContext, RunfilesSupport runfilesSupport,
       Artifact executable, Artifact instrumentedFileManifest, int shards) {
     Preconditions.checkArgument(TargetUtils.isTestRule(ruleContext.getRule()));
     Preconditions.checkArgument(shards >= 0);
     BuildConfiguration config = ruleContext.getConfiguration();
 
-    List<String> targetArgs = runfiles.getArgs();
+    List<String> targetArgs = runfilesSupport.getArgs();
     testArguments = targetArgs.isEmpty()
       ? config.getTestArguments()
       : ImmutableList.copyOf(Iterables.concat(targetArgs, config.getTestArguments()));
@@ -62,8 +64,9 @@ public final class TestTargetExecutionSettings {
 
     this.testFilter = config.getTestFilter();
     this.executable = executable;
-    this.runfilesManifest = runfiles.getRunfilesManifest();
-    this.runfilesInputManifest = runfiles.getRunfilesInputManifest();
+    this.runfilesSymlinksCreated = runfilesSupport.getCreateSymlinks();
+    this.runfilesDir = runfilesSupport.getRunfilesDirectory();
+    this.runfilesInputManifest = runfilesSupport.getRunfilesInputManifest();
     this.instrumentedFileManifest = instrumentedFileManifest;
   }
 
@@ -99,17 +102,14 @@ public final class TestTargetExecutionSettings {
     return executable;
   }
 
-  /**
-   * Returns the runfiles manifest for this test.
-   *
-   * <p>This returns either the input manifest outside of the runfiles tree,
-   * if blaze is run with --nobuild_runfile_links or the manifest inside the
-   * runfiles tree, if blaze is run with --build_runfile_links.
-   *
-   * @see com.google.devtools.build.lib.analysis.RunfilesSupport#getRunfilesManifest()
-   */
-  public Artifact getManifest() {
-    return runfilesManifest;
+  /** @return whether or not the runfiles symlinks were created */
+  public boolean getRunfilesSymlinksCreated() {
+    return runfilesSymlinksCreated;
+  }
+
+  /** @return the directory of the runfiles */
+  public Path getRunfilesDir() {
+    return runfilesDir;
   }
 
   /**