[Skymeld] Move the various skymeld flag check to SkymeldModule.

Also print an INFO message that Skymeld is enabled. PiperOrigin-RevId: 520641129 Change-Id: I9d3301a85aba19569c548ad0828c26326fd504da
bazelbuild · Mar 30, 2023 · 986ef7b · 986ef7b
1 parent 03266a8
commit 986ef7b
Show file tree

Hide file tree

Showing 9 changed files with 99 additions and 66 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/BUILD
@@ -159,6 +159,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/remote",
         "//src/main/java/com/google/devtools/build/lib/runtime/mobileinstall",
         "//src/main/java/com/google/devtools/build/lib/sandbox:sandbox_module",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:skymeld_module",
         "//src/main/java/com/google/devtools/build/lib/standalone",
         "//src/main/java/com/google/devtools/build/lib/starlarkdebug/module",
         "//src/main/java/com/google/devtools/build/lib/worker:worker_module",

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/Bazel.java b/src/main/java/com/google/devtools/build/lib/bazel/Bazel.java
@@ -84,6 +84,7 @@ public final class Bazel {
           com.google.devtools.build.lib.runtime.ExecutionGraphModule.class,
           BazelBuiltinCommandModule.class,
           com.google.devtools.build.lib.includescanning.IncludeScanningModule.class,
+          com.google.devtools.build.lib.skyframe.SkymeldModule.class,
           // This module needs to be registered after any module submitting tasks with its {@code
           // submit} method.
           com.google.devtools.build.lib.runtime.BlockWaitingModule.class);

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
@@ -26,7 +26,6 @@
 import com.google.devtools.build.lib.analysis.AnalysisOptions;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
-import com.google.devtools.build.lib.buildtool.BuildRequestOptions;
 import com.google.devtools.build.lib.clock.Clock;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.QuiescingExecutors;
