diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java index 7b7f1635d6d4a6..2ccebf18777104 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java @@ -177,6 +177,7 @@ public void buildTargets(BuildRequest request, BuildResult result, TargetValidat return; } + env.getSkyframeExecutor().setMergedSkyframeAnalysisExecution(false); AnalysisResult analysisResult = AnalysisPhaseRunner.execute(env, request, buildOptions, validator); @@ -309,7 +310,7 @@ private void buildTargetsWithMergedAnalysisExecution( throws InterruptedException, TargetParsingException, LoadingFailedException, AbruptExitException, ViewCreationFailedException, BuildFailedException, TestExecException, InvalidConfigurationException, RepositoryMappingResolutionException { - + env.getSkyframeExecutor().setMergedSkyframeAnalysisExecution(true); // Target pattern evaluation. TargetPatternPhaseValue loadingResult; Profiler.instance().markPhase(ProfilePhase.TARGET_PATTERN_EVAL); 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 329942587ca85d..d7b790c512b6b4 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 @@ -228,6 +228,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.Optional; import java.util.Set; import java.util.UUID; @@ -243,6 +244,7 @@ import java.util.function.Consumer; import java.util.function.Supplier; import javax.annotation.Nullable; +import javax.annotation.concurrent.GuardedBy; import net.starlark.java.eval.StarlarkSemantics; /** @@ -379,6 +381,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory, Configur private RemoteOutputsMode lastRemoteOutputsMode; private Boolean lastRemoteCacheEnabled; + private boolean mergedSkyframeAnalysisExecution = false; + // Reset after each build. private IncrementalArtifactConflictFinder incrementalArtifactConflictFinder; // Reset after each build. @@ -1406,6 +1410,11 @@ public SkyframeBuildView getSkyframeBuildView() { return skyframeBuildView; } + /** Sets whether this build is done with --experimental_merged_skyframe_analysis_execution. */ + public void setMergedSkyframeAnalysisExecution(boolean mergedSkyframeAnalysisExecution) { + this.mergedSkyframeAnalysisExecution = mergedSkyframeAnalysisExecution; + } + /** Sets the eventBus to use for posting events. */ public void setEventBus(@Nullable EventBus eventBus) { this.eventBus.set(eventBus); @@ -3049,6 +3058,15 @@ protected class SkyframeProgressReceiver implements EvaluationProgressReceiver { /** This receiver is only needed for execution, so it is null otherwise. */ @Nullable EvaluationProgressReceiver executionProgressReceiver = null; + /** + * As the ActionLookupValues are marked done in the graph, put it in the map. This map will be + * "swapped out" for a new one each time it's requested via {@link + * #getBatchedActionLookupValuesForConflictChecking()} + */ + @GuardedBy("this") + private Map batchedActionLookupValuesForConflictChecking = + Maps.newConcurrentMap(); + @Override public void invalidated(SkyKey skyKey, InvalidationState state) { if (ignoreInvalidations) { @@ -3109,6 +3127,15 @@ public void evaluated( } } + if (mergedSkyframeAnalysisExecution + && !tracksStateForIncrementality() + && skyKey instanceof ActionLookupKey + && newValue != null) { + synchronized (this) { + batchedActionLookupValuesForConflictChecking.put(skyKey, newValue); + } + } + if (EvaluationState.BUILT.equals(state)) { skyKeyStateReceiver.evaluated(skyKey); } @@ -3123,6 +3150,23 @@ public void evaluated( skyKey, newValue, newError, evaluationSuccessState, state); } } + + /** + * Returns the SkyKey -> SkyValue map of done ActionLookupValues since the last time this method + * was called. Replaces {@link #batchedActionLookupValuesForConflictChecking} with a new empty + * map. + * + *

This is only used in skymeld mode AND when we don't keep the incremental state. + */ + public ImmutableMap getBatchedActionLookupValuesForConflictChecking() { + Preconditions.checkState(mergedSkyframeAnalysisExecution && !tracksStateForIncrementality()); + synchronized (this) { + ImmutableMap result = + ImmutableMap.copyOf(batchedActionLookupValuesForConflictChecking); + batchedActionLookupValuesForConflictChecking = Maps.newConcurrentMap(); + return result; + } + } } public final ExecutionFinishedEvent createExecutionFinishedEvent() { @@ -3174,12 +3218,23 @@ private ActionLookupValuesCollectionResult collectTransitiveActionLookupValuesOf try (SilentCloseable c = Profiler.instance().profile("SkyframeExecutor.collectTransitiveActionLookupValuesOfKey")) { ActionLookupValuesTraversal result = new ActionLookupValuesTraversal(); - Map foundTransitiveActionLookupEntities = - incrementalTransitiveActionLookupKeysCollector.collect(key); - foundTransitiveActionLookupEntities.forEach(result::accumulate); + if (tracksStateForIncrementality()) { + Map foundTransitiveActionLookupEntities = + incrementalTransitiveActionLookupKeysCollector.collect(key); + foundTransitiveActionLookupEntities.forEach(result::accumulate); + return ActionLookupValuesCollectionResult.create( + result.getActionLookupValueShards(), + ImmutableSet.copyOf(foundTransitiveActionLookupEntities.keySet())); + } + // No graph edges available when there's no incrementality. We get the ALVs collected + // since the last time this method was called. + ImmutableMap batchOfActionLookupValues = + progressReceiver.getBatchedActionLookupValuesForConflictChecking(); + for (Entry entry : batchOfActionLookupValues.entrySet()) { + result.accumulate((ActionLookupKey) entry.getKey(), entry.getValue()); + } return ActionLookupValuesCollectionResult.create( - result.getActionLookupValueShards(), - ImmutableSet.copyOf(foundTransitiveActionLookupEntities.keySet())); + result.getActionLookupValueShards(), batchOfActionLookupValues.keySet()); } } diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java index 3112cf56f248b6..6ef88286c18490 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java @@ -734,4 +734,22 @@ public void dependencyHasConflict_keepGoing_bothTopLevelTargetsFail( assertThat(eventListener.failedTargetNames) .containsExactly("//foo:top_level_a", "//foo:top_level_b"); } + + @Test + public void conflict_noTrackIncrementalState_detected( + @TestParameter boolean mergedAnalysisExecution) throws Exception { + addOptions("--experimental_merged_skyframe_analysis_execution=" + mergedAnalysisExecution); + addOptions("--notrack_incremental_state"); + writeConflictBzl(); + write( + "foo/BUILD", + "load('//foo:conflict.bzl', 'my_rule')", + "my_rule(name = 'first')", + "my_rule(name = 'second')"); + + assertThrows( + ViewCreationFailedException.class, () -> buildTarget("//foo:first", "//foo:second")); + events.assertContainsError( + "file 'foo/conflict_output' is generated by these conflicting actions:"); + } }