diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java index 8dfe7478077222..312e0c325fe424 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java @@ -414,7 +414,8 @@ public Token getTokenIfNeedToExecute( EventHandler handler, MetadataHandler metadataHandler, ArtifactExpander artifactExpander, - Map remoteDefaultPlatformProperties) + Map remoteDefaultPlatformProperties, + boolean isRemoteCacheEnabled) throws InterruptedException { // TODO(bazel-team): (2010) For RunfilesAction/SymlinkAction and similar actions that // produce only symlinks we should not check whether inputs are valid at all - all that matters @@ -456,8 +457,11 @@ public Token getTokenIfNeedToExecute( } ActionCache.Entry entry = getCacheEntry(action); CachedOutputMetadata cachedOutputMetadata = null; - // load remote metadata from action cache - if (entry != null && !entry.isCorrupted() && cacheConfig.storeOutputMetadata()) { + if (entry != null + && !entry.isCorrupted() + && cacheConfig.storeOutputMetadata() + && isRemoteCacheEnabled) { + // load remote metadata from action cache cachedOutputMetadata = loadCachedOutputMetadata(action, entry, metadataHandler); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index 7a0b0df245cde8..c42189cb96cdab 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -599,6 +599,7 @@ Token checkActionCache( remoteOptions != null ? remoteOptions.getRemoteDefaultExecProperties() : ImmutableSortedMap.of(); + boolean isRemoteCacheEnabled = remoteOptions != null && remoteOptions.isRemoteCacheEnabled(); token = actionCacheChecker.getTokenIfNeedToExecute( action, @@ -609,7 +610,8 @@ Token checkActionCache( : null, metadataHandler, artifactExpander, - remoteDefaultProperties); + remoteDefaultProperties, + isRemoteCacheEnabled); } catch (UserExecException e) { throw e.toActionExecutionException(action); } diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java index 00e67ad0957a6a..536d0866e32373 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java @@ -183,7 +183,8 @@ private void runAction( /*handler=*/ null, metadataHandler, /*artifactExpander=*/ null, - platform); + platform, + /*isRemoteCacheEnabled=*/ true); if (token != null) { // Real action execution would happen here. ActionExecutionContext context = mock(ActionExecutionContext.class); @@ -440,7 +441,8 @@ public void testDeletedConstantMetadataOutputCausesReexecution() throws Exceptio /*handler=*/ null, new FakeMetadataHandler(), /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of())) + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), + /*isRemoteCacheEnabled=*/ true)) .isNotNull(); } @@ -557,7 +559,8 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception { /*handler=*/ null, metadataHandler, /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()); + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), + /*isRemoteCacheEnabled=*/ true); assertThat(output.getPath().exists()).isFalse(); assertThat(token).isNull(); @@ -567,6 +570,33 @@ public void saveOutputMetadata_remoteFileMetadataLoaded() throws Exception { assertThat(metadataHandler.getMetadata(output)).isEqualTo(createRemoteFileMetadata(content)); } + @Test + public void saveOutputMetadata_remoteOutputUnavailable_remoteFileMetadataNotLoaded() + throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + Artifact output = createArtifact(artifactRoot, "bin/dummy"); + String content = "content"; + Action action = new InjectOutputFileMetadataAction(output, createRemoteFileMetadata(content)); + MetadataHandler metadataHandler = new FakeMetadataHandler(); + + runAction(action); + Token token = + cacheChecker.getTokenIfNeedToExecute( + action, + /*resolvedCacheArtifacts=*/ null, + /*clientEnv=*/ ImmutableMap.of(), + /*handler=*/ null, + metadataHandler, + /*artifactExpander=*/ null, + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), + /*isRemoteCacheEnabled=*/ false); + + assertThat(output.getPath().exists()).isFalse(); + assertThat(token).isNotNull(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNull(); + } + @Test public void saveOutputMetadata_localMetadataIsSameAsRemoteMetadata_cached() throws Exception { cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); @@ -679,7 +709,8 @@ public void saveOutputMetadata_emptyTreeMetadata_notSaved() throws Exception { /*handler=*/ null, metadataHandler, /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()); + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), + /*isRemoteCacheEnabled=*/ true); assertThat(token).isNull(); assertThat(output.getPath().exists()).isFalse(); @@ -763,7 +794,8 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataLoaded() throws Ex /*handler=*/ null, metadataHandler, /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()); + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), + /*isRemoteCacheEnabled=*/ true); TreeArtifactValue expectedMetadata = createTreeMetadata(output, children, Optional.empty()); assertThat(token).isNull(); @@ -890,7 +922,8 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th /*handler=*/ null, metadataHandler, /*artifactExpander=*/ null, - /*remoteDefaultPlatformProperties=*/ ImmutableMap.of()); + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), + /*isRemoteCacheEnabled=*/ true); assertThat(token).isNull(); assertStatistics(1, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); @@ -905,8 +938,9 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th output, ImmutableMap.of( "file1", - FileArtifactValue.createForTesting(output.getPath().getRelative("file1")), - "file2", createRemoteFileMetadata("content2")), + FileArtifactValue.createForTesting(output.getPath().getRelative("file1")), + "file2", + createRemoteFileMetadata("content2")), Optional.empty())); } diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 04dfaa340f34c8..286e508c07c3f0 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3205,6 +3205,14 @@ EOF } function test_download_toplevel_when_turn_remote_cache_off() { + download_toplevel_when_turn_remote_cache_off +} + +function test_download_toplevel_when_turn_remote_cache_off_with_metadata() { + download_toplevel_when_turn_remote_cache_off --experimental_action_cache_store_output_metadata +} + +function download_toplevel_when_turn_remote_cache_off() { # Test that BwtB doesn't cause build failure if remote cache is disabled in a following build. # See https://github.com/bazelbuild/bazel/issues/13882. @@ -3239,6 +3247,7 @@ EOF bazel build \ --remote_cache=grpc://localhost:${worker_port} \ --remote_download_toplevel \ + "$@" \ //a:consumer >& $TEST_log || fail "Failed to download outputs" [[ -f bazel-bin/a/a.txt ]] || [[ -f bazel-bin/a/b.txt ]] \ && fail "Expected outputs of producer are not downloaded" @@ -3247,6 +3256,7 @@ EOF echo 'bar' > a/in.txt bazel build \ --remote_download_toplevel \ + "$@" \ //a:consumer >& $TEST_log || fail "Failed to build without remote cache" }