Make --noallow_discard_analysis_cache work when used twice in a seque… (

#23653)

…nce.

This didn't work at HEAD because the check there compared the flags in
the new configuration with the flags of the old configuration, but
overwrote the old configuration with the new one, so running a command
that discarded the analysis cache was allowed the second time.

The fix is not as trivial as moving checking the value before setting
the new configuration because creating the new configuration entails a
Skyframe evaluation, which is a waste if we end up erroring out. So the
check is now done by checking the build options of the old configuration
and the new build options, which doesn't require creating the new
configuration first.

Fixes #23491.

RELNOTES: None.
PiperOrigin-RevId: 675555324
Change-Id: Ib402b699ed9ba9f04542896a40ff1180351471db

src/main/java/com/google/devtools/build/lib/analysis/BuildView.java

@@ -243,22 +243,26 @@ public AnalysisResult update(
 
     // Prepare the analysis phase
     BuildConfigurationValue topLevelConfig;
+    boolean shouldDiscardAnalysisCache;
+
     // Configuration creation.
     // TODO(gregce): Consider dropping this phase and passing on-the-fly target / exec configs as
     // needed. This requires cleaning up the invalidation in SkyframeBuildView.setConfigurations.
     try (SilentCloseable c = Profiler.instance().profile("createConfigurations")) {
+      shouldDiscardAnalysisCache =
+          skyframeBuildView.shouldDiscardAnalysisCache(
+              eventHandler,
+              targetOptions,
+              viewOptions.maxConfigChangesToShow,
+              viewOptions.allowAnalysisCacheDiscards);
       topLevelConfig = skyframeExecutor.createConfiguration(eventHandler, targetOptions, keepGoing);
       eventBus.post(new ConfigRequestedEvent(topLevelConfig, /* parentChecksum= */ null));
     }
     if (buildConfigurationsCreatedCallback != null) {
       buildConfigurationsCreatedCallback.run(topLevelConfig);
     }
 
-    skyframeBuildView.setConfiguration(
-        eventHandler,
-        topLevelConfig,
-        viewOptions.maxConfigChangesToShow,
-        viewOptions.allowAnalysisCacheDiscards);
+    skyframeBuildView.setConfiguration(topLevelConfig, targetOptions, shouldDiscardAnalysisCache);
 
     eventBus.post(new MakeEnvironmentEvent(topLevelConfig.getMakeEnvironment()));
     eventBus.post(topLevelConfig.toBuildEvent());

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java

@@ -151,7 +151,9 @@ public final class SkyframeBuildView {
 
   private final ConfiguredRuleClassProvider ruleClassProvider;
 
-  private BuildConfigurationValue configuration;
+  // Null until the build configuration is set.
+  @Nullable private BuildConfigurationValue configuration;
+  @Nullable private BuildOptions originalConfigurationOptions;
 
   /**
    * If the last build was executed with {@code Options#discard_analysis_cache} and we are not
@@ -206,16 +208,8 @@ public TotalAndConfiguredTargetOnlyMetric getEvaluatedActionCounts() {
    */
   @Nullable
   private String describeConfigurationDifference(
-      BuildConfigurationValue configuration, int maxDifferencesToShow) {
-    if (this.configuration == null) {
-      return null;
-    }
-    if (configuration.equals(this.configuration)) {
-      return null;
-    }
-
-    OptionsDiff diff =
-        BuildOptions.diff(this.configuration.getOptions(), configuration.getOptions());
+      BuildOptions oldOptions, BuildOptions newOptions, int maxDifferencesToShow) {
+    OptionsDiff diff = BuildOptions.diff(oldOptions, newOptions);
 
     ImmutableSet<OptionDefinition> nativeCacheInvalidatingDifferences =
         getNativeCacheInvalidatingDifferences(configuration, diff);
@@ -277,49 +271,78 @@ private ImmutableSet<OptionDefinition> getNativeCacheInvalidatingDifferences(
         .collect(toImmutableSet());
   }
 
-  /** Sets the configuration. Not thread-safe. DO NOT CALL except from tests! */
-  @VisibleForTesting
-  public void setConfiguration(
+  /**
+   * Returns whether the analysis results from previous invocations should be discarded or report an
+   * error if it should be, but it's disallowed.
+   *
+   * <p>This should happen when the top-level configuration has changed or if the previous
+   * invocation decided that this should happen. Either way, this method also emits a message
+   * informing the user about this decision.
+   */
+  public boolean shouldDiscardAnalysisCache(
       EventHandler eventHandler,
-      BuildConfigurationValue configuration,
+      BuildOptions newOptions,
       int maxDifferencesToShow,
       boolean allowAnalysisCacheDiscards)
       throws InvalidConfigurationException {
+    if (this.configuration == null) {
+      return false;
+    }
+
     if (skyframeAnalysisWasDiscarded) {
+      logger.atInfo().log("Discarding analysis cache because the previous invocation told us to");
       eventHandler.handle(
           Event.warn(
               "--discard_analysis_cache was used in the previous build, "
                   + "discarding analysis cache."));
-      logger.atInfo().log("Discarding analysis cache because the previous invocation told us to");
-      skyframeExecutor.handleAnalysisInvalidatingChange();
-    } else {
-      String diff = describeConfigurationDifference(configuration, maxDifferencesToShow);
-      if (diff != null) {
-        if (!allowAnalysisCacheDiscards) {
-          String message = String.format("%s, analysis cache would have been discarded.", diff);
-          throw new InvalidConfigurationException(
-              message,
-              FailureDetails.BuildConfiguration.Code.CONFIGURATION_DISCARDED_ANALYSIS_CACHE);
-        }
-        eventHandler.handle(
-            Event.warn(
-                diff
-                    + ", discarding analysis cache (this can be expensive, see"
-                    + " https://bazel.build/advanced/performance/iteration-speed)."));
-        logger.atInfo().log(
-            "Discarding analysis cache because the build configuration changed: %s", diff);
-        // Note that clearing the analysis cache is currently required for correctness. It is also
-        // helpful to save memory.
-        //
-        // If we had more memory, fixing the correctness issue (see also b/144932999) would allow us
-        // to not invalidate the cache, leading to potentially better performance on incremental
-        // builds.
-        skyframeExecutor.handleAnalysisInvalidatingChange();
+      return true;
+    }
+
+    String diff =
+        describeConfigurationDifference(
+            originalConfigurationOptions, newOptions, maxDifferencesToShow);
+
+    if (diff != null) {
+      if (!allowAnalysisCacheDiscards) {
+        String message = String.format("%s, analysis cache would have been discarded.", diff);
+        throw new InvalidConfigurationException(
+            message, FailureDetails.BuildConfiguration.Code.CONFIGURATION_DISCARDED_ANALYSIS_CACHE);
       }
+      eventHandler.handle(
+          Event.warn(
+              diff
+                  + ", discarding analysis cache (this can be expensive, see"
+                  + " https://bazel.build/advanced/performance/iteration-speed)."));
+      logger.atInfo().log(
+          "Discarding analysis cache because the build configuration changed: %s", diff);
+      return true;
+    }
+
+    return false;
+  }
+
+  /** Sets the configuration. Not thread-safe. */
+  @VisibleForTesting
+  public void setConfiguration(
+      BuildConfigurationValue configuration,
+      BuildOptions originalOptions,
+      boolean discardAnalysisCache) {
+    if (discardAnalysisCache) {
+      // Note that clearing the analysis cache is currently required for correctness. It is also
+      // helpful to save memory.
+      //
+      // If we had more memory, fixing the correctness issue (see also b/144932999) would allow us
+      // to not invalidate the cache, leading to potentially better performance on incremental
+      // builds.
+      this.configuration = configuration;
+      this.originalConfigurationOptions = originalOptions;
+      skyframeExecutor.handleAnalysisInvalidatingChange();
+    } else if (this.configuration == null) {
+      this.configuration = configuration;
+      this.originalConfigurationOptions = originalOptions;
     }
 
     skyframeAnalysisWasDiscarded = false;
-    this.configuration = configuration;
     skyframeExecutor.setTopLevelConfiguration(configuration);
   }
 
@@ -1333,6 +1356,7 @@ void clearLegacyData() {
    */
   public void reset() {
     configuration = null;
+    originalConfigurationOptions = null;
     skyframeAnalysisWasDiscarded = false;
     clearLegacyData();
   }

src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java

@@ -1170,10 +1170,30 @@ public void throwsIfAnalysisCacheIsDiscardedWhenOptionSet() throws Exception {
     setupDiffResetTesting();
     scratch.file("test/BUILD", "load(':lib.bzl', 'normal_lib')", "normal_lib(name='top')");
     useConfiguration("--definitely_relevant=old");
+
+    // Set up the analysis cache
     update("//test:top");
-    useConfiguration("--noallow_analysis_cache_discard", "--definitely_relevant=new");
 
+    // Check if things work if the build options are not changed
+    useConfiguration("--noallow_analysis_cache_discard", "--definitely_relevant=old");
+    update("//test:top");
+
+    // Check if an error is raised when the build options are changed. Do it twice because
+    // had already had a bug that the second invocation erroneously worked. See
+    // https://github.com/bazelbuild/bazel/issues/23491 .
+    useConfiguration("--noallow_analysis_cache_discard", "--definitely_relevant=new");
     Throwable t = assertThrows(InvalidConfigurationException.class, () -> update("//test:top"));
     assertThat(t.getMessage().contains("analysis cache would have been discarded")).isTrue();
+
+    t = assertThrows(InvalidConfigurationException.class, () -> update("//test:top"));
+    assertThat(t.getMessage()).contains("analysis cache would have been discarded");
+
+    // Check if going back to the original configuration works.
+    useConfiguration("--noallow_analysis_cache_discard", "--definitely_relevant=old");
+    update("//test:top");
+
+    // Now check if removing --noallow_analysis_cache_discard in fact allows discarding the cache.
+    useConfiguration("--definitely_relevant=new");
+    update("//test:top");
   }
 }

src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java

@@ -62,7 +62,6 @@
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadCompatible;
 import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
@@ -256,20 +255,11 @@ public AnalysisResult update(
   }
 
   /** Sets the configuration. Not thread-safe. */
-  public void setConfigurationForTesting(
-      EventHandler eventHandler, BuildConfigurationValue configuration) {
-    try {
-      skyframeBuildView.setConfiguration(
-          eventHandler,
-          configuration,
-          /* maxDifferencesToShow= */ -1, /* allowAnalysisCacheDiscards */
-          true);
-    } catch (InvalidConfigurationException e) {
-      throw new UnsupportedOperationException(
-          "InvalidConfigurationException was thrown and caught during a test, "
-              + "this case is not yet handled",
-          e);
-    }
+  public void setConfigurationForTesting(BuildConfigurationValue configuration) {
+    skyframeBuildView.setConfiguration(
+        configuration,
+        configuration.getOptions(),
+        true);
   }
 
   public ArtifactFactory getArtifactFactory() {

src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java

@@ -660,7 +660,7 @@ protected final void createBuildView() {
     skyframeExecutor.handleAnalysisInvalidatingChange();
 
     view = new BuildViewForTesting(directories, ruleClassProvider, skyframeExecutor, null);
-    view.setConfigurationForTesting(event -> {}, targetConfig);
+    view.setConfigurationForTesting(targetConfig);
 
     view.setArtifactRoots(new PackageRootsNoSymlinkCreation(Root.fromPath(rootDirectory)));
   }