From 2cd5c3af92441ce473bf47cd30dadeadcc26dfe1 Mon Sep 17 00:00:00 2001
From: Matt Mackay <mattem@gmail.com>
Date: Mon, 31 Jul 2023 13:26:50 -0700
Subject: [PATCH] feat: add option to exit early if analysis cache is discarded

Adds `--allow_analysis_cache_discard` option that allows the build to exit early if the analysis cache would have been discarded.

The flag name is of course open to bikeshedding etc.

fixes #16804

Closes #16805.

PiperOrigin-RevId: 552575951
Change-Id: Ia336eb3a5b7d7e41665fd0e0adf3edc03ed50f18
---
 .../build/lib/analysis/AnalysisOptions.java   | 11 +++++++++
 .../build/lib/analysis/BuildView.java         |  5 ++--
 .../devtools/build/lib/buildeventstream/BUILD |  1 +
 .../BuildCompletingEvent.java                 | 24 +++++++++++++++----
 .../proto/build_event_stream.proto            |  3 +++
 .../buildevent/BuildCompleteEvent.java        |  2 +-
 .../build/lib/skyframe/SkyframeBuildView.java | 18 ++++++++++++--
 src/main/protobuf/failure_details.proto       |  2 ++
 .../lib/analysis/AnalysisCachingTest.java     | 13 ++++++++++
 .../google/devtools/build/lib/analysis/BUILD  |  1 +
 .../analysis/util/BuildViewForTesting.java    | 12 ++++++++--
 11 files changed, 81 insertions(+), 11 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java
index 95256a0549fd24..6810d1f0525816 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisOptions.java
@@ -39,6 +39,17 @@ public class AnalysisOptions extends OptionsBase {
   )
   public boolean discardAnalysisCache;
 
+  @Option(
+      name = "allow_analysis_cache_discard",
+      defaultValue = "true",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.EAGERNESS_TO_EXIT},
+      help =
+          "If discarding the analysis cache due to a change in the build system, setting this"
+              + " option to false will cause bazel to exit, rather than continuing with the build."
+              + " This option has no effect when 'discard_analysis_cache' is also set.")
+  public boolean allowAnalysisCacheDiscards;
+
   @Option(
     name = "max_config_changes_to_show",
     defaultValue = "3",
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index c4208b0a9325c0..14a955d0b205a5 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -256,7 +256,8 @@ public AnalysisResult update(
     }
 
     skyframeBuildView.setConfigurations(
-        eventHandler, configurations, viewOptions.maxConfigChangesToShow);
+        eventHandler, configurations, viewOptions.maxConfigChangesToShow,
+        viewOptions.allowAnalysisCacheDiscards);
 
     eventBus.post(
         new MakeEnvironmentEvent(configurations.getTargetConfiguration().getMakeEnvironment()));
