diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index d9f7cf73d80e2c..4106f10951330a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -748,7 +748,6 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( tsgm.get(), pathResolver, skyframeActionExecutor.getExecRoot().asFragment(), - PathFragment.create(directories.getRelativeOutputPath()), expandedFilesets); // We only need to check the action cache if we haven't done it on a previous run. @@ -808,15 +807,12 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( "Input discovery returned null but no more deps were requested by %s", action); } - switch (addDiscoveredInputs(state, env, action)) { - case VALUES_MISSING: - return null; - case NO_DISCOVERED_DATA: - break; - case DISCOVERED_DATA: - metadataHandler = metadataHandler.transformAfterInputDiscovery(new OutputStore()); - metadataHandler.prepareForActionExecution(); + + addDiscoveredInputs(state, env, action); + if (env.valuesMissing()) { + return null; } + // When discover inputs completes, post an event with the duration values. env.getListener() .post( @@ -866,17 +862,9 @@ public void run( state.getExpandedFilesets(); if (action.discoversInputs()) { state.discoveredInputs = action.getInputs(); - switch (addDiscoveredInputs(state, env, action)) { - case VALUES_MISSING: - return; - case NO_DISCOVERED_DATA: - break; - case DISCOVERED_DATA: - // We are in the interesting case of an action that discovered its inputs during - // execution, and found some new ones, but the new ones were already present in the - // graph. We must therefore cache the metadata for those new ones. - metadataHandler = - metadataHandler.transformAfterInputDiscovery(metadataHandler.getOutputStore()); + addDiscoveredInputs(state, env, action); + if (env.valuesMissing()) { + return; } } checkState(!env.valuesMissing(), action); @@ -895,13 +883,7 @@ public void run( } } - private enum DiscoveredState { - VALUES_MISSING, - NO_DISCOVERED_DATA, - DISCOVERED_DATA - } - - private DiscoveredState addDiscoveredInputs( + private void addDiscoveredInputs( InputDiscoveryState state, Environment env, Action actionForError) throws InterruptedException, ActionExecutionException { // TODO(janakr): This code's assumptions are wrong in the face of Starlark actions with unused @@ -919,7 +901,7 @@ private DiscoveredState addDiscoveredInputs( } if (unknownDiscoveredInputs.isEmpty()) { - return DiscoveredState.NO_DISCOVERED_DATA; + return; } SkyframeLookupResult nonMandatoryDiscovered = @@ -980,7 +962,6 @@ private DiscoveredState addDiscoveredInputs( "unknown metadata for " + input.getExecPathString() + ": " + retrievedMetadata); } } - return env.valuesMissing() ? DiscoveredState.VALUES_MISSING : DiscoveredState.DISCOVERED_DATA; } @Nullable diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index 43d4f99d8a7aa1..d439ebf64db163 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -85,11 +85,6 @@ final class ActionMetadataHandler implements InputMetadataProvider, OutputMetada /** * Creates a new metadata handler. - * - *

