Change how to download toplevel artifacts.

With bazelbuild#16524, we download toplevel artifacts by listening to `TargetCompleteEvent` so that it works for incremental builds (e.g. change toplevel targets, delete downloaded files, etc.).

However, BES sends `TargetComplete` event also by listening to `TargetCompleteEvent` which means at the time when the consumers of BEP received `TargetComplete` events, the outputs might not have been downloaded.

This CL changes the way we download toplevel artifacts from listening `TargetCompleteEvent` to checking whether an output is a toplevel artifact after action execution. It still works for incremental builds because of the RemoteOutputChecker we introduced in bazelbuild@60c9cf3. It doesn't work for BEP yet because the download still happens in the background. A following CL will fix that.

Note that this change makes RemoteOutputChecker depending on *FULL* analysis result in incremental builds because it needs that to determine whether an output is toplevel artifact during action dirtiness check. It's incompatible with skymeld. A way to fix it is described in the code.

PiperOrigin-RevId: 529026614
Change-Id: I6c0c9bf4b3f5bb076f404e46c00bc231789a2820

src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java

@@ -588,11 +588,17 @@ public List<Artifact> getArtifacts() {
   }
 
   @SuppressWarnings({"CheckReturnValue", "FutureReturnValueIgnored"})
-  public void finalizeAction(Action action, OutputMetadataStore outputMetadataStore) {
+  public void finalizeAction(Action action, OutputMetadataStore outputMetadataStore)
+      throws IOException, InterruptedException {
     List<Artifact> inputsToDownload = new ArrayList<>();
     List<Artifact> outputsToDownload = new ArrayList<>();
 
     for (Artifact output : action.getOutputs()) {
+      var metadata = outputMetadataStore.getOutputMetadata(output);
+      if (!metadata.isRemote()) {
+        continue;
+      }
+
       if (outputsAreInputs.remove(output)) {
         if (output.isTreeArtifact()) {
           var children = outputMetadataStore.getTreeArtifactChildren((SpecialArtifact) output);

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

@@ -189,7 +189,11 @@ java_library(
     deps = [
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
+        "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
+        "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
+        "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context",
         "//src/main/java/com/google/devtools/build/lib/clock",
+        "//src/main/java/com/google/devtools/build/lib/packages",
         "//third_party:guava",
     ],
 )
@@ -223,21 +227,11 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:action_input_helper",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
-        "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
-        "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
-        "//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report",
-        "//src/main/java/com/google/devtools/build/lib/packages",
-        "//src/main/java/com/google/devtools/build/lib/remote/util",
-        "//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value",
-        "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key",
-        "//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/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
-        "//src/main/java/com/google/devtools/build/skyframe",
-        "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
         "//third_party:flogger",
         "//third_party:guava",
         "//third_party:jsr305",

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

@@ -140,7 +140,7 @@ public final class RemoteModule extends BlazeModule {
   @Nullable private RemoteOutputService remoteOutputService;
   @Nullable private TempPathGenerator tempPathGenerator;
   @Nullable private BlockWaitingModule blockWaitingModule;
-  @Nullable private ImmutableList<Pattern> patternsToDownload;
+  @Nullable private RemoteOutputChecker remoteOutputChecker;
 
   private ChannelFactory channelFactory =
       new ChannelFactory() {
@@ -287,7 +287,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
     Preconditions.checkState(actionInputFetcher == null, "actionInputFetcher must be null");
     Preconditions.checkState(remoteOptions == null, "remoteOptions must be null");
     Preconditions.checkState(tempPathGenerator == null, "tempPathGenerator must be null");
-    Preconditions.checkState(patternsToDownload == null, "patternsToDownload must be null");
+    Preconditions.checkState(remoteOutputChecker == null, "remoteOutputChecker must be null");
 
     RemoteOptions remoteOptions = env.getOptions().getOptions(RemoteOptions.class);
     if (remoteOptions == null) {
@@ -344,7 +344,13 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
       for (String regex : remoteOptions.remoteDownloadRegex) {
         patternsToDownloadBuilder.add(Pattern.compile(regex));
       }
-      patternsToDownload = patternsToDownloadBuilder.build();
+
+      remoteOutputChecker =
+          new RemoteOutputChecker(
+              new JavaClock(),
+              env.getCommandName(),
+              remoteOptions.remoteOutputsMode.downloadToplevelOutputsOnly(),
+              patternsToDownloadBuilder.build());
     }
 
     env.getEventBus().register(this);
@@ -736,6 +742,19 @@ public void afterTopLevelTargetAnalysis(
       BuildRequest request,
       BuildOptions buildOptions,
       ConfiguredTarget configuredTarget) {
+    if (remoteOutputChecker != null) {
+      // remoteOutputChecker needs analysis result to determine whether an output is toplevel
+      // artifact. We could feed configuredTarget one by one for the clean build to work. However,
+      // for an incremental build, it needs *ALL* toplevel targets to check action dirtiness in
+      // {@link SequencedSkyframeExecutor#detectModifiedOutputFiles}.
+      //
+      // One way to make it work with skymeld is, instead of calling detectModifiedOutputFiles once
+      // in {@link ExecutionTool#prepareForExecution} after the first toplevel target is analyzed,
+      // we call detectModifiedOutputFiles for each toplevel target.
+      //
+      // TODO(chiwang): Make it work with skymeld.
+      throw new UnsupportedOperationException("BwoB currently doesn't support skymeld");
+    }
     if (shouldParseNoCacheOutputs()) {
       parseNoCacheOutputsFromSingleConfiguredTarget(
           Preconditions.checkNotNull(buildEventArtifactUploaderFactoryDelegate.get()),
@@ -749,6 +768,10 @@ public void afterAnalysis(
       BuildRequest request,
       BuildOptions buildOptions,
       AnalysisResult analysisResult) {
+    if (remoteOutputChecker != null) {
+      remoteOutputChecker.afterAnalysis(analysisResult);
+    }
+
     if (shouldParseNoCacheOutputs()) {
       parseNoCacheOutputs(analysisResult);
     }
@@ -854,7 +877,7 @@ public void afterCommand() throws AbruptExitException {
     remoteOutputService = null;
     tempPathGenerator = null;
     rpcLogFile = null;
-    patternsToDownload = null;
+    remoteOutputChecker = null;
   }
 
   private static void afterCommandTask(
@@ -963,9 +986,7 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
     RemoteOutputsMode remoteOutputsMode = remoteOptions.remoteOutputsMode;
 
     if (!remoteOutputsMode.downloadAllOutputs() && actionContextProvider.getRemoteCache() != null) {
-      Preconditions.checkNotNull(patternsToDownload, "patternsToDownload must not be null");
-
-      var remoteOutputChecker = new RemoteOutputChecker(new JavaClock(), patternsToDownload);
+      Preconditions.checkNotNull(remoteOutputChecker, "remoteOutputChecker must not be null");
 
       actionInputFetcher =
           new RemoteActionInputFetcher(
@@ -983,9 +1004,6 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
 
       toplevelArtifactsDownloader =
           new ToplevelArtifactsDownloader(
-              env.getCommandName(),
-              remoteOutputsMode.downloadToplevelOutputsOnly(),
-              env.getSkyframeExecutor().getEvaluator(),
               actionInputFetcher,
               env.getExecRoot().asFragment(),
               (path) -> {

src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java

@@ -14,32 +14,148 @@
 package com.google.devtools.build.lib.remote;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.devtools.build.lib.packages.TargetUtils.isTestRuleName;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
 import com.google.devtools.build.lib.actions.RemoteArtifactChecker;
+import com.google.devtools.build.lib.analysis.AnalysisResult;
+import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.FilesToRunProvider;
+import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
+import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper;
+import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
 import com.google.devtools.build.lib.clock.Clock;
+import java.util.Set;
+import java.util.function.Supplier;
 import java.util.regex.Pattern;
 
 /** A {@link RemoteArtifactChecker} that checks the TTL of remote metadata. */
 public class RemoteOutputChecker implements RemoteArtifactChecker {
+  private enum CommandMode {
+    UNKNOWN,
+    BUILD,
+    TEST,
+    RUN,
+    COVERAGE;
+  }
+
   private final Clock clock;
+  private final CommandMode commandMode;
+  private final boolean downloadToplevel;
   private final ImmutableList<Pattern> patternsToDownload;
+  private final Set<Artifact> toplevelArtifactsToDownload = Sets.newConcurrentHashSet();
 
-  public RemoteOutputChecker(Clock clock, ImmutableList<Pattern> patternsToDownload) {
+  public RemoteOutputChecker(
+      Clock clock,
+      String commandName,
+      boolean downloadToplevel,
+      ImmutableList<Pattern> patternsToDownload) {
     this.clock = clock;
+    switch (commandName) {
+      case "build":
+        this.commandMode = CommandMode.BUILD;
+        break;
+      case "test":
+        this.commandMode = CommandMode.TEST;
+        break;
+      case "run":
+        this.commandMode = CommandMode.RUN;
+        break;
+      case "coverage":
+        this.commandMode = CommandMode.COVERAGE;
+        break;
+      default:
+        this.commandMode = CommandMode.UNKNOWN;
+    }
+    this.downloadToplevel = downloadToplevel;
     this.patternsToDownload = patternsToDownload;
   }
 
+  // TODO(chiwang): Code path reserved for skymeld.
+  public void afterTopLevelTargetAnalysis(
+      ConfiguredTarget configuredTarget,
+      Supplier<TopLevelArtifactContext> topLevelArtifactContextSupplier) {
+    addTopLevelTarget(configuredTarget, topLevelArtifactContextSupplier);
+  }
+
+  public void afterAnalysis(AnalysisResult analysisResult) {
+    for (var target : analysisResult.getTargetsToBuild()) {
+      addTopLevelTarget(target, analysisResult::getTopLevelContext);
+    }
+    var targetsToTest = analysisResult.getTargetsToTest();
+    if (targetsToTest != null) {
+      for (var target : targetsToTest) {
+        addTopLevelTarget(target, analysisResult::getTopLevelContext);
+      }
+    }
+  }
+
+  private void addTopLevelTarget(
+      ConfiguredTarget toplevelTarget,
+      Supplier<TopLevelArtifactContext> topLevelArtifactContextSupplier) {
+    if (shouldDownloadToplevelOutputs(toplevelTarget)) {
+      var topLevelArtifactContext = topLevelArtifactContextSupplier.get();
+      var artifactsToBuild =
+          TopLevelArtifactHelper.getAllArtifactsToBuild(toplevelTarget, topLevelArtifactContext)
+              .getImportantArtifacts();
+      toplevelArtifactsToDownload.addAll(artifactsToBuild.toList());
+
+      addRunfiles(toplevelTarget);
+    }
+  }
+
+  private void addRunfiles(ConfiguredTarget buildTarget) {
+    var runfilesProvider = buildTarget.getProvider(FilesToRunProvider.class);
+    if (runfilesProvider == null) {
+      return;
+    }
+    var runfilesSupport = runfilesProvider.getRunfilesSupport();
+    if (runfilesSupport == null) {
+      return;
+    }
+    for (Artifact runfile : runfilesSupport.getRunfiles().getArtifacts().toList()) {
+      if (runfile.isSourceArtifact()) {
+        continue;
+      }
+      toplevelArtifactsToDownload.add(runfile);
+    }
+  }
+
+  private boolean shouldDownloadToplevelOutputs(ConfiguredTarget configuredTarget) {
+    switch (commandMode) {
+      case RUN:
+        // Always download outputs of toplevel targets in RUN mode
+        return true;
+      case COVERAGE:
+      case TEST:
+        // Do not download test binary in test/coverage mode.
+        if (configuredTarget instanceof RuleConfiguredTarget) {
+          var ruleConfiguredTarget = (RuleConfiguredTarget) configuredTarget;
+          var isTestRule = isTestRuleName(ruleConfiguredTarget.getRuleClassString());
+          return !isTestRule && downloadToplevel;
+        }
+        return downloadToplevel;
+      default:
+        return downloadToplevel;
+    }
+  }
+
   public boolean shouldDownloadFile(Artifact file) {
     checkArgument(!file.isTreeArtifact(), "file must not be a tree.");
 
-    if (outputMatchesPattern(file)) {
+    if (file instanceof TreeFileArtifact) {
+      if (toplevelArtifactsToDownload.contains(file.getParent())) {
+        return true;
+      }
+    } else if (toplevelArtifactsToDownload.contains(file)) {
       return true;
     }
 
-    return false;
+    return outputMatchesPattern(file);
   }
 
   private boolean outputMatchesPattern(Artifact output) {

src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java

@@ -142,7 +142,8 @@ public void flushActionFileSystem(FileSystem actionFileSystem) throws IOExceptio
   }
 
   @Override
-  public void finalizeAction(Action action, OutputMetadataStore outputMetadataStore) {
+  public void finalizeAction(Action action, OutputMetadataStore outputMetadataStore)
+      throws IOException, InterruptedException {
     if (actionInputFetcher != null) {
       actionInputFetcher.finalizeAction(action, outputMetadataStore);
     }