Download toplevel artifacts within spawn execution so that after action execution, the outputs are already downloaded. We have three types of downloads: 1. Download within spawn execution: the download is part of the spawn execution. Similar to normal build that, during the spawn execution, outputs are written to local file system. Metadata are not injected to skyframe, but instead, are calculated, e.g RegularFileArtifactValue. 2. For action that doesn't have spawn, e.g. SymlinkAction, the download is part of action finlization before injecting metadata into skyframe. 3. Download afterwards: the download is not part of the spawn execution. During spawn execution, metadata of outputs are injected into skyframe as RemoteFileArtifactValue. Dynamic execution, for example, can then request the downloads later. PiperOrigin-RevId: 529423585 Change-Id: If78663e22f0d36d621ffb35331c7ae08dc79fccd
diff --git a/src/main/java/com/google/devtools/build/lib/actions/RemoteArtifactChecker.java b/src/main/java/com/google/devtools/build/lib/actions/RemoteArtifactChecker.java index 196ce11..af13fed 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/RemoteArtifactChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/RemoteArtifactChecker.java
@@ -24,5 +24,5 @@ * Returns true if Bazel should trust (and not verify) build artifacts that were last seen * remotely and do not exist locally. */ - boolean shouldTrustRemoteArtifact(Artifact file, RemoteFileArtifactValue metadata); + boolean shouldTrustRemoteArtifact(ActionInput file, RemoteFileArtifactValue metadata); }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index b165c42..20d8a33 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java
@@ -76,7 +76,6 @@ private final AsyncTaskCache.NoResult<Path> downloadCache = AsyncTaskCache.NoResult.create(); private final TempPathGenerator tempPathGenerator; private final OutputPermissions outputPermissions; - protected final Set<Artifact> outputsAreInputs = Sets.newConcurrentHashSet(); protected final Path execRoot; protected final RemoteOutputChecker remoteOutputChecker; @@ -587,61 +586,34 @@ } } - @SuppressWarnings({"CheckReturnValue", "FutureReturnValueIgnored"}) public void finalizeAction(Action action, OutputMetadataStore outputMetadataStore) throws IOException, InterruptedException { - List<Artifact> inputsToDownload = new ArrayList<>(); List<Artifact> outputsToDownload = new ArrayList<>(); for (Artifact output : action.getOutputs()) { + if (outputMetadataStore.artifactOmitted(output)) { + continue; + } + var metadata = outputMetadataStore.getOutputMetadata(output); if (!metadata.isRemote()) { continue; } - if (outputsAreInputs.remove(output)) { - if (output.isTreeArtifact()) { - var children = outputMetadataStore.getTreeArtifactChildren((SpecialArtifact) output); - inputsToDownload.addAll(children); - } else { - inputsToDownload.add(output); - } - } else if (output.isTreeArtifact()) { + if (output.isTreeArtifact()) { var children = outputMetadataStore.getTreeArtifactChildren((SpecialArtifact) output); for (var file : children) { - if (remoteOutputChecker.shouldDownloadFile(file)) { + if (remoteOutputChecker.shouldDownloadFileAfterActionExecution(file)) { outputsToDownload.add(file); } } - } else if (remoteOutputChecker.shouldDownloadFile(output)) { - outputsToDownload.add(output); + } else { + if (remoteOutputChecker.shouldDownloadFileAfterActionExecution(output)) { + outputsToDownload.add(output); + } } } - if (!inputsToDownload.isEmpty()) { - // "input" here means "input to another action" (but an output of this one), so - // getOutputMetadata() is the right method to pass to prefetchFiles() - var future = - prefetchFiles(inputsToDownload, outputMetadataStore::getOutputMetadata, Priority.HIGH); - addCallback( - future, - new FutureCallback<Void>() { - @Override - public void onSuccess(Void unused) { - reporter.post(new InputsEagerlyPrefetched(inputsToDownload)); - } - - @Override - public void onFailure(Throwable throwable) { - reporter.handle( - Event.warn( - String.format( - "Failed to eagerly prefetch inputs: %s", throwable.getMessage()))); - } - }, - directExecutor()); - } - if (!outputsToDownload.isEmpty()) { var future = prefetchFiles(outputsToDownload, outputMetadataStore::getOutputMetadata, Priority.LOW); @@ -669,4 +641,8 @@ public ImmutableSet<ActionInput> getMissingActionInputs() { return ImmutableSet.copyOf(missingActionInputs); } + + public RemoteOutputChecker getRemoteOutputChecker() { + return remoteOutputChecker; + } }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java index 61d7224..df57599 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java
@@ -48,6 +48,7 @@ private TempPathGenerator tempPathGenerator; private RemoteExecutionService remoteExecutionService; @Nullable private RemoteActionInputFetcher actionInputFetcher; + @Nullable private final RemoteOutputChecker remoteOutputChecker; private RemoteActionContextProvider( Executor executor, @@ -56,7 +57,8 @@ @Nullable RemoteExecutionClient remoteExecutor, @Nullable ListeningScheduledExecutorService retryScheduler, DigestUtil digestUtil, - @Nullable Path logDir) { + @Nullable Path logDir, + @Nullable RemoteOutputChecker remoteOutputChecker) { this.executor = executor; this.env = Preconditions.checkNotNull(env, "env"); this.remoteCache = remoteCache; @@ -64,6 +66,7 @@ this.retryScheduler = retryScheduler; this.digestUtil = digestUtil; this.logDir = logDir; + this.remoteOutputChecker = remoteOutputChecker; } public static RemoteActionContextProvider createForPlaceholder( @@ -73,11 +76,12 @@ return new RemoteActionContextProvider( directExecutor(), env, - /*remoteCache=*/ null, - /*remoteExecutor=*/ null, + /* remoteCache= */ null, + /* remoteExecutor= */ null, retryScheduler, digestUtil, - /*logDir=*/ null); + /* logDir= */ null, + /* remoteOutputChecker= */ null); } public static RemoteActionContextProvider createForRemoteCaching( @@ -85,15 +89,17 @@ CommandEnvironment env, RemoteCache remoteCache, ListeningScheduledExecutorService retryScheduler, - DigestUtil digestUtil) { + DigestUtil digestUtil, + @Nullable RemoteOutputChecker remoteOutputChecker) { return new RemoteActionContextProvider( executor, env, remoteCache, - /*remoteExecutor=*/ null, + /* remoteExecutor= */ null, retryScheduler, digestUtil, - /*logDir=*/ null); + /* logDir= */ null, + remoteOutputChecker); } public static RemoteActionContextProvider createForRemoteExecution( @@ -103,9 +109,17 @@ RemoteExecutionClient remoteExecutor, ListeningScheduledExecutorService retryScheduler, DigestUtil digestUtil, - Path logDir) { + Path logDir, + @Nullable RemoteOutputChecker remoteOutputChecker) { return new RemoteActionContextProvider( - executor, env, remoteCache, remoteExecutor, retryScheduler, digestUtil, logDir); + executor, + env, + remoteCache, + remoteExecutor, + retryScheduler, + digestUtil, + logDir, + remoteOutputChecker); } private RemotePathResolver createRemotePathResolver() { @@ -155,7 +169,8 @@ remoteCache, remoteExecutor, tempPathGenerator, - captureCorruptedOutputsDir); + captureCorruptedOutputsDir, + remoteOutputChecker); env.getEventBus().register(remoteExecutionService); }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java index e3ab738..295c70e 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
@@ -133,20 +133,20 @@ remoteOutputTree.injectRemoteFile(path, digest, size, expireAtEpochMilli); } - void flush() throws IOException { + void flush() throws IOException, InterruptedException { checkNotNull(metadataInjector, "metadataInjector is null"); for (Map.Entry<PathFragment, Artifact> entry : outputMapping.entrySet()) { PathFragment path = execRoot.getRelative(entry.getKey()); Artifact output = entry.getValue(); - maybeInjectMetadataForSymlink(path, output); + maybeInjectMetadataForSymlinkOrDownload(path, output); } } /** * Inject metadata for non-symlink outputs that were materialized as a symlink to a remote - * artifact. + * artifact, and download the target artifact if required by the remote output mode. * * <p>If a non-symlink output is materialized as a symlink, the symlink has "copy" semantics, * i.e., the output metadata is identical to that of the symlink target. For these artifacts, we @@ -162,8 +162,8 @@ * fetching multiple copies when multiple symlinks to the same artifact are created in the * same build. */ - private void maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact output) - throws IOException { + private void maybeInjectMetadataForSymlinkOrDownload(PathFragment linkPath, Artifact output) + throws IOException, InterruptedException { if (output.isSymlink()) { return; } @@ -185,6 +185,25 @@ targetPath.isAbsolute(), "non-symlink artifact materialized as symlink must point to absolute path"); + if (inputFetcher.getRemoteOutputChecker().shouldDownloadOutputDuringActionExecution(output)) { + var targetActionInput = getInput(targetPath.relativeTo(execRoot).getPathString()); + if (targetActionInput != null) { + if (output.isTreeArtifact()) { + var metadata = getRemoteTreeMetadata(targetPath); + if (metadata != null) { + getFromFuture( + inputFetcher.prefetchFiles( + metadata.getChildren(), this::getInputMetadata, Priority.LOW)); + } + } else { + getFromFuture( + inputFetcher.prefetchFiles( + ImmutableList.of(targetActionInput), this::getInputMetadata, Priority.LOW)); + } + } + return; + } + if (output.isTreeArtifact()) { TreeArtifactValue metadata = getRemoteTreeMetadata(targetPath); if (metadata == null) {
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java index 8f8874e..0923429 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -178,6 +178,8 @@ private final AtomicBoolean shutdown = new AtomicBoolean(false); private final AtomicBoolean buildInterrupted = new AtomicBoolean(false); + @Nullable private final RemoteOutputChecker remoteOutputChecker; + public RemoteExecutionService( Executor executor, Reporter reporter, @@ -191,7 +193,8 @@ @Nullable RemoteCache remoteCache, @Nullable RemoteExecutionClient remoteExecutor, TempPathGenerator tempPathGenerator, - @Nullable Path captureCorruptedOutputsDir) { + @Nullable Path captureCorruptedOutputsDir, + @Nullable RemoteOutputChecker remoteOutputChecker) { this.reporter = reporter; this.verboseFailures = verboseFailures; this.execRoot = execRoot; @@ -214,6 +217,7 @@ this.captureCorruptedOutputsDir = captureCorruptedOutputsDir; this.scheduler = Schedulers.from(executor, /* interruptibleWorker= */ true); + this.remoteOutputChecker = remoteOutputChecker; } static Command buildCommand( @@ -1158,7 +1162,7 @@ ImmutableList.Builder<ListenableFuture<FileMetadata>> downloadsBuilder = ImmutableList.builder(); - boolean downloadOutputs = shouldDownloadOutputsFor(result, metadata); + boolean downloadOutputs = shouldDownloadOutputsFor(action, result, metadata); // Download into temporary paths, then move everything at the end. // This avoids holding the output lock while downloading, which would prevent the local branch @@ -1322,7 +1326,7 @@ } private boolean shouldDownloadOutputsFor( - RemoteActionResult result, ActionResultMetadata metadata) { + RemoteAction action, RemoteActionResult result, ActionResultMetadata metadata) { if (remoteOptions.remoteOutputsMode.downloadAllOutputs()) { return true; } @@ -1341,6 +1345,14 @@ Iterables.get(metadata.symlinks(), 0).path()))); return true; } + + checkNotNull(remoteOutputChecker, "remoteOutputChecker must not be null"); + for (var output : action.getSpawn().getOutputFiles()) { + if (remoteOutputChecker.shouldDownloadOutputDuringActionExecution(output)) { + return true; + } + } + return false; }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index 6180167..60a7c35 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java
@@ -268,7 +268,12 @@ new RemoteCache(HTTP_AND_DISK_CACHE_CAPABILITIES, cacheClient, remoteOptions, digestUtil); actionContextProvider = RemoteActionContextProvider.createForRemoteCaching( - executorService, env, remoteCache, /* retryScheduler= */ null, digestUtil); + executorService, + env, + remoteCache, + /* retryScheduler= */ null, + digestUtil, + remoteOutputChecker); } @Override @@ -657,7 +662,8 @@ remoteExecutor, retryScheduler, digestUtil, - logDir); + logDir, + remoteOutputChecker); repositoryRemoteExecutorFactoryDelegate.init( new RemoteRepositoryRemoteExecutorFactory( remoteCache, @@ -689,7 +695,7 @@ cacheCapabilities.getCacheCapabilities(), cacheClient, remoteOptions, digestUtil); actionContextProvider = RemoteActionContextProvider.createForRemoteCaching( - executorService, env, remoteCache, retryScheduler, digestUtil); + executorService, env, remoteCache, retryScheduler, digestUtil, remoteOutputChecker); } buildEventArtifactUploaderFactoryDelegate.init(
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java index 8418237..3ac336c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java
@@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Sets; +import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; @@ -47,7 +48,8 @@ private final CommandMode commandMode; private final boolean downloadToplevel; private final ImmutableList<Pattern> patternsToDownload; - private final Set<Artifact> toplevelArtifactsToDownload = Sets.newConcurrentHashSet(); + private final Set<ActionInput> toplevelArtifactsToDownload = Sets.newConcurrentHashSet(); + private final Set<ActionInput> inputsToDownload = Sets.newConcurrentHashSet(); public RemoteOutputChecker( Clock clock, @@ -125,6 +127,10 @@ } } + public void addInputToDownload(ActionInput file) { + inputsToDownload.add(file); + } + private boolean shouldDownloadToplevelOutputs(ConfiguredTarget configuredTarget) { switch (commandMode) { case RUN: @@ -144,32 +150,70 @@ } } - public boolean shouldDownloadFile(Artifact file) { - checkArgument(!file.isTreeArtifact(), "file must not be a tree."); - - if (file instanceof TreeFileArtifact) { - if (toplevelArtifactsToDownload.contains(file.getParent())) { - return true; - } - } else if (toplevelArtifactsToDownload.contains(file)) { - return true; - } - - return outputMatchesPattern(file); + private boolean shouldDownloadOutputForToplevel(ActionInput output) { + return shouldDownloadOutputFor(output, toplevelArtifactsToDownload); } - private boolean outputMatchesPattern(Artifact output) { + private boolean shouldDownloadOutputForLocalAction(ActionInput output) { + return shouldDownloadOutputFor(output, inputsToDownload); + } + + private boolean shouldDownloadFileForRegex(ActionInput file) { + checkArgument( + !(file instanceof Artifact && ((Artifact) file).isTreeArtifact()), + "file must not be a tree."); + for (var pattern : patternsToDownload) { - if (pattern.matcher(output.getExecPathString()).matches()) { + if (pattern.matcher(file.getExecPathString()).matches()) { return true; } } + return false; } + private static boolean shouldDownloadOutputFor( + ActionInput output, Set<ActionInput> artifactCollection) { + if (output instanceof TreeFileArtifact) { + if (artifactCollection.contains(((Artifact) output).getParent())) { + return true; + } + } else if (artifactCollection.contains(output)) { + return true; + } + + return false; + } + + /** + * Returns {@code true} if Bazel should download this {@link ActionInput} during spawn execution. + * + * @param output output of the spawn. Tree is accepted since we can't know the content of tree + * before executing the spawn. + */ + public boolean shouldDownloadOutputDuringActionExecution(ActionInput output) { + // Download toplevel artifacts within action execution so that when the event TargetComplete is + // emitted, related toplevel artifacts are downloaded. + // + // Download outputs that are inputs to local actions within action execution so that the local + // actions don't need to wait for background downloads. + return shouldDownloadOutputForToplevel(output) || shouldDownloadOutputForLocalAction(output); + } + + /** + * Returns {@code true} if Bazel should download this {@link ActionInput} after action execution. + * + * @param file file output of the action. Tree must be expanded to tree file. + */ + public boolean shouldDownloadFileAfterActionExecution(ActionInput file) { + // Download user requested blobs in background to finish action execution sooner so that other + // actions can start sooner. + return shouldDownloadFileForRegex(file); + } + @Override - public boolean shouldTrustRemoteArtifact(Artifact file, RemoteFileArtifactValue metadata) { - if (shouldDownloadFile(file)) { + public boolean shouldTrustRemoteArtifact(ActionInput file, RemoteFileArtifactValue metadata) { + if (shouldDownloadOutputForToplevel(file) || shouldDownloadFileForRegex(file)) { // If Bazel should download this file, but it does not exist locally, returns false to rerun // the generating action to trigger the download (just like in the normal build, when local // outputs are missing).
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java index 03048b8..996d1ca 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputService.java
@@ -137,7 +137,8 @@ } @Override - public void flushActionFileSystem(FileSystem actionFileSystem) throws IOException { + public void flushActionFileSystem(FileSystem actionFileSystem) + throws InterruptedException, IOException { ((RemoteActionFileSystem) actionFileSystem).flush(); }
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 6b37fd0..954a87c 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
@@ -1468,7 +1468,7 @@ private static void flushActionFileSystem( @Nullable FileSystem actionFileSystem, @Nullable OutputService outputService) - throws IOException { + throws IOException, InterruptedException { if (outputService != null && actionFileSystem != null) { outputService.flushActionFileSystem(actionFileSystem); }
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java b/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java index f87ab8b..840a6aa 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/OutputService.java
@@ -212,7 +212,8 @@ * Flush the internal state of filesystem returned by {@link #createActionFileSystem} after action * execution, before skyframe checking the action outputs. */ - default void flushActionFileSystem(FileSystem actionFileSystem) throws IOException {} + default void flushActionFileSystem(FileSystem actionFileSystem) + throws IOException, InterruptedException {} default boolean supportsPathResolverForArtifactValues() { return false;
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 c869f60..9ae6943 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
@@ -1045,9 +1045,11 @@ /* remoteDefaultPlatformProperties= */ ImmutableMap.of(), mockedRemoteArtifactChecker); verify(mockedRemoteArtifactChecker) - .shouldTrustRemoteArtifact(argThat(arg -> arg.getFilename().equals("file1")), any()); + .shouldTrustRemoteArtifact( + argThat(arg -> arg.getExecPathString().endsWith("file1")), any()); verify(mockedRemoteArtifactChecker, never()) - .shouldTrustRemoteArtifact(argThat(arg -> arg.getFilename().equals("file2")), any()); + .shouldTrustRemoteArtifact( + argThat(arg -> arg.getExecPathString().endsWith("file2")), any()); // Not cached since local file changed runAction( action,
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index cedd015..a010b14 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD
@@ -139,6 +139,7 @@ "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/util:os", "//src/main/java/com/google/devtools/build/lib/util/io", "//src/main/java/com/google/devtools/build/lib/vfs",
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java index d07d0e4..a1aa27e 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTest.java
@@ -91,8 +91,7 @@ } @Override - protected void assertOutputEquals(Path path, String expectedContent, boolean isLocal) - throws Exception { + protected void assertOutputEquals(Path path, String expectedContent) throws Exception { assertThat(readContent(path, UTF_8)).isEqualTo(expectedContent); } @@ -683,7 +682,90 @@ waitDownloads(); // Assert: target was successfully built - assertValidOutputFile( - "a/bar.out", "file-inside\nupdated bar" + lineSeparator(), /* isLocal= */ true); + assertValidOutputFile("a/bar.out", "file-inside\nupdated bar" + lineSeparator()); + } + + @Test + public void downloadToplevel_symlinkFile() throws Exception { + // TODO(chiwang): Make metadata for downloaded symlink non-remote. + assumeFalse(OS.getCurrent() == OS.WINDOWS); + + setDownloadToplevel(); + writeSymlinkRule(); + write( + "BUILD", + "load(':symlink.bzl', 'symlink')", + "genrule(", + " name = 'foo',", + " srcs = [],", + " outs = ['out/foo.txt'],", + " cmd = 'echo foo > $@',", + ")", + "symlink(", + " name = 'foo-link',", + " target = ':foo'", + ")"); + + buildTarget("//:foo-link"); + + assertValidOutputFile("foo-link", "foo\n"); + + // Delete link, re-plant symlink + getOutputPath("foo-link").delete(); + + buildTarget("//:foo-link"); + + assertValidOutputFile("foo-link", "foo\n"); + + // Delete target, re-download it + getOutputPath("foo").delete(); + + assertValidOutputFile("foo-link", "foo\n"); + } + + @Test + public void downloadToplevel_symlinkTree() throws Exception { + // TODO(chiwang): Make metadata for downloaded symlink non-remote. + assumeFalse(OS.getCurrent() == OS.WINDOWS); + + setDownloadToplevel(); + writeSymlinkRule(); + writeOutputDirRule(); + write( + "BUILD", + "load(':output_dir.bzl', 'output_dir')", + "load(':symlink.bzl', 'symlink')", + "output_dir(", + " name = 'foo',", + " content_map = {'file-1': '1', 'file-2': '2', 'file-3': '3'},", + ")", + "symlink(", + " name = 'foo-link',", + " target = ':foo'", + ")"); + + buildTarget("//:foo-link"); + + assertValidOutputFile("foo-link/file-1", "1"); + assertValidOutputFile("foo-link/file-2", "2"); + assertValidOutputFile("foo-link/file-3", "3"); + + getOutputPath("foo-link").deleteTree(); + + // Delete link, re-plant symlink + buildTarget("//:foo-link"); + + assertValidOutputFile("foo-link/file-1", "1"); + assertValidOutputFile("foo-link/file-2", "2"); + assertValidOutputFile("foo-link/file-3", "3"); + + // Delete target, re-download them + getOutputPath("foo").deleteTree(); + + buildTarget("//:foo-link"); + + assertValidOutputFile("foo-link/file-1", "1"); + assertValidOutputFile("foo-link/file-2", "2"); + assertValidOutputFile("foo-link/file-3", "3"); } }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java index 83bb5b4..a34b117 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
@@ -20,7 +20,6 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertThrows; -import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.common.eventbus.Subscribe; import com.google.devtools.build.lib.actions.ActionExecutedEvent; @@ -30,6 +29,7 @@ import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase; import com.google.devtools.build.lib.skyframe.ActionExecutionValue; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.util.io.RecordingOutErr; import com.google.devtools.build.lib.vfs.Path; @@ -46,8 +46,7 @@ protected abstract void setDownloadAll(); - protected abstract void assertOutputEquals(Path path, String expectedContent, boolean isLocal) - throws Exception; + protected abstract void assertOutputEquals(Path path, String expectedContent) throws Exception; protected abstract void assertOutputContains(String content, String contains) throws Exception; @@ -641,6 +640,8 @@ assertValidOutputFile("foo/file-1", "1"); assertValidOutputFile("foo/file-2", "2"); assertValidOutputFile("foo/file-3", "3"); + assertThat(getMetadata("//:foo").values().stream().noneMatch(FileArtifactValue::isRemote)) + .isTrue(); } @Test @@ -671,8 +672,14 @@ waitDownloads(); assertValidOutputFile("out/foo1.txt", "foo1\n"); + assertThat(getMetadata("//:foo1").values().stream().noneMatch(FileArtifactValue::isRemote)) + .isTrue(); assertValidOutputFile("out/foo2.txt", "foo2\n"); + assertThat(getMetadata("//:foo2").values().stream().noneMatch(FileArtifactValue::isRemote)) + .isTrue(); assertValidOutputFile("out/foo3.txt", "foo3\n"); + assertThat(getMetadata("//:foo3").values().stream().noneMatch(FileArtifactValue::isRemote)) + .isTrue(); } @Test @@ -705,15 +712,27 @@ waitDownloads(); assertOutputsDoNotExist("//:foo1"); + assertThat(getMetadata("//:foo1").values().stream().allMatch(FileArtifactValue::isRemote)) + .isTrue(); assertOutputsDoNotExist("//:foo2"); + assertThat(getMetadata("//:foo2").values().stream().allMatch(FileArtifactValue::isRemote)) + .isTrue(); assertOutputsDoNotExist("//:foo3"); + assertThat(getMetadata("//:foo3").values().stream().allMatch(FileArtifactValue::isRemote)) + .isTrue(); buildTarget("//:foo1", "//:foo2", "//:foo3"); waitDownloads(); assertValidOutputFile("out/foo1.txt", "foo1\n"); + assertThat(getMetadata("//:foo1").values().stream().noneMatch(FileArtifactValue::isRemote)) + .isTrue(); assertValidOutputFile("out/foo2.txt", "foo2\n"); + assertThat(getMetadata("//:foo2").values().stream().noneMatch(FileArtifactValue::isRemote)) + .isTrue(); assertValidOutputFile("out/foo3.txt", "foo3\n"); + assertThat(getMetadata("//:foo3").values().stream().noneMatch(FileArtifactValue::isRemote)) + .isTrue(); } @Test @@ -1003,7 +1022,7 @@ buildTarget("//:foobar"); assertValidOutputFile("out/foo.txt", "foo\n"); assertValidOutputFile("out/foobar.txt", "foo\nbar\n"); - assertThat(getOnlyElement(getFileMetadata("//:foo").values()).isRemote()).isTrue(); + assertThat(getOnlyElement(getMetadata("//:foo").values()).isRemote()).isTrue(); // Act: Do an incremental build without any modifications ActionEventCollector actionEventCollector = new ActionEventCollector(); @@ -1016,21 +1035,34 @@ assertThat(actionEventCollector.getActionExecutedEvents()).isEmpty(); // Two actions are invalidated but were able to hit the action cache assertThat(actionEventCollector.getCachedActionEvents()).hasSize(2); - assertThat(getOnlyElement(getFileMetadata("//:foo").values()).isRemote()).isFalse(); + assertThat(getOnlyElement(getMetadata("//:foo").values()).isRemote()).isFalse(); } - private ImmutableMap<Artifact, FileArtifactValue> getFileMetadata(String target) - throws Exception { + protected ImmutableMap<Artifact, FileArtifactValue> getMetadata(String target) throws Exception { var result = ImmutableMap.<Artifact, FileArtifactValue>builder(); var evaluator = getRuntimeWrapper().getSkyframeExecutor().getEvaluator(); for (var artifact : getArtifacts(target)) { var value = evaluator.getExistingValue(Artifact.key(artifact)); - Preconditions.checkState(value instanceof ActionExecutionValue); - result.putAll(((ActionExecutionValue) value).getAllFileValues()); + if (value instanceof ActionExecutionValue) { + result.putAll(((ActionExecutionValue) value).getAllFileValues()); + } else if (value instanceof TreeArtifactValue) { + result.putAll(((TreeArtifactValue) value).getChildValues()); + } } return result.buildOrThrow(); } + protected FileArtifactValue getMetadata(Artifact output) throws Exception { + var evaluator = getRuntimeWrapper().getSkyframeExecutor().getEvaluator(); + var value = evaluator.getExistingValue(Artifact.key(output)); + if (value instanceof ActionExecutionValue) { + return ((ActionExecutionValue) value).getAllFileValues().get(output); + } else if (value instanceof TreeArtifactValue) { + return ((TreeArtifactValue) value).getChildValues().get(output); + } + return null; + } + @Test public void incrementalBuild_intermediateOutputModified_rerunGeneratingActions() throws Exception { @@ -1169,8 +1201,7 @@ buildTarget("//a:bar"); // Assert: target was successfully built - assertValidOutputFile( - "a/bar.out", "file-inside\nupdated bar" + lineSeparator(), /* isLocal= */ true); + assertValidOutputFile("a/bar.out", "file-inside\nupdated bar" + lineSeparator()); } @Test @@ -1262,8 +1293,7 @@ waitDownloads(); // Assert: target was successfully built - assertValidOutputFile( - "a/bar.out", "file-inside\nupdated bar" + lineSeparator(), /* isLocal= */ true); + assertValidOutputFile("a/bar.out", "file-inside\nupdated bar" + lineSeparator()); } protected void assertOutputsDoNotExist(String target) throws Exception { @@ -1292,22 +1322,37 @@ Artifact output = getOnlyElement(getArtifacts(target)); assertThat(output.getFilename()).isEqualTo(filename); assertThat(output.getPath().exists()).isTrue(); - assertOutputEquals(output.getPath(), content, /* isLocal= */ false); + assertOutputEquals(output.getPath(), content); } protected void assertValidOutputFile(String binRelativePath, String content) throws Exception { - assertValidOutputFile(binRelativePath, content, /* isLocal= */ false); - } - - protected void assertValidOutputFile(String binRelativePath, String content, boolean isLocal) - throws Exception { Path output = getOutputPath(binRelativePath); - assertOutputEquals(getOutputPath(binRelativePath), content, isLocal); + assertOutputEquals(getOutputPath(binRelativePath), content); assertThat(output.isReadable()).isTrue(); assertThat(output.isWritable()).isFalse(); assertThat(output.isExecutable()).isTrue(); } + protected void writeSymlinkRule() throws IOException { + write( + "symlink.bzl", + "def _symlink_impl(ctx):", + " target = ctx.file.target", + " if target.is_directory:", + " link = ctx.actions.declare_directory(ctx.attr.name)", + " else:", + " link = ctx.actions.declare_file(ctx.attr.name)", + " ctx.actions.symlink(output = link, target_file = target)", + " return DefaultInfo(files = depset([link]))", + "", + "symlink = rule(", + " implementation = _symlink_impl,", + " attrs = {", + " 'target': attr.label(mandatory = True, allow_single_file = True),", + " }", + ")"); + } + protected void writeOutputDirRule() throws IOException { write( "output_dir.bzl",
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java index 32f6a0f..3f585c1 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java
@@ -21,6 +21,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -45,6 +46,7 @@ import com.google.devtools.build.lib.actions.StaticInputMetadataProvider; import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; +import com.google.devtools.build.lib.clock.JavaClock; import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; @@ -68,6 +70,9 @@ /** Tests for {@link RemoteActionFileSystem} */ @RunWith(JUnit4.class) public final class RemoteActionFileSystemTest extends RemoteActionFileSystemTestBase { + private static final RemoteOutputChecker DUMMY_REMOTE_OUTPUT_CHECKER = + new RemoteOutputChecker( + new JavaClock(), "build", /* downloadToplevel= */ false, ImmutableList.of()); private static final DigestHashFunction HASH_FUNCTION = DigestHashFunction.SHA256; @@ -90,6 +95,7 @@ protected RemoteActionFileSystem createActionFileSystem( ActionInputMap inputs, Iterable<Artifact> outputs, InputMetadataProvider fileCache) throws IOException { + doReturn(DUMMY_REMOTE_OUTPUT_CHECKER).when(inputFetcher).getRemoteOutputChecker(); RemoteActionFileSystem remoteActionFileSystem = new RemoteActionFileSystem( fs, @@ -373,7 +379,7 @@ } @Test - public void createSymbolicLink_localFileArtifact() throws IOException { + public void createSymbolicLink_localFileArtifact() throws Exception { // arrange ActionInputMap inputs = new ActionInputMap(1); Artifact localArtifact = createLocalArtifact("local-file", "local contents", inputs); @@ -403,7 +409,7 @@ } @Test - public void createSymbolicLink_remoteFileArtifact() throws IOException { + public void createSymbolicLink_remoteFileArtifact() throws Exception { // arrange ActionInputMap inputs = new ActionInputMap(1); Artifact remoteArtifact = createRemoteArtifact("remote-file", "remote contents", inputs); @@ -440,7 +446,7 @@ } @Test - public void createSymbolicLink_localTreeArtifact() throws IOException { + public void createSymbolicLink_localTreeArtifact() throws Exception { // arrange ActionInputMap inputs = new ActionInputMap(1); ImmutableMap<String, String> contentMap = @@ -473,7 +479,7 @@ } @Test - public void createSymbolicLink_remoteTreeArtifact() throws IOException { + public void createSymbolicLink_remoteTreeArtifact() throws Exception { // arrange ActionInputMap inputs = new ActionInputMap(1); ImmutableMap<String, String> contentMap = @@ -511,7 +517,7 @@ } @Test - public void createSymbolicLink_unresolvedSymlink() throws IOException { + public void createSymbolicLink_unresolvedSymlink() throws Exception { // arrange ActionInputMap inputs = new ActionInputMap(1); SpecialArtifact outputArtifact =
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java index 12e8de9..61a16d7 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
@@ -128,6 +128,9 @@ /** Tests for {@link RemoteExecutionService}. */ @RunWith(JUnit4.class) public class RemoteExecutionServiceTest { + private static final RemoteOutputChecker DUMMY_REMOTE_OUTPUT_CHECKER = + new RemoteOutputChecker( + new JavaClock(), "build", /* downloadToplevel= */ false, ImmutableList.of()); @Rule public final RxNoGlobalErrorsRule rxNoGlobalErrorsRule = new RxNoGlobalErrorsRule(); private final DigestUtil digestUtil = @@ -2157,6 +2160,7 @@ cache, executor, tempPathGenerator, - null); + null, + DUMMY_REMOTE_OUTPUT_CHECKER); } }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java index 318f1bd..22c5ff7 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
@@ -104,6 +104,9 @@ /** Tests for {@link RemoteSpawnCache}. */ @RunWith(JUnit4.class) public class RemoteSpawnCacheTest { + private static final RemoteOutputChecker DUMMY_REMOTE_OUTPUT_CHECKER = + new RemoteOutputChecker( + new JavaClock(), "build", /* downloadToplevel= */ false, ImmutableList.of()); private static final ArtifactExpander SIMPLE_ARTIFACT_EXPANDER = (artifact, output) -> output.add(artifact); @@ -236,7 +239,7 @@ new RemoteExecutionService( directExecutor(), reporter, - /*verboseFailures=*/ true, + /* verboseFailures= */ true, execRoot, remotePathResolver, BUILD_REQUEST_ID, @@ -246,7 +249,8 @@ remoteCache, null, tempPathGenerator, - /* captureCorruptedOutputsDir= */ null)); + /* captureCorruptedOutputsDir= */ null, + DUMMY_REMOTE_OUTPUT_CHECKER)); return new RemoteSpawnCache(execRoot, options, /* verboseFailures=*/ true, service); }
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index ca4e6ec..e7e22a9 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
@@ -128,6 +128,9 @@ /** Tests for {@link com.google.devtools.build.lib.remote.RemoteSpawnRunner} */ @RunWith(JUnit4.class) public class RemoteSpawnRunnerTest { + private static final RemoteOutputChecker DUMMY_REMOTE_OUTPUT_CHECKER = + new RemoteOutputChecker( + new JavaClock(), "build", /* downloadToplevel= */ false, ImmutableList.of()); private final Reporter reporter = new Reporter(new EventBus()); private static final ImmutableMap<String, String> NO_CACHE = @@ -1037,7 +1040,7 @@ new RemoteExecutionService( directExecutor(), reporter, - /*verboseFailures=*/ true, + /* verboseFailures= */ true, execRoot, RemotePathResolver.createDefault(execRoot), "build-req-id", @@ -1047,14 +1050,15 @@ cache, executor, tempPathGenerator, - /* captureCorruptedOutputsDir= */ null); + /* captureCorruptedOutputsDir= */ null, + DUMMY_REMOTE_OUTPUT_CHECKER); RemoteSpawnRunner runner = new RemoteSpawnRunner( execRoot, remoteOptions, executionOptions, true, - /*cmdlineReporter=*/ null, + /* cmdlineReporter= */ null, retryService, logDir, remoteExecutionService); @@ -1573,7 +1577,7 @@ new RemoteExecutionService( directExecutor(), reporter, - /*verboseFailures=*/ true, + /* verboseFailures= */ true, execRoot, remotePathResolver, "build-req-id", @@ -1583,7 +1587,8 @@ cache, executor, tempPathGenerator, - /*captureCorruptedOutputsDir=*/ null)); + /* captureCorruptedOutputsDir= */ null, + DUMMY_REMOTE_OUTPUT_CHECKER)); return new RemoteSpawnRunner( execRoot,
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java index 11a14cb..86c7faf 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java
@@ -126,6 +126,9 @@ /** Tests for {@link RemoteSpawnRunner} in combination with {@link GrpcRemoteExecutor}. */ @RunWith(JUnit4.class) public class RemoteSpawnRunnerWithGrpcRemoteExecutorTest { + private static final RemoteOutputChecker DUMMY_REMOTE_OUTPUT_CHECKER = + new RemoteOutputChecker( + new JavaClock(), "build", /* downloadToplevel= */ false, ImmutableList.of()); private static final DigestUtil DIGEST_UTIL = new DigestUtil(SyscallCache.NO_CACHE, DigestHashFunction.SHA256); @@ -309,7 +312,7 @@ new RemoteExecutionService( directExecutor(), reporter, - /*verboseFailures=*/ true, + /* verboseFailures= */ true, execRoot, RemotePathResolver.createDefault(execRoot), "build-req-id", @@ -319,7 +322,8 @@ remoteCache, executor, tempPathGenerator, - /* captureCorruptedOutputsDir= */ null); + /* captureCorruptedOutputsDir= */ null, + DUMMY_REMOTE_OUTPUT_CHECKER); client = new RemoteSpawnRunner( execRoot,