If the handler is for use during input discovery, calling {@link #getMetadata} with an - * artifact which is neither in {@code inputArtifactData} nor {@code outputs} is tolerated and - * will return {@code null}. To subsequently transform the handler for regular action execution - * (where such a call is not permitted), use {@link #transformAfterInputDiscovery}. */ static ActionMetadataHandler create( ActionInputMap inputArtifactData, @@ -100,7 +95,6 @@ static ActionMetadataHandler create( TimestampGranularityMonitor tsgm, ArtifactPathResolver artifactPathResolver, PathFragment execRoot, - PathFragment derivedPathPrefix, Map> expandedFilesets) { return new ActionMetadataHandler( inputArtifactData, @@ -111,7 +105,6 @@ static ActionMetadataHandler create( tsgm, artifactPathResolver, execRoot, - derivedPathPrefix, createFilesetMapping(expandedFilesets, execRoot), new OutputStore()); } @@ -128,7 +121,6 @@ static ActionMetadataHandler create( private final TimestampGranularityMonitor tsgm; private final ArtifactPathResolver artifactPathResolver; private final PathFragment execRoot; - private final PathFragment derivedPathPrefix; private final AtomicBoolean executionMode = new AtomicBoolean(false); private final OutputStore store; @@ -142,7 +134,6 @@ private ActionMetadataHandler( TimestampGranularityMonitor tsgm, ArtifactPathResolver artifactPathResolver, PathFragment execRoot, - PathFragment derivedPathPrefix, ImmutableMap filesetMapping, OutputStore store) { this.inputArtifactData = checkNotNull(inputArtifactData); @@ -153,37 +144,10 @@ private ActionMetadataHandler( this.tsgm = checkNotNull(tsgm); this.artifactPathResolver = checkNotNull(artifactPathResolver); this.execRoot = checkNotNull(execRoot); - this.derivedPathPrefix = checkNotNull(derivedPathPrefix); this.filesetMapping = checkNotNull(filesetMapping); this.store = checkNotNull(store); } - /** - * Returns a new handler mostly identical to this one, except uses the given {@code store} and - * does not permit {@link #getMetadata} to be called with an artifact which is neither in inputs - * nor outputs. - * - *

The returned handler will be in the mode for action cache checking. To prepare it for action - * execution, call {@link #prepareForActionExecution}. - * - *

This method is designed to be called after input discovery when a fresh handler is needed - * but all of the parameters in {@link #create} would be the same as the original handler. - */ - ActionMetadataHandler transformAfterInputDiscovery(OutputStore store) { - return new ActionMetadataHandler( - inputArtifactData, - archivedTreeArtifactsEnabled, - outputPermissions, - outputs, - xattrProvider, - tsgm, - artifactPathResolver, - execRoot, - derivedPathPrefix, - filesetMapping, - store); - } - /** * If {@code value} represents an existing file, returns it as is, otherwise throws {@link * FileNotFoundException}. diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java index 7b8be7e77751b4..1b574cb1536ee2 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java @@ -103,7 +103,6 @@ private ActionMetadataHandler createHandler( tsgm, ArtifactPathResolver.IDENTITY, execRoot.asFragment(), - derivedPathPrefix, /* expandedFilesets= */ ImmutableMap.of()); } @@ -420,7 +419,6 @@ public void getMetadataFromFilesetMapping() throws Exception { tsgm, ArtifactPathResolver.IDENTITY, execRoot.asFragment(), - derivedPathPrefix, expandedFilesets); // Only the regular FileArtifactValue should have its metadata stored. @@ -530,7 +528,6 @@ public void outputArtifactNotPreviouslyInjectedInExecutionMode_writablePermissio tsgm, ArtifactPathResolver.IDENTITY, execRoot.asFragment(), - derivedPathPrefix, /* expandedFilesets= */ ImmutableMap.of()); handler.prepareForActionExecution(); @@ -581,37 +578,6 @@ public void outputTreeArtifactNotPreviouslyInjectedInExecutionMode() throws Exce 0555); } - @Test - public void transformAfterInputDiscovery() throws Exception { - Artifact known = - ActionsTestUtil.createArtifactWithRootRelativePath( - outputRoot, PathFragment.create("known")); - Artifact unknown = - ActionsTestUtil.createArtifactWithRootRelativePath( - outputRoot, PathFragment.create("unknown")); - ActionMetadataHandler handler = - createHandler( - new ActionInputMap(0), /* forInputDiscovery= *//* outputs= */ ImmutableSet.of(known)); - - // Unknown artifact returns null during input discovery. - assertThat(handler.getInputMetadata(unknown)).isNull(); - - OutputStore newStore = new OutputStore(); - FileArtifactValue knownMetadata = - RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1, /* expireAtEpochMilli= */ -1); - newStore.putArtifactData(known, knownMetadata); - ActionMetadataHandler newHandler = handler.transformAfterInputDiscovery(newStore); - assertThat(newHandler.getOutputStore()).isNotEqualTo(handler.getOutputStore()); - assertThat(newHandler.getOutputStore()).isEqualTo(newStore); - - assertThat(newHandler.getOutputMetadata(known)).isEqualTo(knownMetadata); - // Unknown artifact throws outside of input discovery. - // assertThrows(IllegalStateException.class, () -> newHandler.getInputMetadata(unknown)); - // We can transform it again. - assertThat(newHandler.transformAfterInputDiscovery(new OutputStore())).isNotNull(); - assertThat(chmodCalls).isEmpty(); - } - @Test public void getTreeArtifactChildren_noData_returnsEmptySet() { SpecialArtifact treeArtifact =