Skip to content

Commit

Permalink
throw InvalidConfigurationException and add FailureDetail to `Bui…
Browse files Browse the repository at this point in the history 
…ldCompletingEvent`
  • Loading branch information
@mattem
mattem committed Mar 25, 2023
1 parent cd08c0a commit bb30682
Show file tree
Hide file tree
Showing 16 changed files with 101 additions and 93 deletions.
27 changes: 21 additions & 6 deletions src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,20 @@
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.server.FailureDetails.TargetPatterns;
import com.google.devtools.build.lib.server.FailureDetails.TargetPatterns.Code;
import com.google.devtools.build.lib.skyframe.*;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.TopLevelAspectsKey;
import com.google.devtools.build.lib.skyframe.BuildConfigurationKey;
import com.google.devtools.build.lib.skyframe.BuildResultListener;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import com.google.devtools.build.lib.skyframe.CoverageReportValue;
import com.google.devtools.build.lib.skyframe.RepositoryMappingValue.RepositoryMappingResolutionException;
import com.google.devtools.build.lib.skyframe.SkyframeAnalysisAndExecutionResult;
import com.google.devtools.build.lib.skyframe.SkyframeAnalysisResult;
import com.google.devtools.build.lib.skyframe.SkyframeBuildView;
import com.google.devtools.build.lib.skyframe.SkyframeBuildView.BuildDriverKeyTestContext;
import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
import com.google.devtools.build.lib.skyframe.TargetPatternPhaseValue;
import com.google.devtools.build.lib.util.AbruptExitException;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.RegexFilter;
Expand Down Expand Up @@ -213,9 +223,14 @@ public AnalysisResult update(
@Nullable ResourceManager resourceManager,
@Nullable BuildResultListener buildResultListener,
@Nullable ExecutionSetup executionSetupCallback,
@Nullable BuildConfigurationsCreated buildConfigurationsCreatedCallback)
throws ViewCreationFailedException, InvalidConfigurationException, InterruptedException,
BuildFailedException, TestExecException, AbruptExitException, AnalysisCacheDiscardedException {
@Nullable BuildConfigurationsCreated buildConfigurationsCreatedCallback,
@Nullable BuildDriverKeyTestContext buildDriverKeyTestContext)
throws ViewCreationFailedException,
InvalidConfigurationException,
InterruptedException,
BuildFailedException,
TestExecException,
AbruptExitException {
logger.atInfo().log("Starting analysis");
pollInterruptedStatus();

Expand Down Expand Up @@ -253,8 +268,8 @@ public AnalysisResult update(
skyframeExecutor);
}

skyframeBuildView.setConfigurations(
eventHandler, configurations, viewOptions.maxConfigChangesToShow, viewOptions.failOnAnalysisCacheDiscard);
skyframeBuildView.setConfiguration(
eventHandler, configuration, viewOptions.maxConfigChangesToShow, viewOptions.failOnAnalysisCacheDiscard);

eventBus.post(new MakeEnvironmentEvent(configuration.getMakeEnvironment()));
eventBus.post(configuration.toBuildEvent());
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/buildeventstream/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,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",
Expand Down
26 changes: 22 additions & 4 deletions src/main/java/com/google/devtools/build/lib/buildeventstream/BuildCompletingEvent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +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 javax.annotation.Nullable;
import java.util.Collection;

