Add a flag to split test.xml generation into a separate Spawn

At this time, this is only implemented for the StandaloneTestStrategy.

This solves a race condition on Posix-like systems, where we cannot guarantee that the pipes are actually fully flushed to disk when the test process exits, and this can cause the test.xml to be empty, which makes it hard to debug issues. (The test.log files do not show up in normal CI systems, only the test.xml files.)

Progress on #4608.

PiperOrigin-RevId: 206292179

4 files changed, 22 insertions, 0 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
index 4c4c501d07..2bd7daf1b9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
@@ -175,6 +175,11 @@ public class BaseRuleClasses {
                   .singleArtifact()
                   .value(env.getToolsLabel("//tools/test:test_setup")))
           .add(
+              attr("$xml_generator_script", LABEL)
+                  .cfg(HostTransition.INSTANCE)
+                  .singleArtifact()
+                  .value(env.getToolsLabel("//tools/test:test_xml_generator")))
+          .add(
               attr("$collect_coverage_script", LABEL)
                   .cfg(HostTransition.INSTANCE)
                   .singleArtifact()
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
index 34cd54fd8c..fffbe7a5ea 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
@@ -177,6 +177,12 @@ public class SkylarkRuleClassFunctions implements SkylarkRuleFunctionsApi<Artifa
                 .singleArtifact()
                 .value(labelCache.getUnchecked(toolsRepository + "//tools/test:test_setup")))
         .add(
+            attr("$xml_generator_script", LABEL)
+                .cfg(HostTransition.INSTANCE)
+                .singleArtifact()
+                .value(
+                    labelCache.getUnchecked(toolsRepository + "//tools/test:test_xml_generator")))
+        .add(
             attr("$collect_coverage_script", LABEL)
                 .cfg(HostTransition.INSTANCE)
                 .singleArtifact()
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
index 7fe18a2300..40725da69d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
@@ -204,6 +204,9 @@ public final class TestActionBuilder {
 
     Artifact testSetupScript = ruleContext.getHostPrerequisiteArtifact("$test_setup_script");
     inputsBuilder.add(testSetupScript);
+    Artifact testXmlGeneratorScript =
+        ruleContext.getHostPrerequisiteArtifact("$xml_generator_script");
+    inputsBuilder.add(testXmlGeneratorScript);
 
     Artifact collectCoverageScript = null;
     TreeMap<String, String> extraTestEnv = new TreeMap<>();
@@ -307,6 +310,7 @@ public final class TestActionBuilder {
                 ruleContext.getActionOwner(),
                 inputs,
                 testSetupScript,
+                testXmlGeneratorScript,
                 collectCoverageScript,
                 testLog,
                 cacheStatus,
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
index 3548b9aafc..5827488c09 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
@@ -73,6 +73,7 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
   private static final String GUID = "cc41f9d0-47a6-11e7-8726-eb6ce83a8cc8";
 
   private final Artifact testSetupScript;
+  private final Artifact testXmlGeneratorScript;
   private final Artifact collectCoverageScript;
   private final BuildConfiguration configuration;
   private final TestConfiguration testConfiguration;
@@ -135,6 +136,7 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
       ActionOwner owner,
       Iterable<Artifact> inputs,
       Artifact testSetupScript, // Must be in inputs
+      Artifact testXmlGeneratorScript, // Must be in inputs
       @Nullable Artifact collectCoverageScript, // Must be in inputs, if not null
       Artifact testLog,
       Artifact cacheStatus,
@@ -157,6 +159,7 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
         configuration.getActionEnvironment());
     Preconditions.checkState((collectCoverageScript == null) == (coverageArtifact == null));
     this.testSetupScript = testSetupScript;
+    this.testXmlGeneratorScript = testXmlGeneratorScript;
     this.collectCoverageScript = collectCoverageScript;
     this.configuration = Preconditions.checkNotNull(configuration);
     this.testConfiguration =
@@ -741,6 +744,10 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
     return testSetupScript;
   }
 
+  public Artifact getTestXmlGeneratorScript() {
+    return testXmlGeneratorScript;
+  }
+
   @Nullable
   public Artifact getCollectCoverageScript() {
     return collectCoverageScript;