[Skymeld] Add build-info.txt and build-changelist.txt These 2 were missing from Skymeld. This however creates a race condition with the CriticalPathComputer: sometimes Action(build-info.txt) starts executing before the CriticalPathComputer is initialized and finishes after, hence leaving the CriticalPathComputer in an illegal state. To avoid this race condition, we need to move the generation of workspace artifacts to where they belong: after some execution has started. PiperOrigin-RevId: 467928812 Change-Id: Ie671971199d287d3329dc14fa98665a18fd2b7af
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 17dadf6..9a35e7c 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
@@ -543,7 +543,7 @@ ImmutableSet.Builder<Artifact> artifactsToBuild = ImmutableSet.builder(); // build-info and build-changelist. - Collection<Artifact> buildInfoArtifacts = + ImmutableList<Artifact> buildInfoArtifacts = skyframeExecutor.getWorkspaceStatusArtifacts(eventHandler); Preconditions.checkState(buildInfoArtifacts.size() == 2, buildInfoArtifacts);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 6eacd7e..ccb5d42 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -2251,6 +2251,7 @@ ":top_level_conflict_exception", ":transitive_target_key", "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data", "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java index 4e18a9b..6f6289f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java
@@ -101,7 +101,7 @@ * with the appropriate CompletionFunctions. This is the bridge between the conceptual analysis & * execution phases. * - * <p>TODO(b/199053098): implement build-info, build-changelist, coverage & exception handling. + * <p>TODO(b/240944910): implement coverage. */ @Nullable @Override
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 9c1c7ce..ef42ac3 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
@@ -34,6 +34,7 @@ import com.google.devtools.build.lib.actions.ActionLookupKey; import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.AnalysisGraphStatsEvent; +import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactFactory; import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException; import com.google.devtools.build.lib.actions.BuildFailedException; @@ -615,6 +616,9 @@ largestTopLevelKeySetCheckedForConflicts = newKeys; } + ImmutableList<Artifact> workspaceStatusArtifacts = + skyframeExecutor.getWorkspaceStatusArtifacts(eventHandler); + ImmutableSet<BuildDriverKey> buildDriverCTKeys = ctKeys.stream() .map( @@ -657,6 +661,7 @@ try { resourceManager.resetResourceUsage(); + EvaluationResult<SkyValue> workspaceStatusResult; try (SilentCloseable c = Profiler.instance().profile("skyframeExecutor.evaluateBuildDriverKeys")) { // Will be disabled later by the AnalysisOperationWatcher upon conclusion of analysis. @@ -675,11 +680,35 @@ // We unconditionally reset the states here instead of in #analysisFinishedCallback since // in case of --nokeep_going & analysis error, the analysis phase is never finished. skyframeExecutor.resetIncrementalArtifactConflictFindingStates(); + + // Unconditionally create build-info.txt and build-changelist.txt. + // No action conflict expected here. + // This evaluation involves action executions, and hence has to be done after the first + // SomeExecutionStartedEvent (which is posted from BuildDriverFunction). The most + // straightforward solution is to perform this here, after + // SkyframeExecutor#evaluateBuildDriverKeys. + workspaceStatusResult = + skyframeExecutor.evaluateSkyKeys( + eventHandler, Artifact.keys(workspaceStatusArtifacts), keepGoing); + if (workspaceStatusResult.hasError()) { + detailedExitCodes.add( + SkyframeErrorProcessor.processErrors( + workspaceStatusResult, + configurationLookupSupplier, + skyframeExecutor.getCyclesReporter(), + eventHandler, + keepGoing, + eventBus, + bugReporter, + /*includeExecutionPhase=*/ true) + .executionDetailedExitCode()); + } } // The exclusive tests whose analysis succeeded i.e. those that can be run. ImmutableSet<ConfiguredTarget> exclusiveTestsToRun = getExclusiveTests(evaluationResult); - boolean continueWithExclusiveTests = !evaluationResult.hasError() || keepGoing; + boolean continueWithExclusiveTests = + (!evaluationResult.hasError() && !workspaceStatusResult.hasError()) || keepGoing; if (continueWithExclusiveTests && !exclusiveTestsToRun.isEmpty()) { // Run exclusive tests sequentially.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java index 442a3e7..0f1c1db 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java
@@ -27,6 +27,7 @@ import com.google.common.flogger.GoogleLogger; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionExecutionException; +import com.google.devtools.build.lib.actions.ActionLookupData; import com.google.devtools.build.lib.actions.ActionLookupKey; import com.google.devtools.build.lib.actions.BuildFailedException; import com.google.devtools.build.lib.actions.InputFileErrorException; @@ -431,6 +432,17 @@ /*loadingRootCauses=*/ ImmutableSet.of()); } + // Only possible with actions generating build-info.txt and build-changelist.txt. + if (errorKey.argument() instanceof ActionLookupData) { + return IndividualErrorProcessingResult.create( + /*actionConflicts=*/ ImmutableMap.of(), + getExecutionDetailedExitCodeFromCause(result, cause), + /*rootCauses=*/ cause instanceof ActionExecutionException + ? ((ActionExecutionException) cause).getRootCauses() + : NestedSetBuilder.emptySet(Order.STABLE_ORDER), + /*loadingRootCauses=*/ ImmutableSet.of()); + } + Preconditions.checkState( errorKey.argument() instanceof ConfiguredTargetKey, "expected '%s' to be a TopLevelAspectsKey or ConfiguredTargetKey",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index bfa9950..6c9b43f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1215,18 +1215,21 @@ } /** Returns the build-info.txt and build-changelist.txt artifacts. */ - public Collection<Artifact> getWorkspaceStatusArtifacts(ExtendedEventHandler eventHandler) + public ImmutableList<Artifact> getWorkspaceStatusArtifacts(ExtendedEventHandler eventHandler) throws InterruptedException { - // Should already be present, unless the user didn't request any targets for analysis. - EvaluationResult<WorkspaceStatusValue> result = - evaluate( - ImmutableList.of(WorkspaceStatusValue.BUILD_INFO_KEY), - /*keepGoing=*/ true, - /*numThreads=*/ 1, - eventHandler); - WorkspaceStatusValue value = - Preconditions.checkNotNull(result.get(WorkspaceStatusValue.BUILD_INFO_KEY)); - return ImmutableList.of(value.getStableArtifact(), value.getVolatileArtifact()); + try (SilentCloseable c = + Profiler.instance().profile("SkyframeExecutor.getWorkspaceStatusArtifact")) { + // Should already be present, unless the user didn't request any targets for analysis. + EvaluationResult<WorkspaceStatusValue> result = + evaluate( + ImmutableList.of(WorkspaceStatusValue.BUILD_INFO_KEY), + /*keepGoing=*/ true, + /*numThreads=*/ 1, + eventHandler); + WorkspaceStatusValue value = + Preconditions.checkNotNull(result.get(WorkspaceStatusValue.BUILD_INFO_KEY)); + return ImmutableList.of(value.getStableArtifact(), value.getVolatileArtifact()); + } } @VisibleForTesting
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/BUILD b/src/test/java/com/google/devtools/build/lib/buildtool/BUILD index 56170b9..b71de7a 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/BUILD +++ b/src/test/java/com/google/devtools/build/lib/buildtool/BUILD
@@ -647,6 +647,7 @@ "//src/main/java/com/google/devtools/build/lib/skyframe:top_level_status_events", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/test/java/com/google/devtools/build/lib/buildtool/util", + "//src/test/java/com/google/devtools/build/lib/testutil:TestConstants", "//third_party:guava", "//third_party:junit4", "//third_party:truth",
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 index 27dd29f..4f3780f 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldBuildIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldBuildIntegrationTest.java
@@ -15,6 +15,7 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.testutil.TestConstants.WORKSPACE_NAME; import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableSet; @@ -162,6 +163,38 @@ assertThat(analysisEventsSubscriber.getTopLevelEntityAnalysisConcludedEvents()).hasSize(2); assertSingleAnalysisPhaseCompleteEventWithLabels("//foo:foo", "//foo:bar"); + + assertThat(directories.getOutputPath(WORKSPACE_NAME).getRelative("build-info.txt").isFile()) + .isTrue(); + assertThat( + directories.getOutputPath(WORKSPACE_NAME).getRelative("build-changelist.txt").isFile()) + .isTrue(); + } + + @Test + public void multiTargetNullIncrementalBuild_success() throws Exception { + writeMyRuleBzl(); + write( + "foo/BUILD", + "load('//foo:my_rule.bzl', 'my_rule')", + "my_rule(name = 'bar', srcs = ['bar.in'])", + "my_rule(name = 'foo', srcs = ['foo.in'])"); + write("foo/foo.in"); + write("foo/bar.in"); + + // First build, ignored. + buildTarget("//foo:foo", "//foo:bar"); + BuildResult result = buildTarget("//foo:foo", "//foo:bar"); + + assertThat(result.getSuccess()).isTrue(); + assertSingleOutputBuilt("//foo:foo"); + assertSingleOutputBuilt("//foo:bar"); + + assertThat(directories.getOutputPath(WORKSPACE_NAME).getRelative("build-info.txt").isFile()) + .isTrue(); + assertThat( + directories.getOutputPath(WORKSPACE_NAME).getRelative("build-changelist.txt").isFile()) + .isTrue(); } @Test