/**
Expand All @@ -27,18 +30,29 @@
* 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;

private final Collection<BuildEventId> children;

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 comment has been minimized.

Sign in to view

Copy link
@michaeledgar

michaeledgar Apr 4, 2023

Contributor

Please mark the DetailedExitCode argument @Nullable, matching the field.
this.detailedExitCode = detailedExitCode;
this.exitCode = detailedExitCode.getExitCode();
this.finishTimeMillis = finishTimeMillis;
this.children = children;
}

public BuildCompletingEvent(ExitCode exitCode, long finishTimeMillis) {
this(exitCode, finishTimeMillis, ImmutableList.of());
}
Expand All @@ -65,13 +79,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();
}
}
3 changes: 3 additions & 0 deletions src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,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 {
Expand Down
33 changes: 23 additions & 10 deletions src/main/java/com/google/devtools/build/lib/buildtool/AnalysisAndExecutionPhaseRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,12 @@
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.server.FailureDetails.BuildConfiguration.Code;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.skyframe.*;
import com.google.devtools.build.lib.skyframe.BuildInfoCollectionFunction;
import com.google.devtools.build.lib.skyframe.BuildResultListener;
import com.google.devtools.build.lib.skyframe.PrecomputedValue;
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.TopLevelTargetAnalyzedEvent;
import com.google.devtools.build.lib.util.AbruptExitException;
import com.google.devtools.build.lib.util.DetailedExitCode;
Expand Down Expand Up @@ -75,11 +79,15 @@ static AnalysisAndExecutionResult execute(
BuildOptions buildOptions,
TargetPatternPhaseValue loadingResult,
ExecutionSetup executionSetupCallback,
BuildConfigurationsCreated buildConfigurationCreatedCallback)
throws BuildFailedException, InterruptedException, ViewCreationFailedException,
TargetParsingException, LoadingFailedException, AbruptExitException,
InvalidConfigurationException, TestExecException, RepositoryMappingResolutionException,
AnalysisCacheDiscardedException {
BuildConfigurationsCreated buildConfigurationCreatedCallback,
BuildDriverKeyTestContext buildDriverKeyTestContext)
throws BuildFailedException,
InterruptedException,
ViewCreationFailedException,
AbruptExitException,
InvalidConfigurationException,
TestExecException,
RepositoryMappingResolutionException {

// Compute the heuristic instrumentation filter if needed.
if (request.needsInstrumentationFilter()) {
Expand Down Expand Up @@ -199,10 +207,15 @@ private static AnalysisAndExecutionResult runAnalysisAndExecutionPhase(
TargetPatternPhaseValue loadingResult,
BuildOptions targetOptions,
ExecutionSetup executionSetupCallback,
BuildConfigurationsCreated buildConfigurationCreatedCallback)
throws InterruptedException, InvalidConfigurationException, ViewCreationFailedException,
BuildFailedException, TestExecException, RepositoryMappingResolutionException,
AbruptExitException, AnalysisCacheDiscardedException {
BuildConfigurationsCreated buildConfigurationCreatedCallback,
BuildDriverKeyTestContext buildDriverKeyTestContext)
throws InterruptedException,
InvalidConfigurationException,
ViewCreationFailedException,
BuildFailedException,
TestExecException,
RepositoryMappingResolutionException,
AbruptExitException {
env.getReporter().handle(Event.progress("Loading complete. Analyzing..."));

ImmutableSet<Label> explicitTargetPatterns =
Expand Down
9 changes: 6 additions & 3 deletions src/main/java/com/google/devtools/build/lib/buildtool/AnalysisPhaseRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,12 @@
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.server.FailureDetails.BuildConfiguration.Code;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.skyframe.*;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
import com.google.devtools.build.lib.skyframe.BuildConfigurationKey;
import com.google.devtools.build.lib.skyframe.BuildInfoCollectionFunction;
import com.google.devtools.build.lib.skyframe.PrecomputedValue;
import com.google.devtools.build.lib.skyframe.RepositoryMappingValue.RepositoryMappingResolutionException;
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;
Expand Down Expand Up @@ -84,7 +87,7 @@ public static AnalysisResult execute(
TargetValidator validator)
throws BuildFailedException, InterruptedException, ViewCreationFailedException,
TargetParsingException, LoadingFailedException, AbruptExitException,
InvalidConfigurationException, RepositoryMappingResolutionException, AnalysisCacheDiscardedException {
InvalidConfigurationException, RepositoryMappingResolutionException {

// Target pattern evaluation.
TargetPatternPhaseValue loadingResult;
Expand Down Expand Up @@ -216,7 +219,7 @@ private static AnalysisResult runAnalysisPhase(
TargetPatternPhaseValue loadingResult,
BuildOptions targetOptions)
throws InterruptedException, InvalidConfigurationException,
RepositoryMappingResolutionException, ViewCreationFailedException, AnalysisCacheDiscardedException {
RepositoryMappingResolutionException, ViewCreationFailedException {
Stopwatch timer = Stopwatch.createStarted();
env.getReporter().handle(Event.progress("Loading complete. Analyzing..."));

Expand Down
9 changes: 4 additions & 5 deletions src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.server.FailureDetails.ActionQuery;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.skyframe.AnalysisCacheDiscardedException;
import com.google.devtools.build.lib.skyframe.BuildResultListener;
import com.google.devtools.build.lib.skyframe.RepositoryMappingValue.RepositoryMappingResolutionException;
import com.google.devtools.build.lib.skyframe.SequencedSkyframeExecutor;
import com.google.devtools.build.lib.skyframe.SkyframeBuildView.BuildDriverKeyTestContext;
Expand Down Expand Up @@ -151,7 +151,7 @@ public void buildTargets(BuildRequest request, BuildResult result, TargetValidat
throws BuildFailedException, InterruptedException, ViewCreationFailedException,
TargetParsingException, LoadingFailedException, AbruptExitException,
InvalidConfigurationException, TestExecException, ExitException,
PostExecutionActionGraphDumpException, RepositoryMappingResolutionException, AnalysisCacheDiscardedException {
PostExecutionActionGraphDumpException, RepositoryMappingResolutionException {
try (SilentCloseable c = Profiler.instance().profile("validateOptions")) {
validateOptions(request);
}
Expand Down Expand Up @@ -311,8 +311,7 @@ private void buildTargetsWithMergedAnalysisExecution(
BuildOptions buildOptions)
throws InterruptedException, TargetParsingException, LoadingFailedException,
AbruptExitException, ViewCreationFailedException, BuildFailedException, TestExecException,
InvalidConfigurationException, RepositoryMappingResolutionException, AnalysisCacheDiscardedException {

InvalidConfigurationException, RepositoryMappingResolutionException {
// Target pattern evaluation.
TargetPatternPhaseValue loadingResult;
Profiler.instance().markPhase(ProfilePhase.TARGET_PATTERN_EVAL);
Expand Down Expand Up @@ -573,7 +572,7 @@ public BuildResult processRequest(
} else {
result.setCatastrophe();
}
} catch (TargetParsingException | LoadingFailedException | AnalysisCacheDiscardedException e) {
} catch (TargetParsingException | LoadingFailedException e) {
detailedExitCode = e.getDetailedExitCode();
reportExceptionError(e);
} catch (RepositoryMappingResolutionException e) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/buildtool/buildevent/BuildCompleteEvent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
33 changes: 0 additions & 33 deletions src/main/java/com/google/devtools/build/lib/skyframe/AnalysisCacheDiscardedException.java
View file

This file was deleted.

1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/skyframe/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ java_library(
"ActionLookupConflictFindingFunction.java",
"ActionLookupValuesTraversal.java",
"ActionOutputDirectoryHelper.java",
"AnalysisCacheDiscardedException.java",
"AspectCompletor.java",
"AspectFunction.java",
"BazelSkyframeExecutorConstants.java",
Expand Down
18 changes: 7 additions & 11 deletions src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,8 @@
import com.google.devtools.build.lib.analysis.ToolchainCollection;
import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
import com.google.devtools.build.lib.analysis.ViewCreationFailedException;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.*;

This comment has been minimized.

Sign in to view

Copy link
@michaeledgar

michaeledgar Apr 4, 2023

Contributor

Tiny nitpick, this file has the wildcard import -- could you revert these lines?
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.StarlarkTransitionCache;
import com.google.devtools.build.lib.analysis.test.AnalysisFailurePropagationException;
import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.bugreport.BugReporter;
Expand All @@ -84,6 +81,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;
Expand Down Expand Up @@ -269,11 +267,8 @@ private ImmutableSet<OptionDefinition> getNativeCacheInvalidatingDifferences(

/** Sets the configuration. Not thread-safe. DO NOT CALL except from tests! */
@VisibleForTesting
public void setConfigurations(
EventHandler eventHandler,
BuildConfigurationCollection configurations,
int maxDifferencesToShow,
boolean failOnAnalysisCacheDiscard) throws AnalysisCacheDiscardedException {
public void setConfiguration(
EventHandler eventHandler, BuildConfigurationValue configuration, int maxDifferencesToShow, boolean failOnAnalysisCacheDiscard) throws InvalidConfigurationException {
if (skyframeAnalysisWasDiscarded) {
eventHandler.handle(
Event.info(
Expand All @@ -284,9 +279,10 @@ public void setConfigurations(
String diff = describeConfigurationDifference(configuration, maxDifferencesToShow);
if (diff != null) {
if (failOnAnalysisCacheDiscard) {
throw new AnalysisCacheDiscardedException(diff);
String message = String.format("%s, analysis cache would have been discarded.", diff);
throw new InvalidConfigurationException(

This comment has been minimized.

Sign in to view

Copy link
@michaeledgar

michaeledgar Apr 4, 2023

Contributor

This is great! One thing we should add here, before we throw, is to post an EventBus event for this particular early-exit: eventBus.post(new RefusedToDiscardAnalysisCacheEvent());. You'll need to update the two callers of #setConfiguration(EventHandler, BuildConfigurationValue, int, boolean):

  1. The call in BuildView#update can be updated easily using the EventBus provided as a parameter, and
  2. The call in BuildViewForTesting#setConfigurationForTesting you can just pass a new EventBus().

Model your new Event after NoAnalyzeEvent:

public class RefusedToDiscardAnalysisCacheEvent implements ExtendedEventHandler.Postable {}

A handler in BuildEventStreamer then will note that the build is being aborted early for the newly-declared reason:

  @Subscribe
  public void refusedToDiscardCache(RefusedToDiscardAnalysisCacheEvent event) {
    addAbortReason(AbortReason.REFUSED_TO_DISCARD_CACHE);
  }

Finally, add the new AbortReason to build_event_stream.proto:

message Aborted {
  enum AbortReason {
...
    // Build tool failed-fast to avoid discarding one or more caches.
    REFUSED_TO_DISCARD_CACHE = 12;
...
  }
}

This will ensure that all targets have an event posted specifically attributing their incomplete result to the new AbortReason.
message, FailureDetails.BuildConfiguration.Code.CONFIGURATION_DISCARDED_ANALYSIS_CACHE);
}

eventHandler.handle(Event.info(diff + ", discarding analysis cache."));
// Note that clearing the analysis cache is currently required for correctness. It is also
// helpful to save memory.
Expand Down
3 changes: 0 additions & 3 deletions src/main/java/com/google/devtools/build/lib/util/ExitCode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,6 @@ public final class ExitCode {
ExitCode.create(45, "PERSISTENT_BUILD_EVENT_SERVICE_UPLOAD_ERROR");
public static final ExitCode EXTERNAL_DEPS_ERROR = ExitCode.create(48, "EXTERNAL_DEPS_ERROR");

public static final ExitCode ANALYSIS_CACHE_DISCARD_FAILURE =
ExitCode.create(49, "ANALYSIS_CACHE_DISCARD_FAILURE");

/**
* Creates and returns an ExitCode. Requires a unique exit code number.
*
Expand Down
1 change: 1 addition & 0 deletions src/main/protobuf/failure_details.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,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;
Expand Down
Loading

1 comment on commit bb30682

@michaeledgar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks great! There are a few tiny style issues to fix, but substantively this change is just about ready. I've added one comment describing how to add a new AbortReason for this case. By adding that, the BEP will mark all the per-ConfiguredTarget events with your custom AbortReason. Without adding that, no AbortReason will be present at all.

Please sign in to comment.