@@ -535,7 +536,7 @@ private AnalysisResult createResult(
       TopLevelTargetsAndConfigsResult topLevelTargetsWithConfigs,
       boolean includeExecutionPhase)
       throws InterruptedException {
-    Set<Label> testsToRun = loadingResult.getTestsToRunLabels();
+    ImmutableSet<Label> testsToRun = loadingResult.getTestsToRunLabels();
     Set<ConfiguredTarget> configuredTargets =
         Sets.newLinkedHashSet(skyframeAnalysisResult.getConfiguredTargets());
     ImmutableMap<AspectKey, ConfiguredAspect> aspects = skyframeAnalysisResult.getAspects();
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/BUILD b/src/main/java/com/google/devtools/build/lib/buildeventstream/BUILD
index 2ac74e452d4699..eb17c389d0e4fe 100644
--- a/src/main/java/com/google/devtools/build/lib/buildeventstream/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/BUILD
@@ -24,6 +24,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
         "//src/main/java/com/google/devtools/build/lib/util",
+        "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
         "//src/main/java/com/google/devtools/build/lib/util:exit_code",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildCompletingEvent.java b/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildCompletingEvent.java
index 8390012989bfc8..b028b4f831fd1d 100644
--- a/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildCompletingEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildCompletingEvent.java
@@ -16,9 +16,11 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId;
+import com.google.devtools.build.lib.util.DetailedExitCode;
 import com.google.devtools.build.lib.util.ExitCode;
 import com.google.protobuf.util.Timestamps;
 import java.util.Collection;
+import javax.annotation.Nullable;
 
 /**
  * Class all events completing a build inherit from.
@@ -27,6 +29,7 @@
  * However, subclasses do not have to implement anything.
  */
 public abstract class BuildCompletingEvent implements BuildEvent {
+  @Nullable private final DetailedExitCode detailedExitCode;
   private final ExitCode exitCode;
   private final long finishTimeMillis;
 
@@ -34,11 +37,20 @@ public abstract class BuildCompletingEvent implements BuildEvent {
 
   public BuildCompletingEvent(
       ExitCode exitCode, long finishTimeMillis, Collection<BuildEventId> children) {
+    this.detailedExitCode = null;
     this.exitCode = exitCode;
     this.finishTimeMillis = finishTimeMillis;
     this.children = children;
   }
 
+  public BuildCompletingEvent(
+      DetailedExitCode detailedExitCode, long finishTimeMillis, Collection<BuildEventId> children) {
+    this.detailedExitCode = detailedExitCode;
+    this.exitCode = detailedExitCode.getExitCode();
+    this.finishTimeMillis = finishTimeMillis;
+    this.children = children;
+  }
+
   public BuildCompletingEvent(ExitCode exitCode, long finishTimeMillis) {
     this(exitCode, finishTimeMillis, ImmutableList.of());
   }
@@ -65,13 +77,17 @@ public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext convert
             .setCode(exitCode.getNumericExitCode())
             .build();
 
-    BuildEventStreamProtos.BuildFinished finished =
+    BuildEventStreamProtos.BuildFinished.Builder finished =
         BuildEventStreamProtos.BuildFinished.newBuilder()
             .setOverallSuccess(ExitCode.SUCCESS.equals(exitCode))
             .setExitCode(protoExitCode)
             .setFinishTime(Timestamps.fromMillis(finishTimeMillis))
-            .setFinishTimeMillis(finishTimeMillis)
-            .build();
-    return GenericBuildEvent.protoChaining(this).setFinished(finished).build();
+            .setFinishTimeMillis(finishTimeMillis);
+
+    if (detailedExitCode != null && detailedExitCode.getFailureDetail() != null) {
+      finished.setFailureDetail(detailedExitCode.getFailureDetail());
+    }
+
+    return GenericBuildEvent.protoChaining(this).setFinished(finished.build()).build();
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto
index c0b862ad8e4f42..3514f89b46a6c5 100644
--- a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto
+++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto
@@ -826,6 +826,9 @@ message BuildFinished {
   google.protobuf.Timestamp finish_time = 5;
 
   AnomalyReport anomaly_report = 4 [deprecated = true];
+
+  // Only populated if success = false, and sometimes not even then.
+  failure_details.FailureDetail failure_detail = 6;
 }
 
 message BuildMetrics {
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/BuildCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/BuildCompleteEvent.java
index 2b77fa5b0d9b4d..ff631281bda0f1 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/BuildCompleteEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/BuildCompleteEvent.java
@@ -32,7 +32,7 @@ public final class BuildCompleteEvent extends BuildCompletingEvent {
 
   /** Construct the BuildCompleteEvent. */
   public BuildCompleteEvent(BuildResult result, Collection<BuildEventId> children) {
-    super(result.getDetailedExitCode().getExitCode(), result.getStopTime(), children);
+    super(result.getDetailedExitCode(), result.getStopTime(), children);
     this.result = checkNotNull(result);
   }
 
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
index 484114c3a8f39f..7a818cc71f7edd 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -65,6 +65,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiff;
 import com.google.devtools.build.lib.analysis.config.ConfigConditions;
+import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.config.StarlarkTransitionCache;
 import com.google.devtools.build.lib.analysis.test.AnalysisFailurePropagationException;
 import com.google.devtools.build.lib.bugreport.BugReport;
@@ -85,6 +86,7 @@
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.SilentCloseable;
+import com.google.devtools.build.lib.server.FailureDetails;
 import com.google.devtools.build.lib.skyframe.ArtifactConflictFinder.ConflictException;
 import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
 import com.google.devtools.build.lib.skyframe.AspectKeyCreator.TopLevelAspectsKey;
@@ -275,7 +277,9 @@ private ImmutableSet<OptionDefinition> getNativeCacheInvalidatingDifferences(
   public void setConfigurations(
       EventHandler eventHandler,
       BuildConfigurationCollection configurations,
-      int maxDifferencesToShow) {
+      int maxDifferencesToShow,
+      boolean allowAnalysisCacheDiscards)
+      throws InvalidConfigurationException {
     if (skyframeAnalysisWasDiscarded) {
       eventHandler.handle(
           Event.info(
@@ -285,7 +289,17 @@ public void setConfigurations(
     } else {
       String diff = describeConfigurationDifference(configurations, maxDifferencesToShow);
       if (diff != null) {
-        eventHandler.handle(Event.info(diff + ", discarding analysis cache."));
+        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)."));
         // Note that clearing the analysis cache is currently required for correctness. It is also
         // helpful to save memory.
         //
diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto
index 788dfc5bc1be39..ebce5159a339ab 100644
--- a/src/main/protobuf/failure_details.proto
+++ b/src/main/protobuf/failure_details.proto
@@ -603,6 +603,7 @@ message BuildConfiguration {
     // possibilities in Bazel, so we go with the more straightforward
     // command-line error exit code 2.
     INVALID_OUTPUT_DIRECTORY_MNEMONIC = 11 [(metadata) = { exit_code: 2 }];
+    CONFIGURATION_DISCARDED_ANALYSIS_CACHE = 12 [(metadata) = { exit_code: 2 }];
   }
 
   Code code = 1;
@@ -1190,6 +1191,7 @@ message Analysis {
     CONFIGURED_VALUE_CREATION_FAILED = 18 [(metadata) = { exit_code: 1 }];
     INCOMPATIBLE_TARGET_REQUESTED = 19 [(metadata) = { exit_code: 1 }];
     ANALYSIS_FAILURE_PROPAGATION_FAILED = 20 [(metadata) = { exit_code: 1 }];
+    ANALYSIS_CACHE_DISCARDED = 21 [(metadata) = { exit_code: 1 }];
   }
 
   Code code = 1;
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
index 28681ea5a5d682..e347df4ba28499 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
 import com.google.devtools.build.lib.analysis.config.Fragment;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
+import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.config.RequiresOptions;
 import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
@@ -1160,4 +1161,16 @@ public void cacheClearMessageAfterDiscardAnalysisCacheBuildWithRelevantOptionCha
     assertDoesNotContainEvent("Build option");
     assertContainsEvent("discarding analysis cache");
   }
+
+  @Test
+  public void throwsIfAnalysisCacheIsDiscardedWhenOptionSet() throws Exception {
+    setupDiffResetTesting();
+    scratch.file("test/BUILD", "load(':lib.bzl', 'normal_lib')", "normal_lib(name='top')");
+    useConfiguration("--definitely_relevant=old");
+    update("//test:top");
+    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();
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
index 00e004beaaa340..386e851c6d2b49 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
@@ -81,6 +81,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_registry",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/per_label_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/run_under",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/run_under_converter",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
index f6a39211c82b20..5b7fb2079a6cab 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
@@ -228,8 +228,16 @@ public AnalysisResult update(
   /** Sets the configurations. Not thread-safe. */
   public void setConfigurationsForTesting(
       EventHandler eventHandler, BuildConfigurationCollection configurations) {
-    skyframeBuildView.setConfigurations(
-        eventHandler, configurations, /* maxDifferencesToShow */ -1);
+    try {
+      skyframeBuildView.setConfigurations(
+          eventHandler, configurations, /* 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 ArtifactFactory getArtifactFactory() {