Make it possible to use BwoB with Skymeld.

Particularly the downloading behavior during a build. We achieve this by incrementally building the list of artifacts to download, to make up for the lack of a complete `AnalysisResult` in skymeld. This however brings about a regression in incremental builds: instead of dirtying just the artifacts of top-level targets, we would need to dirty all action nodes in the Skyframe graph. We'd for sure be doing more work, but likely the other layers of caching would help alleviate this. This should be nonetheless addressed in a follow up CL. PiperOrigin-RevId: 540547330 Change-Id: I8a2639fee135afb629b50d28846e8a3439612f56
bazelbuild · Jun 15, 2023 · d94816a · d94816a
1 parent 11affc3
commit d94816a
Show file tree

Hide file tree

Showing 14 changed files with 157 additions and 50 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD
@@ -337,6 +337,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
         "//src/main/java/com/google/devtools/build/lib/analysis:template_expansion_exception",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_action_finished_event",
+        "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_artifacts_known_event",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report_action_factory",
         "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context",
         "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection",

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -2693,6 +2693,17 @@ java_library(
     ],
 )
 
+java_library(
+    name = "test/coverage_artifacts_known_event",
+    srcs = ["test/CoverageArtifactsKnownEvent.java"],
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
+        "//src/main/java/com/google/devtools/build/lib/events",
+        "//third_party:auto_value",
+        "//third_party:guava",
+    ],
+)
+
 java_library(
     name = "test/execution_info",
     srcs = ["test/ExecutionInfo.java"],

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageArtifactsKnownEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageArtifactsKnownEvent.java
@@ -0,0 +1,28 @@
+// Copyright 2023 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.analysis.test;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
+
+@AutoValue
+public abstract class CoverageArtifactsKnownEvent implements Postable {
+  public abstract ImmutableSet<Artifact> coverageArtifacts();
+
+  public static CoverageArtifactsKnownEvent create(ImmutableSet<Artifact> coverageArtifacts) {
+    return new AutoValue_CoverageArtifactsKnownEvent(coverageArtifacts);
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisAndExecutionPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/buildtool/AnalysisAndExecutionPhaseRunner.java
@@ -27,6 +27,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
+import com.google.devtools.build.lib.analysis.test.CoverageArtifactsKnownEvent;
 import com.google.devtools.build.lib.buildtool.buildevent.NoAnalyzeEvent;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
@@ -50,6 +51,8 @@
 import com.google.devtools.build.lib.skyframe.RepositoryMappingValue.RepositoryMappingResolutionException;
 import com.google.devtools.build.lib.skyframe.SkyframeBuildView.BuildDriverKeyTestContext;
 import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue;
+import com.google.devtools.build.lib.skyframe.TopLevelStatusEvents.AspectAnalyzedEvent;
+import com.google.devtools.build.lib.skyframe.TopLevelStatusEvents.TestAnalyzedEvent;
 import com.google.devtools.build.lib.skyframe.TopLevelStatusEvents.TopLevelTargetAnalyzedEvent;
 import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.util.DetailedExitCode;
@@ -351,14 +354,35 @@ public static TopLevelTargetAnalysisWatcher createAndRegisterWithEventBus(
     }
 
     @Subscribe
-    public void handleTopLevelEntityAnalysisConcluded(TopLevelTargetAnalyzedEvent e)
+    public void handleTopLevelTargetAnalysisConcluded(TopLevelTargetAnalyzedEvent e)
         throws ViewCreationFailedException, InterruptedException {
       for (BlazeModule blazeModule : blazeModules) {
         blazeModule.afterTopLevelTargetAnalysis(
             env, buildRequest, buildOptions, e.configuredTarget());
       }
     }
 
+    @Subscribe
+    public void handleAspectAnalyzed(AspectAnalyzedEvent e) {
+      for (BlazeModule blazeModule : blazeModules) {
+        blazeModule.afterSingleAspectAnalysis(buildRequest, e.configuredAspect());
+      }
+    }
+
+    @Subscribe
+    public void handleTestAnalyzed(TestAnalyzedEvent e) {
+      for (BlazeModule blazeModule : blazeModules) {
+        blazeModule.afterSingleTestAnalysis(buildRequest, e.configuredTarget());
+      }
+    }
+
+    @Subscribe
+    public void handleKnownCoverageArtifacts(CoverageArtifactsKnownEvent e) {
+      for (BlazeModule blazeModule : blazeModules) {
+        blazeModule.coverageArtifactsKnown(e.coverageArtifacts());
+      }
+    }
+
     @Override
     public void close() {
       env.getEventBus().unregister(this);

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
@@ -307,14 +307,15 @@ public void prepareForExecution(Stopwatch executionTimer)
     }
 
     skyframeExecutor.drainChangedFiles();
-    var remoteArtifactChecker =
-        env.getOutputService() != null
-            ? env.getOutputService().getRemoteArtifactChecker()
-            : RemoteArtifactChecker.IGNORE_ALL;
+    // With Skymeld, we don't have enough information at this stage to consider remote files.
+    // This allows BwoB to function (in the sense that it's now compatible with Skymeld), but
+    // there's a performance penalty for incremental build: all action nodes will be dirtied.
+    // We'll be relying on the other forms of caching (local action cache or remote cache).
+    // TODO(b/281655526): Improve this.
     skyframeExecutor.detectModifiedOutputFiles(
         modifiedOutputFiles,
         env.getBlazeWorkspace().getLastExecutionTimeRange(),
-        remoteArtifactChecker,
+        RemoteArtifactChecker.IGNORE_ALL,
         buildRequestOptions.fsvcThreads);
     try (SilentCloseable c = Profiler.instance().profile("configureActionExecutor")) {
       skyframeExecutor.configureActionExecutor(

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java
@@ -30,13 +30,15 @@
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.util.concurrent.ListeningScheduledExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.AnalysisResult;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
+import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
@@ -759,17 +761,8 @@ public void afterTopLevelTargetAnalysis(
       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");
+      remoteOutputChecker.afterTopLevelTargetAnalysis(
+          configuredTarget, request::getTopLevelArtifactContext);
     }
     if (shouldParseNoCacheOutputs()) {
       parseNoCacheOutputsFromSingleConfiguredTarget(
@@ -778,6 +771,28 @@ public void afterTopLevelTargetAnalysis(
     }
   }
 
+  @Override
+  public void afterSingleAspectAnalysis(BuildRequest request, ConfiguredAspect configuredTarget) {
+    if (remoteOutputChecker != null) {
+      remoteOutputChecker.afterAspectAnalysis(
+          configuredTarget, request::getTopLevelArtifactContext);
+    }
+  }
+
+  @Override
+  public void afterSingleTestAnalysis(BuildRequest request, ConfiguredTarget configuredTarget) {
+    if (remoteOutputChecker != null) {
+      remoteOutputChecker.afterTestAnalyzedEvent(configuredTarget);
+    }
+  }
+
+  @Override
+  public void coverageArtifactsKnown(ImmutableSet<Artifact> coverageArtifacts) {
+    if (remoteOutputChecker != null) {
+      remoteOutputChecker.coverageArtifactsKnown(coverageArtifacts);
+    }
+  }
+
   @Override
   public void afterAnalysis(
       CommandEnvironment env,

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java
@@ -26,6 +26,7 @@
 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.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.ProviderCollection;
@@ -83,13 +84,31 @@ public RemoteOutputChecker(
     this.patternsToDownload = patternsToDownload;
   }
 
-  // TODO(chiwang): Code path reserved for skymeld.
+  // Skymeld-only.
   public void afterTopLevelTargetAnalysis(
       ConfiguredTarget configuredTarget,
       Supplier<TopLevelArtifactContext> topLevelArtifactContextSupplier) {
     addTopLevelTarget(configuredTarget, configuredTarget, topLevelArtifactContextSupplier);
   }
 
+  // Skymeld-only.
+  public void afterTestAnalyzedEvent(ConfiguredTarget configuredTarget) {
+    addTargetUnderTest(configuredTarget);
+  }
+
+  // Skymeld-only.
+  public void afterAspectAnalysis(
+      ConfiguredAspect configuredAspect,
+      Supplier<TopLevelArtifactContext> topLevelArtifactContextSupplier) {
+    addTopLevelTarget(
+        configuredAspect, /* configuredTarget= */ null, topLevelArtifactContextSupplier);
+  }
+
+  // Skymeld-only
+  public void coverageArtifactsKnown(ImmutableSet<Artifact> coverageArtifacts) {
+    maybeAddCoverageArtifacts(coverageArtifacts);
+  }
+
   public void afterAnalysis(AnalysisResult analysisResult) {
     for (var target : analysisResult.getTargetsToBuild()) {
       addTopLevelTarget(target, target, analysisResult::getTopLevelContext);
@@ -100,8 +119,9 @@ public void afterAnalysis(AnalysisResult analysisResult) {
     var targetsToTest = analysisResult.getTargetsToTest();
     if (targetsToTest != null) {
       for (var target : targetsToTest) {
-        addTargetUnderTest(target, analysisResult.getArtifactsToBuild());
+        addTargetUnderTest(target);
       }
+      maybeAddCoverageArtifacts(analysisResult.getArtifactsToBuild());
     }
   }
 
@@ -152,8 +172,7 @@ private void addRunfiles(ProviderCollection buildTarget) {
     }
   }
 
-  private void addTargetUnderTest(
-      ProviderCollection target, ImmutableSet<Artifact> artifactsToBuild) {
+  private void addTargetUnderTest(ProviderCollection target) {
     TestProvider testProvider = checkNotNull(target.getProvider(TestProvider.class));
     if (downloadToplevel && commandMode == CommandMode.TEST) {
       // In test mode, download the outputs of the test runner action.
@@ -164,10 +183,16 @@ private void addTargetUnderTest(
       // Do this even for MINIMAL, since coverage (unlike test) doesn't produce any observable
       // results other than outputs.
       toplevelArtifactsToDownload.addAll(testProvider.getTestParams().getCoverageArtifacts());
-      for (Artifact artifactToBuild : artifactsToBuild) {
-        if (artifactToBuild.getArtifactOwner().equals(COVERAGE_REPORT_KEY)) {
-          toplevelArtifactsToDownload.add(artifactToBuild);
-        }
+    }
+  }
+
+  private void maybeAddCoverageArtifacts(ImmutableSet<Artifact> artifactsToBuild) {
+    if (commandMode != CommandMode.COVERAGE) {
+      return;
+    }
+    for (Artifact artifactToBuild : artifactsToBuild) {
+      if (artifactToBuild.getArtifactOwner().equals(COVERAGE_REPORT_KEY)) {
+        toplevelArtifactsToDownload.add(artifactToBuild);
       }
     }
   }

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java
@@ -15,10 +15,13 @@
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.eventbus.SubscriberExceptionHandler;
+import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.AnalysisResult;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.BlazeVersionInfo;
+import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.ServerDirectories;
@@ -283,6 +286,12 @@ public void afterTopLevelTargetAnalysis(
       ConfiguredTarget configuredTarget)
       throws InterruptedException, ViewCreationFailedException {}
 
+  public void afterSingleAspectAnalysis(BuildRequest request, ConfiguredAspect configuredTarget) {}
+
+  public void afterSingleTestAnalysis(BuildRequest request, ConfiguredTarget configuredTarget) {}
+
+  public void coverageArtifactsKnown(ImmutableSet<Artifact> coverageArtifacts) {}
+
   /**
    * Called when Bazel initializes the action execution subsystem. This is called once per build if
    * action execution is enabled. Modules can override this method to affect how execution is

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -273,6 +273,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:template_expansion_exception",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/analysis_failure_propagation_exception",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_action_finished_event",
+        "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_artifacts_known_event",
         "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_collection",
         "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context",
         "//src/main/java/com/google/devtools/build/lib/analysis:view_creation_failed_exception",
@@ -2357,7 +2358,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/pkgcache",
-        "//src/main/java/com/google/devtools/build/lib/remote/options",
     ],
 )
 

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -69,6 +69,7 @@
 import com.google.devtools.build.lib.analysis.config.StarlarkTransitionCache;
 import com.google.devtools.build.lib.analysis.test.AnalysisFailurePropagationException;
 import com.google.devtools.build.lib.analysis.test.CoverageActionFinishedEvent;
+import com.google.devtools.build.lib.analysis.test.CoverageArtifactsKnownEvent;
 import com.google.devtools.build.lib.bugreport.BugReport;
 import com.google.devtools.build.lib.bugreport.BugReporter;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildMetrics.BuildGraphMetrics;
@@ -664,14 +665,13 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets(
           skyframeExecutor.setTestTypeResolver(null);
 
           // Coverage needs to be done after the list of analyzed targets/tests is known.
+          ImmutableSet<Artifact> coverageArtifacts =
+              coverageReportActionsWrapperSupplier.getCoverageArtifacts(
+                  buildResultListener.getAnalyzedTargets(), buildResultListener.getAnalyzedTests());
+          eventBus.post(CoverageArtifactsKnownEvent.create(coverageArtifacts));
           additionalArtifactsResult =
               skyframeExecutor.evaluateSkyKeys(
-                  eventHandler,
-                  Artifact.keys(
-                      coverageReportActionsWrapperSupplier.getCoverageArtifacts(
-                          buildResultListener.getAnalyzedTargets(),
-                          buildResultListener.getAnalyzedTests())),
-                  keepGoing);
+                  eventHandler, Artifact.keys(coverageArtifacts), keepGoing);
           eventBus.post(new CoverageActionFinishedEvent());
           if (additionalArtifactsResult.hasError()) {
             detailedExitCodes.add(

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldModule.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldModule.java
@@ -18,8 +18,6 @@
 import com.google.devtools.build.lib.buildtool.BuildRequestOptions;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
-import com.google.devtools.build.lib.remote.options.RemoteOptions;
-import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
 import com.google.devtools.build.lib.runtime.BlazeModule;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 
@@ -99,19 +97,6 @@ boolean determineIfMergingAnalysisExecution(CommandEnvironment env) {
       effectiveValue = false;
     }
 
-    if (effectiveValue
-        && env.getOptions().getOptions(RemoteOptions.class) != null
-        && env.getOptions().getOptions(RemoteOptions.class).remoteOutputsMode
-            != RemoteOutputsMode.ALL) {
-      env.getReporter()
-          .handle(
-              Event.warn(
-                  "--experimental_merged_skyframe_analysis_execution is incompatible with"
-                      + " --remote_download_outputs=(minimal|toplevel) and its value will be"
-                      + " ignored."));
-      effectiveValue = false;
-    }
-
     // TODO(b/245873370) --check_licenses is going away.
     if (effectiveValue
         && env.getOptions().getOptions(CoreOptions.class) != null

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelStatusEvents.java b/src/main/java/com/google/devtools/build/lib/skyframe/TopLevelStatusEvents.java
@@ -200,7 +200,7 @@ public Type getType() {
   public abstract static class AspectAnalyzedEvent implements TopLevelStatusEventWithType {
     abstract AspectKey aspectKey();
 
-    abstract ConfiguredAspect configuredAspect();
+    public abstract ConfiguredAspect configuredAspect();
 
     public static AspectAnalyzedEvent create(
         AspectKey aspectKey, ConfiguredAspect configuredAspect) {

diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java
@@ -60,8 +60,6 @@ protected void setupOptions() throws Exception {
         "--remote_download_minimal",
         "--dynamic_local_strategy=standalone",
         "--dynamic_remote_strategy=remote");
-    // (b/281655526) Skymeld is incompatible.
-    addOptions("--noexperimental_merged_skyframe_analysis_execution");
   }
 
   @Override