@@ -112,7 +111,7 @@ public class CommandEnvironment {
   private final BuildResultListener buildResultListener;
   private final CommandLinePathFactory commandLinePathFactory;
 
-  private final boolean mergedAnalysisAndExecution;
+  private boolean mergedAnalysisAndExecution;
 
   private OutputService outputService;
   private String workspaceName;
@@ -284,49 +283,6 @@ public void exit(AbruptExitException exception) {
 
     this.commandLinePathFactory =
         CommandLinePathFactory.create(runtime.getFileSystem(), directories);
-
-    this.mergedAnalysisAndExecution =
-        determineIfRunningWithMergedAnalysisAndExecution(
-            options, command.name(), packageLocator, warnings);
-  }
-
-  private static boolean determineIfRunningWithMergedAnalysisAndExecution(
-      OptionsParsingResult options,
-      String commandName,
-      @Nullable PathPackageLocator packageLocator,
-      List<String> warnings) {
-    BuildRequestOptions buildRequestOptions = options.getOptions(BuildRequestOptions.class);
-    // --nobuild means no execution will be carried out, hence it doesn't make sense to interleave
-    // analysis and execution in that case and --experimental_merged_skyframe_analysis_execution
-    // should be ignored.
-    // Aquery and Cquery implicitly set --nobuild, so there's no need to have a warning here: it
-    // makes no different from the users' perspective.
-    if (buildRequestOptions != null
-        && buildRequestOptions.mergedSkyframeAnalysisExecutionDoNotUseDirectly
-        && !buildRequestOptions.performExecutionPhase
-        && !(commandName.equals("aquery") || commandName.equals("cquery"))) {
-      warnings.add(
-          "--experimental_merged_skyframe_analysis_execution is incompatible with --nobuild and"
-              + " will be ignored.");
-    }
-
-    boolean valueFromFlags =
-        buildRequestOptions != null
-            && buildRequestOptions.mergedSkyframeAnalysisExecutionDoNotUseDirectly
-            && buildRequestOptions.performExecutionPhase;
-    boolean havingMultiPackagePath =
-        packageLocator != null && packageLocator.getPathEntries().size() > 1;
-
-    // TODO(b/246324830): Skymeld and multi-package_path are incompatible.
-    if (valueFromFlags && havingMultiPackagePath) {
-      warnings.add(
-          "--experimental_merged_skyframe_analysis_execution is "
-              + "incompatible with multiple --package_path ("
-              + packageLocator.getPathEntries()
-              + ") and its value will be ignored.");
-    }
-
-    return valueFromFlags && !havingMultiPackagePath;
   }
 
   private Path computeWorkingDirectory(CommonCommandOptions commandOptions)
@@ -481,6 +437,10 @@ public boolean withMergedAnalysisAndExecution() {
     return mergedAnalysisAndExecution;
   }
 
+  public void setMergedAnalysisAndExecution(boolean value) {
+    mergedAnalysisAndExecution = value;
+  }
+
   private Map<String, String> filterClientEnv(Set<String> vars) {
     Map<String, String> result = new TreeMap<>();
     for (String var : vars) {

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -2379,6 +2379,17 @@ java_library(
     ],
 )
 
+java_library(
+    name = "skymeld_module",
+    srcs = ["SkymeldModule.java"],
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib:build-request-options",
+        "//src/main/java/com/google/devtools/build/lib:runtime",
+        "//src/main/java/com/google/devtools/build/lib/events",
+        "//src/main/java/com/google/devtools/build/lib/pkgcache",
+    ],
+)
+
 java_library(
     name = "bzl_load_cycle_reporter",
     srcs = ["BzlLoadCycleReporter.java"],

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
@@ -0,0 +1,71 @@
+// 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.skyframe;
+
+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.runtime.BlazeModule;
+import com.google.devtools.build.lib.runtime.CommandEnvironment;
+
+/** Module to handle various Skymeld checks. */
+public class SkymeldModule extends BlazeModule {
+  @Override
+  public void beforeCommand(CommandEnvironment env) {
+    env.setMergedAnalysisAndExecution(determineIfMergingAnalysisExecution(env));
+  }
+
+  boolean determineIfMergingAnalysisExecution(CommandEnvironment env) {
+    String commandName = env.getCommandName();
+    PathPackageLocator packageLocator = env.getPackageLocator();
+    BuildRequestOptions buildRequestOptions =
+        env.getOptions().getOptions(BuildRequestOptions.class);
+    // --nobuild means no execution will be carried out, hence it doesn't make sense to interleave
+    // analysis and execution in that case and --experimental_merged_skyframe_analysis_execution
+    // should be ignored.
+    // Aquery and Cquery implicitly set --nobuild, so there's no need to have a warning here: it
+    // makes no different from the users' perspective.
+    if (buildRequestOptions != null
+        && buildRequestOptions.mergedSkyframeAnalysisExecutionDoNotUseDirectly
+        && !buildRequestOptions.performExecutionPhase
+        && !(commandName.equals("aquery") || commandName.equals("cquery"))) {
+      env.getReporter()
+          .handle(
+              Event.warn(
+                  "--experimental_merged_skyframe_analysis_execution is incompatible with --nobuild"
+                      + " and will be ignored."));
+    }
+    boolean plainValueFromFlag = getPlainValueFromFlag(buildRequestOptions);
+    boolean havingMultiPackagePath =
+        packageLocator != null && packageLocator.getPathEntries().size() > 1;
+    // TODO(b/246324830): Skymeld and multi-package_path are incompatible.
+    if (plainValueFromFlag && havingMultiPackagePath) {
+      env.getReporter()
+          .handle(
+              Event.warn(
+                  "--experimental_merged_skyframe_analysis_execution is "
+                      + "incompatible with multiple --package_path ("
+                      + packageLocator.getPathEntries()
+                      + ") and its value will be ignored."));
+    }
+    return plainValueFromFlag
+        && buildRequestOptions.performExecutionPhase
+        && !havingMultiPackagePath;
+  }
+
+  static boolean getPlainValueFromFlag(BuildRequestOptions buildRequestOptions) {
+    return buildRequestOptions != null
+        && buildRequestOptions.mergedSkyframeAnalysisExecutionDoNotUseDirectly;
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldBuildIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldBuildIntegrationTest.java
@@ -141,11 +141,9 @@ public void nobuild_warning() throws Exception {
     BuildResult result = buildTarget("//foo:foo");
 
     assertThat(result.getSuccess()).isTrue();
-    assertThat(
-            runtimeWrapper.workspaceSetupWarningsContains(
-                "--experimental_merged_skyframe_analysis_execution is incompatible with --nobuild"
-                    + " and will be ignored"))
-        .isTrue();
+    events.assertContainsWarning(
+        "--experimental_merged_skyframe_analysis_execution is incompatible with --nobuild"
+            + " and will be ignored");
   }
 
   @Test
@@ -488,13 +486,10 @@ public void multiplePackagePath_ignoreSkymeldWithWarning() throws Exception {
 
     assertThat(buildResult.getSuccess()).isTrue();
 
-    assertThat(
-            runtimeWrapper.workspaceSetupWarningsContains(
-                "--experimental_merged_skyframe_analysis_execution is incompatible with multiple"
-                    + " --package_path"))
-        .isTrue();
-    assertThat(runtimeWrapper.workspaceSetupWarningsContains("and its value will be ignored."))
-        .isTrue();
+    events.assertContainsWarning(
+        "--experimental_merged_skyframe_analysis_execution is incompatible with multiple"
+            + " --package_path");
+    events.assertContainsWarning("and its value will be ignored.");
   }
 
   // Regression test for b/245919888.

diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/util/BUILD b/src/test/java/com/google/devtools/build/lib/buildtool/util/BUILD
@@ -82,6 +82,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
         "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:skymeld_module",
         "//src/main/java/com/google/devtools/build/lib/standalone",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/util:command",

diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java b/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java
@@ -259,15 +259,6 @@ public void addOptionsClass(Class<? extends OptionsBase> optionsClass) {
     additionalOptionsClasses.add(optionsClass);
   }
 
-  public boolean workspaceSetupWarningsContains(String message) {
-    for (String warning : workspaceSetupWarnings) {
-      if (warning.contains(message)) {
-        return true;
-      }
-    }
-    return false;
-  }
-
   void finalizeBuildResult(@SuppressWarnings("unused") BuildResult request) {}
 
   /**

diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java b/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java
@@ -105,6 +105,7 @@
 import com.google.devtools.build.lib.skyframe.PrecomputedValue;
 import com.google.devtools.build.lib.skyframe.PrecomputedValue.Injected;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
+import com.google.devtools.build.lib.skyframe.SkymeldModule;
 import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
 import com.google.devtools.build.lib.standalone.StandaloneModule;
 import com.google.devtools.build.lib.testutil.TestConstants;
@@ -561,6 +562,7 @@ protected BlazeRuntime.Builder getRuntimeBuilder() throws Exception {
             .addBlazeModule(new BuildIntegrationTestCommandsModule())
             .addBlazeModule(new OutputFilteringModule())
             .addBlazeModule(connectivityModule)
+            .addBlazeModule(new SkymeldModule())
             .addBlazeModule(getMockBazelRepositoryModule());
     getSpawnModules().forEach(builder::addBlazeModule);
     builder