Revert "Remote: Fix a bug that the XML generation is executed even if test.xml is generated when build with --remote_download_minimal."

This reverts commit 9f8c678.

Author: larsrc-google

src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java

@@ -380,28 +380,6 @@ public ActionExecutionContext withFileOutErr(FileOutErr fileOutErr) {
         nestedSetExpander);
   }
 
-  /** Allows us to create a new context that overrides the MetadataHandler with another one. */
-  public ActionExecutionContext withMetadataHandler(MetadataHandler metadataHandler) {
-    return new ActionExecutionContext(
-        executor,
-        actionInputFileCache,
-        actionInputPrefetcher,
-        actionKeyContext,
-        metadataHandler,
-        rewindingEnabled,
-        lostInputsCheck,
-        fileOutErr,
-        eventHandler,
-        clientEnv,
-        topLevelFilesets,
-        artifactExpander,
-        env,
-        actionFileSystem,
-        skyframeDepsResult,
-        nestedSetExpander,
-        syscalls);
-  }
-
   /**
    * A way of checking whether any lost inputs have been detected during the execution of this
    * action.

src/main/java/com/google/devtools/build/lib/exec/BUILD

@@ -338,14 +338,12 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
-        "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
         "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/events",
-        "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/util/io",
         "//src/main/java/com/google/devtools/build/lib/vfs",

src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java

@@ -25,22 +25,17 @@
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputHelper;
-import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
-import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.ExecutionRequirements;
-import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.SimpleSpawn;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnContinuation;
 import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.actions.TestExecException;
-import com.google.devtools.build.lib.actions.cache.MetadataHandler;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.test.TestAttempt;
 import com.google.devtools.build.lib.analysis.test.TestResult;
@@ -55,7 +50,6 @@
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
 import com.google.devtools.build.lib.server.FailureDetails.TestAction;
-import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileStatus;
@@ -73,8 +67,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
 import javax.annotation.Nullable;
 
 /** Runs TestRunnerAction actions. */
@@ -143,7 +135,7 @@ public TestRunnerSpawn createTestRunnerSpawn(
             action.getTestProperties().isPersistentTestRunner()
                 ? action.getTools()
                 : NestedSetBuilder.emptySet(Order.STABLE_ORDER),
-            createSpawnOutputs(action),
+            ImmutableSet.copyOf(action.getSpawnOutputs()),
             localResourceUsage);
     Path execRoot = actionExecutionContext.getExecRoot();
     ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver();
@@ -154,21 +146,6 @@ public TestRunnerSpawn createTestRunnerSpawn(
         action, actionExecutionContext, spawn, tmpDir, workingDirectory, execRoot);
   }
 
-  private ImmutableSet<ActionInput> createSpawnOutputs(TestRunnerAction action) {
-    ImmutableSet.Builder<ActionInput> builder = ImmutableSet.builder();
-    for (ActionInput output : action.getSpawnOutputs()) {
-      if (output.getExecPath().equals(action.getXmlOutputPath())) {
-        // HACK: Convert type of test.xml from BasicActionInput to DerivedArtifact. We want to
-        // inject metadata of test.xml if it is generated remotely and it's currently only possible
-        // to inject Artifact.
-        builder.add(createArtifactOutput(action, output.getExecPath()));
-      } else {
-        builder.add(output);
-      }
-    }
-    return builder.build();
-  }
-
   private static ImmutableList<Pair<String, Path>> renameOutputs(
       ActionExecutionContext actionExecutionContext,
       TestRunnerAction action,
@@ -317,84 +294,11 @@ private static Map<String, String> setupEnvironment(
       relativeTmpDir = tmpDir.asFragment();
     }
     return DEFAULT_LOCAL_POLICY.computeTestEnvironment(
-        action, clientEnv, getTimeout(action), runfilesDir.relativeTo(execRoot), relativeTmpDir);
-  }
-
-  static class TestMetadataHandler implements MetadataHandler {
-    private final MetadataHandler metadataHandler;
-    private final ImmutableSet<Artifact> outputs;
-    private final ConcurrentMap<Artifact, FileArtifactValue> fileMetadataMap =
-        new ConcurrentHashMap<>();
-
-    TestMetadataHandler(MetadataHandler metadataHandler, ImmutableSet<Artifact> outputs) {
-      this.metadataHandler = metadataHandler;
-      this.outputs = outputs;
-    }
-
-    @Nullable
-    @Override
-    public ActionInput getInput(String execPath) {
-      return metadataHandler.getInput(execPath);
-    }
-
-    @Nullable
-    @Override
-    public FileArtifactValue getMetadata(ActionInput input) throws IOException {
-      return metadataHandler.getMetadata(input);
-    }
-
-    @Override
-    public void setDigestForVirtualArtifact(Artifact artifact, byte[] digest) {
-      metadataHandler.setDigestForVirtualArtifact(artifact, digest);
-    }
-
-    @Override
-    public FileArtifactValue constructMetadataForDigest(
-        Artifact output, FileStatus statNoFollow, byte[] injectedDigest) throws IOException {
-      return metadataHandler.constructMetadataForDigest(output, statNoFollow, injectedDigest);
-    }
-
-    @Override
-    public ImmutableSet<TreeFileArtifact> getTreeArtifactChildren(SpecialArtifact treeArtifact) {
-      return metadataHandler.getTreeArtifactChildren(treeArtifact);
-    }
-
-    @Override
-    public TreeArtifactValue getTreeArtifactValue(SpecialArtifact treeArtifact) throws IOException {
-      return metadataHandler.getTreeArtifactValue(treeArtifact);
-    }
-
-    @Override
-    public void markOmitted(Artifact output) {
-      metadataHandler.markOmitted(output);
-    }
-
-    @Override
-    public boolean artifactOmitted(Artifact artifact) {
-      return metadataHandler.artifactOmitted(artifact);
-    }
-
-    @Override
-    public void resetOutputs(Iterable<? extends Artifact> outputs) {
-      metadataHandler.resetOutputs(outputs);
-    }
-
-    @Override
-    public void injectFile(Artifact output, FileArtifactValue metadata) {
-      if (outputs.contains(output)) {
-        metadataHandler.injectFile(output, metadata);
-      }
-      fileMetadataMap.put(output, metadata);
-    }
-
-    @Override
-    public void injectTree(SpecialArtifact output, TreeArtifactValue tree) {
-      metadataHandler.injectTree(output, tree);
-    }
-
-    public boolean fileInjected(Artifact output) {
-      return fileMetadataMap.containsKey(output);
-    }
+        action,
+        clientEnv,
+        getTimeout(action),
+        runfilesDir.relativeTo(execRoot),
+        relativeTmpDir);
   }
 
   private TestAttemptContinuation beginTestAttempt(
@@ -413,26 +317,12 @@ private TestAttemptContinuation beginTestAttempt(
           createStreamedTestOutput(
               Reporter.outErrForReporter(actionExecutionContext.getEventHandler()), out);
     }
-
-    // We use TestMetadataHandler here mainly because the one provided by actionExecutionContext
-    // doesn't allow to inject undeclared outputs and test.xml is undeclared by the test action.
-    TestMetadataHandler testMetadataHandler = null;
-    if (actionExecutionContext.getMetadataHandler() != null) {
-      testMetadataHandler =
-          new TestMetadataHandler(
-              actionExecutionContext.getMetadataHandler(), testAction.getOutputs());
-    }
-
     long startTimeMillis = actionExecutionContext.getClock().currentTimeMillis();
     SpawnStrategyResolver resolver = actionExecutionContext.getContext(SpawnStrategyResolver.class);
     SpawnContinuation spawnContinuation;
     try {
       spawnContinuation =
-          resolver.beginExecution(
-              spawn,
-              actionExecutionContext
-                  .withFileOutErr(testOutErr)
-                  .withMetadataHandler(testMetadataHandler));
+          resolver.beginExecution(spawn, actionExecutionContext.withFileOutErr(testOutErr));
     } catch (InterruptedException e) {
       if (streamed != null) {
         streamed.close();
@@ -442,7 +332,6 @@ private TestAttemptContinuation beginTestAttempt(
     }
     return new BazelTestAttemptContinuation(
         testAction,
-        testMetadataHandler,
         actionExecutionContext,
         spawn,
         resolvedPaths,
@@ -498,12 +387,6 @@ private static BuildEventStreamProtos.TestResult.ExecutionInfo extractExecutionI
     return executionInfo.build();
   }
 
-  private static Artifact.DerivedArtifact createArtifactOutput(
-      TestRunnerAction action, PathFragment outputPath) {
-    Artifact.DerivedArtifact testLog = (Artifact.DerivedArtifact) action.getTestLog();
-    return new DerivedArtifact(testLog.getRoot(), outputPath, testLog.getArtifactOwner());
-  }
-
   /**
    * A spawn to generate a test.xml file from the test log. This is only used if the test does not
    * generate a test.xml file itself.
@@ -540,7 +423,7 @@ private static Spawn createXmlGeneratingSpawn(
         /*inputs=*/ NestedSetBuilder.create(
             Order.STABLE_ORDER, action.getTestXmlGeneratorScript(), action.getTestLog()),
         /*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
-        /*outputs=*/ ImmutableSet.of(createArtifactOutput(action, action.getXmlOutputPath())),
+        /*outputs=*/ ImmutableSet.of(ActionInputHelper.fromPath(action.getXmlOutputPath())),
         SpawnAction.DEFAULT_RESOURCE_SET);
   }
 
@@ -693,7 +576,6 @@ public void finalizeCancelledTest(List<FailedAttemptResult> failedAttempts) thro
 
   private final class BazelTestAttemptContinuation extends TestAttemptContinuation {
     private final TestRunnerAction testAction;
-    @Nullable private final TestMetadataHandler testMetadataHandler;
     private final ActionExecutionContext actionExecutionContext;
     private final Spawn spawn;
     private final ResolvedPaths resolvedPaths;
@@ -706,7 +588,6 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation
 
     BazelTestAttemptContinuation(
         TestRunnerAction testAction,
-        @Nullable TestMetadataHandler testMetadataHandler,
         ActionExecutionContext actionExecutionContext,
         Spawn spawn,
         ResolvedPaths resolvedPaths,
@@ -717,7 +598,6 @@ private final class BazelTestAttemptContinuation extends TestAttemptContinuation
         TestResultData.Builder testResultDataBuilder,
         ImmutableList<SpawnResult> spawnResults) {
       this.testAction = testAction;
-      this.testMetadataHandler = testMetadataHandler;
       this.actionExecutionContext = actionExecutionContext;
       this.spawn = spawn;
       this.resolvedPaths = resolvedPaths;
@@ -758,7 +638,6 @@ public TestAttemptContinuation execute()
           if (!nextContinuation.isDone()) {
             return new BazelTestAttemptContinuation(
                 testAction,
-                testMetadataHandler,
                 actionExecutionContext,
                 spawn,
                 resolvedPaths,
@@ -848,7 +727,6 @@ public TestAttemptContinuation execute()
           appendCoverageLog(coverageOutErr, fileOutErr);
           return new BazelCoveragePostProcessingContinuation(
               testAction,
-              testMetadataHandler,
               actionExecutionContext,
               spawn,
               resolvedPaths,
@@ -884,21 +762,15 @@ public TestAttemptContinuation execute()
         throw new EnvironmentalExecException(e, Code.TEST_OUT_ERR_IO_EXCEPTION);
       }
 
-      Path xmlOutputPath = resolvedPaths.getXmlOutputPath();
-      boolean testXmlGenerated = xmlOutputPath.exists();
-      if (!testXmlGenerated && testMetadataHandler != null) {
-        testXmlGenerated =
-            testMetadataHandler.fileInjected(
-                createArtifactOutput(testAction, testAction.getXmlOutputPath()));
-      }
 
       // If the test did not create a test.xml, and --experimental_split_xml_generation is enabled,
       // then we run a separate action to create a test.xml from test.log. We do this as a spawn
       // rather than doing it locally in-process, as the test.log file may only exist remotely (when
       // remote execution is enabled), and we do not want to have to download it.
+      Path xmlOutputPath = resolvedPaths.getXmlOutputPath();
       if (executionOptions.splitXmlGeneration
           && fileOutErr.getOutputPath().exists()
-          && !testXmlGenerated) {
+          && !xmlOutputPath.exists()) {
         Spawn xmlGeneratingSpawn =
             createXmlGeneratingSpawn(testAction, spawn.getEnvironment(), spawnResults.get(0));
         SpawnStrategyResolver spawnStrategyResolver =
@@ -909,10 +781,7 @@ public TestAttemptContinuation execute()
         try {
           SpawnContinuation xmlContinuation =
               spawnStrategyResolver.beginExecution(
-                  xmlGeneratingSpawn,
-                  actionExecutionContext
-                      .withFileOutErr(xmlSpawnOutErr)
-                      .withMetadataHandler(testMetadataHandler));
+                  xmlGeneratingSpawn, actionExecutionContext.withFileOutErr(xmlSpawnOutErr));
           return new BazelXmlCreationContinuation(
               resolvedPaths, xmlSpawnOutErr, testResultDataBuilder, spawnResults, xmlContinuation);
         } catch (InterruptedException e) {
@@ -1010,7 +879,6 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept
 
   private final class BazelCoveragePostProcessingContinuation extends TestAttemptContinuation {
     private final ResolvedPaths resolvedPaths;
-    @Nullable private final TestMetadataHandler testMetadataHandler;
     private final FileOutErr fileOutErr;
     private final Closeable streamed;
     private final TestResultData.Builder testResultDataBuilder;
@@ -1022,7 +890,6 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC
 
     BazelCoveragePostProcessingContinuation(
         TestRunnerAction testAction,
-        @Nullable TestMetadataHandler testMetadataHandler,
         ActionExecutionContext actionExecutionContext,
         Spawn spawn,
         ResolvedPaths resolvedPaths,
@@ -1032,7 +899,6 @@ private final class BazelCoveragePostProcessingContinuation extends TestAttemptC
         ImmutableList<SpawnResult> primarySpawnResults,
         SpawnContinuation spawnContinuation) {
       this.testAction = testAction;
-      this.testMetadataHandler = testMetadataHandler;
       this.actionExecutionContext = actionExecutionContext;
       this.spawn = spawn;
       this.resolvedPaths = resolvedPaths;
@@ -1057,7 +923,6 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept
         if (!nextContinuation.isDone()) {
           return new BazelCoveragePostProcessingContinuation(
               testAction,
-              testMetadataHandler,
               actionExecutionContext,
               spawn,
               resolvedPaths,
@@ -1093,7 +958,6 @@ public TestAttemptContinuation execute() throws InterruptedException, ExecExcept
 
       return new BazelTestAttemptContinuation(
           testAction,
-          testMetadataHandler,
           actionExecutionContext,
           spawn,
           resolvedPaths,