From d476b264ef7aeaa80aa68db75c3cfb6b3ccf2a67 Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Thu, 15 Sep 2022 11:34:05 +0200 Subject: [PATCH] Inject metadata when creating a filesystem symlink for a non-symlink artifact. A ctx.actions.symlink whose output is a declare_file or declare_directory (as opposed to a declare_symlink) has "copy" semantics, i.e., the output artifact is indistinguishable from the referent except for its name; the symlink is just a filesystem-level optimization to avoid an actual copy, and is transparently resolved when collecting the action output metadata. When the symlink points to an artifact that was built remotely and without the bytes, we currently must download it before executing the symlink action, so that the output metadata can be constructed from the local filesystem. This change short-circuits the filesystem traversal by injecting output metadata, which is identical to the input plus a pointer to the original path. This is used by the prefetcher to avoid downloading the same files multiple times when they're symlinked more than once. (An alternative would have been to teach all of the RemoteActionFileSystem methods to resolve symlinks by patching together the local and remote metadata, but that would have resulted in an awful lot of complexity.) Fixes #15678. --- .../build/lib/actions/ActionInputMap.java | 23 ++ .../build/lib/actions/FileArtifactValue.java | 41 ++- .../build/lib/actions/MetadataProvider.java | 6 + .../cache/CompactPersistentActionCache.java | 18 +- .../com/google/devtools/build/lib/exec/BUILD | 1 + .../build/lib/exec/SingleBuildFileCache.java | 10 + .../remote/AbstractActionInputPrefetcher.java | 88 +++++- .../google/devtools/build/lib/remote/BUILD | 2 + .../lib/remote/RemoteActionFileSystem.java | 108 ++++++- .../build/lib/remote/RemoteOutputService.java | 8 +- .../lib/skyframe/ActionMetadataHandler.java | 19 ++ .../lib/skyframe/SkyframeActionExecutor.java | 9 + .../build/lib/skyframe/TreeArtifactValue.java | 35 ++- .../build/lib/actions/ActionInputMapTest.java | 4 + .../CompactPersistentActionCacheTest.java | 25 +- .../lib/actions/util/ActionsTestUtil.java | 20 ++ .../google/devtools/build/lib/exec/util/BUILD | 1 + .../exec/util/FakeActionInputFileCache.java | 7 + .../remote/ActionInputPrefetcherTestBase.java | 163 ++++++++-- .../google/devtools/build/lib/remote/BUILD | 1 + .../BuildWithoutTheBytesIntegrationTest.java | 286 ++++++++++++++++++ ...eStreamBuildEventArtifactUploaderTest.java | 2 + .../lib/remote/FakeActionInputFileCache.java | 7 + .../remote/RemoteActionFileSystemTest.java | 143 +++++++-- .../devtools/build/lib/remote/util/BUILD | 1 + .../remote/util/StaticMetadataProvider.java | 16 + .../lib/skyframe/TreeArtifactValueTest.java | 11 + .../build/remote/worker/ExecutionServer.java | 5 + 28 files changed, 970 insertions(+), 90 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputMap.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputMap.java index 902678d42e247a..45c065bbd83bd9 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputMap.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputMap.java @@ -84,6 +84,18 @@ TrieArtifact getOrPutSubFolder(String name) { return trieArtifact; } + @Nullable + TreeArtifactValue findTreeArtifactNode(PathFragment execPath) { + TrieArtifact current = this; + for (String segment : execPath.segments()) { + current = current.subFolders.get(segment); + if (current == null) { + return null; + } + } + return current.treeArtifactValue; + } + @Nullable TreeArtifactValue findTreeArtifactNodeAtPrefix(PathFragment execPath) { TrieArtifact current = this; @@ -244,6 +256,17 @@ private FileArtifactValue getMetadataFromTreeArtifacts(PathFragment execPath) { return entry != null ? entry.getValue() : null; } + @Nullable + @Override + public TreeArtifactValue getTreeMetadata(SpecialArtifact input) { + return getTreeMetadata(input.getExecPath()); + } + + @Nullable + public TreeArtifactValue getTreeMetadata(PathFragment execPath) { + return treeArtifactsRoot.findTreeArtifactNode(execPath); + } + @Nullable @Override public ActionInput getInput(String execPathString) { diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index b8dd83f748e348..9a5968cf73f876 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.actions; import static java.nio.charset.StandardCharsets.ISO_8859_1; +import static com.google.common.base.Preconditions.checkState; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; @@ -34,8 +35,10 @@ import com.google.devtools.build.skyframe.SkyValue; import java.io.ByteArrayInputStream; import java.io.IOException; +import java.rmi.Remote; import java.util.Arrays; import java.util.Objects; +import java.util.Optional; import javax.annotation.Nullable; /** @@ -539,16 +542,27 @@ public static class RemoteFileArtifactValue extends FileArtifactValue { private final long size; private final int locationIndex; private final String actionId; - - public RemoteFileArtifactValue(byte[] digest, long size, int locationIndex, String actionId) { + @Nullable private final PathFragment symlinkTargetExecPath; + + public RemoteFileArtifactValue( + byte[] digest, + long size, + int locationIndex, + String actionId, + @Nullable PathFragment symlinkTargetExecPath) { this.digest = Preconditions.checkNotNull(digest, actionId); this.size = size; this.locationIndex = locationIndex; this.actionId = actionId; + this.symlinkTargetExecPath = symlinkTargetExecPath; + } + + public RemoteFileArtifactValue(byte[] digest, long size, int locationIndex, String actionId) { + this(digest, size, locationIndex, actionId, /* symlinkTargetExecPath= */ null); } public RemoteFileArtifactValue(byte[] digest, long size, int locationIndex) { - this(digest, size, locationIndex, /* actionId= */ ""); + this(digest, size, locationIndex, /* actionId= */ "", /* symlinkTargetExecPath= */ null); } @Override @@ -561,12 +575,14 @@ public boolean equals(Object o) { return Arrays.equals(digest, that.digest) && size == that.size && locationIndex == that.locationIndex - && Objects.equals(actionId, that.actionId); + && Objects.equals(actionId, that.actionId) + && Objects.equals(symlinkTargetExecPath, that.symlinkTargetExecPath); } @Override public int hashCode() { - return Objects.hash(Arrays.hashCode(digest), size, locationIndex, actionId); + return Objects.hash( + Arrays.hashCode(digest), size, locationIndex, actionId, symlinkTargetExecPath); } @Override @@ -597,7 +613,7 @@ public String getActionId() { @Override public long getModifiedTime() { throw new UnsupportedOperationException( - "RemoteFileArifactValue doesn't support getModifiedTime"); + "RemoteFileArtifactValue doesn't support getModifiedTime"); } @Override @@ -615,12 +631,25 @@ public boolean isRemote() { return true; } + /** + * Optional symlink target path. + * + *

If set, this artifact is a copy of another artifact. It is materialized in the local + * filesystem as a symlink to the original artifact, whose contents live at this location. This + * is used to implement zero-cost copies of remotely stored artifacts. + */ + public Optional getSymlinkTargetExecPath() { + return Optional.ofNullable(symlinkTargetExecPath); + } + @Override public String toString() { return MoreObjects.toStringHelper(this) .add("digest", bytesToString(digest)) .add("size", size) .add("locationIndex", locationIndex) + .add("actionId", actionId) + .add("symlinkTargetExecPath", symlinkTargetExecPath) .toString(); } } diff --git a/src/main/java/com/google/devtools/build/lib/actions/MetadataProvider.java b/src/main/java/com/google/devtools/build/lib/actions/MetadataProvider.java index ed1bbe5e9cbb16..58a690b436106c 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/MetadataProvider.java +++ b/src/main/java/com/google/devtools/build/lib/actions/MetadataProvider.java @@ -14,7 +14,9 @@ package com.google.devtools.build.lib.actions; import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.vfs.FileSystem; import java.io.IOException; import javax.annotation.Nullable; @@ -48,6 +50,10 @@ public interface MetadataProvider { @Nullable FileArtifactValue getMetadata(ActionInput input) throws IOException; + /** Returns a {@link TreeArtifactValue} for the given {@link ActionInput}. */ + @Nullable + TreeArtifactValue getTreeMetadata(SpecialArtifact artifact) throws IOException; + /** Looks up an input from its exec path. */ @Nullable ActionInput getInput(String execPath); diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java index 36c4a8ef99ccf1..f5ed4b6e02f8bb 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java @@ -36,6 +36,7 @@ import com.google.devtools.build.lib.util.VarInt; import com.google.devtools.build.lib.vfs.DigestUtils; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.lib.vfs.UnixGlob; import java.io.ByteArrayOutputStream; @@ -466,6 +467,14 @@ private static void encodeRemoteMetadata( VarInt.putVarInt(value.getLocationIndex(), sink); VarInt.putVarInt(indexer.getOrCreateIndex(value.getActionId()), sink); + + Optional symlinkTargetExecPath = value.getSymlinkTargetExecPath(); + if (symlinkTargetExecPath.isPresent()) { + VarInt.putVarInt(1, sink); + VarInt.putVarInt(indexer.getOrCreateIndex(symlinkTargetExecPath.get().toString()), sink); + } else { + VarInt.putVarInt(0, sink); + } } private static final int MAX_REMOTE_METADATA_SIZE = @@ -484,7 +493,14 @@ private static RemoteFileArtifactValue decodeRemoteMetadata( String actionId = getStringForIndex(indexer, VarInt.getVarInt(source)); - return new RemoteFileArtifactValue(digest, size, locationIndex, actionId); + PathFragment symlinkTargetExecPath = null; + if (VarInt.getVarInt(source) != 0) { + symlinkTargetExecPath = + PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source))); + } + + return new RemoteFileArtifactValue( + digest, size, locationIndex, actionId, symlinkTargetExecPath); } /** @return action data encoded as a byte[] array. */ diff --git a/src/main/java/com/google/devtools/build/lib/exec/BUILD b/src/main/java/com/google/devtools/build/lib/exec/BUILD index 86868c5c6acacd..bc726d62104231 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD @@ -203,6 +203,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:action_input_helper", "//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:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/vfs", "//third_party:caffeine", "//third_party:jsr305", diff --git a/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java b/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java index e7542a1ad9e57b..5156961371eec0 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java @@ -17,9 +17,11 @@ import com.github.benmanes.caffeine.cache.Caffeine; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputHelper; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.DigestOfDirectoryException; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.MetadataProvider; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.Symlinks; @@ -81,6 +83,14 @@ public FileArtifactValue getMetadata(ActionInput input) throws IOException { .getMetadata(); } + @Override + @Nullable + public TreeArtifactValue getTreeMetadata(SpecialArtifact artifact) throws IOException { + // TODO(tjgq): Construct a TreeArtifactValue from the filesystem to mirror getMetadata? + // If so, we should share the logic with MetadataHandler#constructTreeArtifactFromFilesystem. + return null; + } + @Override @Nullable public ActionInput getInput(String execPath) { 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 752212fa70639b..44a629055e4615 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 @@ -31,13 +31,16 @@ import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.MetadataProvider; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.remote.util.AsyncTaskCache; import com.google.devtools.build.lib.remote.util.RxUtils.TransferResult; import com.google.devtools.build.lib.remote.util.TempPathGenerator; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; import io.reactivex.rxjava3.core.Completable; import io.reactivex.rxjava3.core.Flowable; import io.reactivex.rxjava3.core.Single; @@ -118,6 +121,7 @@ protected ListenableFuture prefetchFiles( Priority priority) { Map> trees = new HashMap<>(); List files = new ArrayList<>(); + for (ActionInput input : inputs) { if (input instanceof Artifact && ((Artifact) input).isSourceArtifact()) { continue; @@ -138,23 +142,55 @@ protected ListenableFuture prefetchFiles( .flatMapSingle( entry -> toTransferResult( - prefetchInputTree( + prefetchInputTreeOrSymlink( metadataProvider, entry.getKey(), entry.getValue(), priority))); + Flowable fileDownloads = Flowable.fromIterable(files) .flatMapSingle( - input -> toTransferResult(prefetchInputFile(metadataProvider, input, priority))); + input -> + toTransferResult( + prefetchInputFileOrSymlink(metadataProvider, input, priority))); + Flowable transfers = Flowable.merge(treeDownloads, fileDownloads); Completable prefetch = mergeBulkTransfer(transfers).onErrorResumeNext(this::onErrorResumeNext); return toListenableFuture(prefetch); } - private Completable prefetchInputTree( + private Completable prefetchInputTreeOrSymlink( MetadataProvider provider, SpecialArtifact tree, List treeFiles, + Priority priority) + throws IOException { + + PathFragment execPath = tree.getExecPath(); + + TreeArtifactValue treeMetadata = provider.getTreeMetadata(tree); + if (treeMetadata == null) { + return Completable.complete(); + } + + PathFragment prefetchExecPath = treeMetadata.getSymlinkTargetExecPath().orElse(execPath); + + Completable prefetch = prefetchInputTree(provider, prefetchExecPath, treeFiles, priority); + + // If prefetching to a different path, plant a symlink into it. + if (!prefetchExecPath.equals(execPath)) { + Completable prefetchAndSymlink = + prefetch.doOnComplete(() -> createSymlink(execPath, prefetchExecPath)); + return downloadCache.executeIfNot(execRoot.getRelative(execPath), prefetchAndSymlink); + } + + return prefetch; + } + + private Completable prefetchInputTree( + MetadataProvider provider, + PathFragment execPath, + List treeFiles, Priority priority) { - Path treeRoot = execRoot.getRelative(tree.getExecPath()); + Path treeRoot = execRoot.getRelative(execPath); HashMap treeFileTmpPathMap = new HashMap<>(); Flowable transfers = @@ -182,10 +218,15 @@ private Completable prefetchInputTree( () -> { HashSet dirs = new HashSet<>(); - // Tree root is created by Bazel before action execution, but the permission is - // changed to 0555 afterwards. We need to set it as writable in order to move - // files into it. - treeRoot.setWritable(true); + if (!treeRoot.exists()) { + // If the tree root doesn't already exist, create it now. This can happen if + // we're prefetching to a different location and symlinking into it. + treeRoot.createDirectory(); + } else { + // If the tree root already exists, make it writable, so we can move files into + // it. + treeRoot.setWritable(true); + } dirs.add(treeRoot); for (Map.Entry entry : treeFileTmpPathMap.entrySet()) { @@ -230,7 +271,7 @@ private Completable prefetchInputTree( return downloadCache.executeIfNot(treeRoot, download); } - private Completable prefetchInputFile( + private Completable prefetchInputFileOrSymlink( MetadataProvider metadataProvider, ActionInput input, Priority priority) throws IOException { if (input instanceof VirtualActionInput) { prefetchVirtualActionInput((VirtualActionInput) input); @@ -238,12 +279,26 @@ private Completable prefetchInputFile( } FileArtifactValue metadata = metadataProvider.getMetadata(input); - if (metadata == null) { + if (metadata == null || !metadata.isRemote()) { return Completable.complete(); } - Path path = execRoot.getRelative(input.getExecPath()); - return downloadFileRx(path, metadata, priority); + PathFragment execPath = input.getExecPath(); + + PathFragment prefetchExecPath = + ((RemoteFileArtifactValue) metadata).getSymlinkTargetExecPath().orElse(execPath); + + Completable prefetch = + downloadFileRx(execRoot.getRelative(prefetchExecPath), metadata, priority); + + // If prefetching to a different path, plant a symlink into it. + if (!prefetchExecPath.equals(execPath)) { + Completable prefetchAndSymlink = + prefetch.doOnComplete(() -> createSymlink(execPath, prefetchExecPath)); + return downloadCache.executeIfNot(execRoot.getRelative(execPath), prefetchAndSymlink); + } + + return prefetch; } /** @@ -327,6 +382,15 @@ private void deletePartialDownload(Path path) { } } + private void createSymlink(PathFragment linkPath, PathFragment targetPath) throws IOException { + Path link = execRoot.getRelative(linkPath); + Path target = execRoot.getRelative(targetPath); + // Delete the link path if it already exists. + // This will happen for output directories, which get created before the action runs. + link.delete(); + link.createSymbolicLink(target); + } + public ImmutableSet downloadedFiles() { return downloadCache.getFinishedTasks(); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD index d783b419a8c414..1fa5d832a705d1 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD @@ -177,7 +177,9 @@ java_library( "//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/remote/util", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//third_party:flogger", "//third_party:guava", "//third_party:rxjava3", 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 6c43cf65b16f0d..3a40a7c3ff6447 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 @@ -16,7 +16,11 @@ package com.google.devtools.build.lib.remote; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableMap.toImmutableMap; +import static com.google.common.collect.Streams.stream; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputMap; import com.google.devtools.build.lib.actions.Artifact; @@ -43,7 +47,7 @@ /** * This is a basic implementation and incomplete implementation of an action file system that's been - * tuned to what native (non-spawn) actions in Bazel currently use. + * tuned to what internal (non-spawn) actions in Bazel currently use. * *

The implementation mostly delegates to the local file system except for the case where an * action input is a remotely stored action output. Most notably {@link @@ -52,12 +56,13 @@ *

This implementation only supports creating local action outputs. */ public class RemoteActionFileSystem extends DelegateFileSystem { - private final PathFragment execRoot; private final PathFragment outputBase; private final ActionInputMap inputArtifactData; + private final ImmutableMap outputArtifactByExecPath; private final RemoteActionInputFetcher inputFetcher; - private final Map injectedMetadata = new HashMap<>(); + private final HashMap injectedMetadata = new HashMap<>(); + private final HashMap injectedTreeMetadata = new HashMap<>(); @Nullable private MetadataInjector metadataInjector = null; @@ -66,11 +71,15 @@ public class RemoteActionFileSystem extends DelegateFileSystem { PathFragment execRootFragment, String relativeOutputPath, ActionInputMap inputArtifactData, + Iterable outputArtifacts, RemoteActionInputFetcher inputFetcher) { super(localDelegate); this.execRoot = checkNotNull(execRootFragment, "execRootFragment"); this.outputBase = execRoot.getRelative(checkNotNull(relativeOutputPath, "relativeOutputPath")); this.inputArtifactData = checkNotNull(inputArtifactData, "inputArtifactData"); + this.outputArtifactByExecPath = + stream(checkNotNull(outputArtifacts, "outputArtifacts")) + .collect(toImmutableMap(Artifact::getExecPath, a -> a)); this.inputFetcher = checkNotNull(inputFetcher, "inputFetcher"); } @@ -86,12 +95,13 @@ public void updateContext(MetadataInjector metadataInjector) { void injectTree(SpecialArtifact tree, TreeArtifactValue metadata) { checkNotNull(metadataInjector, "metadataInject is null"); + injectedTreeMetadata.put(tree.getExecPath(), metadata); + for (Map.Entry entry : metadata.getChildValues().entrySet()) { FileArtifactValue childMetadata = entry.getValue(); if (childMetadata instanceof RemoteFileArtifactValue) { - TreeFileArtifact child = entry.getKey(); - injectedMetadata.put(child.getExecPath(), (RemoteFileArtifactValue) childMetadata); + injectedMetadata.put(entry.getKey().getExecPath(), (RemoteFileArtifactValue) childMetadata); } } @@ -227,14 +237,76 @@ protected PathFragment readSymbolicLink(PathFragment path) throws IOException { @Override protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment) throws IOException { - /* - * TODO(buchgr): Optimize the case where we are creating a symlink to a remote output. This does - * add a non-trivial amount of complications though (as symlinks tend to do). - */ - downloadFileIfRemote(execRoot.getRelative(targetFragment)); + FileStatus stat = statNullable(linkPath, /* followSymlinks= */ false); + if (stat != null) { + throw new IOException(linkPath + " (File exists)"); + } + + // If the output is a regular file or tree artifact, it has "copy" semantics, i.e., its contents + // are identical to the target artifact. If the target artifact is stored remotely, then, + // instead of creating a symlink in the local filesystem (which would necessarily dangle), we + // inject a special type of metadata that is later used by the input prefetcher to download the + // original artifact and put the symlink in place. + Artifact output = outputArtifactByExecPath.get(linkPath.relativeTo(execRoot)); + if (output != null) { + if (output.isTreeArtifact()) { + TreeArtifactValue metadata = getRemoteInputTreeMetadata(targetFragment); + if (metadata != null) { + // The archived representation is never used on open Source Bazel. + checkState( + metadata.getArchivedRepresentation().isEmpty(), + "symlink to archived tree artifact is not supported"); + checkState(targetFragment.isAbsolute()); + injectTree( + (SpecialArtifact) output, + createTreeArtifactValueForSymlink( + (SpecialArtifact) output, metadata, targetFragment.relativeTo(execRoot))); + return; + } + } else if (!output.isSymlink()) { + RemoteFileArtifactValue metadata = getRemoteInputMetadata(targetFragment); + if (metadata != null) { + checkState(targetFragment.isAbsolute()); + injectFile( + output, + createRemoteFileArtifactValueForSymlink( + metadata, targetFragment.relativeTo(execRoot))); + return; + } + } + } + super.createSymbolicLink(linkPath, targetFragment); } + private RemoteFileArtifactValue createRemoteFileArtifactValueForSymlink( + RemoteFileArtifactValue original, PathFragment targetPath) { + return new RemoteFileArtifactValue( + original.getDigest(), + original.getSize(), + original.getLocationIndex(), + original.getActionId(), + // Avoid a double indirection when the original artifact is already a symlink. + original.getSymlinkTargetExecPath().orElse(targetPath)); + } + + private TreeArtifactValue createTreeArtifactValueForSymlink( + SpecialArtifact output, TreeArtifactValue original, PathFragment targetPath) { + TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder(output); + for (Map.Entry entry : + original.getChildValues().entrySet()) { + PathFragment parentRelativePath = entry.getKey().getParentRelativePath(); + builder.putChild( + TreeFileArtifact.createTreeOutput(output, parentRelativePath), + createRemoteFileArtifactValueForSymlink( + (RemoteFileArtifactValue) entry.getValue(), + targetPath.getRelative(parentRelativePath))); + } + // Avoid a double indirection when the original artifact is already a symlink. + builder.setSymlinkTargetExecPath(original.getSymlinkTargetExecPath().orElse(targetPath)); + return builder.build(); + } + // -------------------- Implementations based on stat() -------------------- @Override @@ -370,10 +442,22 @@ private RemoteFileArtifactValue getRemoteInputMetadata(PathFragment path) { if (m != null && m.isRemote()) { return (RemoteFileArtifactValue) m; } - return injectedMetadata.get(execPath); } + @Nullable + private TreeArtifactValue getRemoteInputTreeMetadata(PathFragment path) { + if (!path.startsWith(outputBase)) { + return null; + } + PathFragment execPath = path.relativeTo(execRoot); + TreeArtifactValue m = inputArtifactData.getTreeMetadata(execPath); + if (m != null && m.isEntirelyRemote()) { + return m; + } + return injectedTreeMetadata.get(execPath); + } + private void downloadFileIfRemote(PathFragment path) throws IOException { FileArtifactValue m = getRemoteInputMetadata(path); if (m != null) { @@ -391,7 +475,7 @@ private void downloadFileIfRemote(PathFragment path) throws IOException { * -------------------- TODO(buchgr): Not yet implemented -------------------- * * The below methods have not (yet) been properly implemented due to time constraints mostly and - * with little risk as they currently don't seem to be used by native actions in Bazel. However, + * with little risk as they currently don't seem to be used by internal actions in Bazel. However, * before making the --experimental_remote_download_outputs flag non-experimental we should make * sure to fully implement this file system. */ 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 3f42018ad7fb4a..20bc2a14903e2a 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 @@ -70,6 +70,7 @@ public FileSystem createActionFileSystem( execRootFragment, relativeOutputPath, inputArtifactData, + outputArtifacts, actionInputFetcher); } @@ -142,7 +143,12 @@ public ArtifactPathResolver createPathResolverForArtifactValues( Map> filesets) { FileSystem remoteFileSystem = new RemoteActionFileSystem( - fileSystem, execRoot, relativeOutputPath, actionInputMap, actionInputFetcher); + fileSystem, + execRoot, + relativeOutputPath, + actionInputMap, + ImmutableList.of(), + actionInputFetcher); return ArtifactPathResolver.createPathResolver(remoteFileSystem, fileSystem.getPath(execRoot)); } } 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 77104d996b1ee3..bd57676b2a9a2d 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 @@ -293,6 +293,25 @@ public FileArtifactValue getMetadata(ActionInput actionInput) throws IOException return checkExists(value, artifact); } + @Override + @Nullable + public TreeArtifactValue getTreeMetadata(SpecialArtifact artifact) throws IOException { + checkArgument(artifact.isTreeArtifact()); + + if (!isKnownOutput(artifact)) { + TreeArtifactValue value = inputArtifactData.getTreeMetadata(artifact); + if (value != null) { + return checkExists(value, artifact); + } + checkState(forInputDiscovery, "%s is not present in declared outputs: %s", artifact, outputs); + return null; + } + + // TODO(tjgq): We probably should do the same as getMetadata. + + return null; + } + @Override public ActionInput getInput(String execPath) { return inputArtifactData.getInput(execPath); 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 6ba4671e0d5e90..e669705ad9bb95 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 @@ -1783,6 +1783,15 @@ public FileArtifactValue getMetadata(ActionInput input) throws IOException { : perBuildFileCache.getMetadata(input); } + @Override + public TreeArtifactValue getTreeMetadata(SpecialArtifact artifact) throws IOException { + Preconditions.checkArgument(artifact.isTreeArtifact()); + TreeArtifactValue metadata = perActionCache.getTreeMetadata(artifact); + return (metadata != null) && (metadata != TreeArtifactValue.MISSING_TREE_ARTIFACT) + ? metadata + : perBuildFileCache.getTreeMetadata(artifact); + } + @Override public ActionInput getInput(String execPath) { ActionInput input = perActionCache.getInput(execPath); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java index 117cca18c371c5..feaf38e413dda0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java @@ -178,6 +178,7 @@ public static ArchivedRepresentation create( EMPTY_MAP, 0L, /*archivedRepresentation=*/ null, + /*symlinkTargetExecPath=*/ null, /*entirelyRemote=*/ false); private final byte[] digest; @@ -190,6 +191,15 @@ public static ArchivedRepresentation create( */ @Nullable private final ArchivedRepresentation archivedRepresentation; + /** + * Optional symlink target path. + * + *

If set, this artifact is a copy of another artifact. It is materialized in the local + * filesystem as a symlink to the original artifact, whose contents live at this location. This is + * used to implement zero-cost copies of remotely stored artifacts. + */ + @Nullable private final PathFragment symlinkTargetExecPath; + private final boolean entirelyRemote; private TreeArtifactValue( @@ -197,11 +207,13 @@ private TreeArtifactValue( ImmutableSortedMap childData, long totalChildSize, @Nullable ArchivedRepresentation archivedRepresentation, + @Nullable PathFragment symlinkTargetExecPath, boolean entirelyRemote) { this.digest = digest; this.childData = childData; this.totalChildSize = totalChildSize; this.archivedRepresentation = archivedRepresentation; + this.symlinkTargetExecPath = symlinkTargetExecPath; this.entirelyRemote = entirelyRemote; } @@ -233,6 +245,11 @@ public Optional getArchivedRepresentation() { return Optional.ofNullable(archivedRepresentation); } + /** Return symlinkTarget path (if present). */ + public Optional getSymlinkTargetExecPath() { + return Optional.ofNullable(symlinkTargetExecPath); + } + @VisibleForTesting @Nullable public ArchivedTreeArtifact getArchivedArtifactForTesting() { @@ -313,7 +330,12 @@ public String toString() { private static TreeArtifactValue createMarker(String toStringRepresentation) { return new TreeArtifactValue( - null, EMPTY_MAP, 0L, /*archivedRepresentation=*/ null, /*entirelyRemote=*/ false) { + null, + EMPTY_MAP, + 0L, + /*archivedRepresentation=*/ null, + /*symlinkTargetExecPath=*/ null, + /*entirelyRemote=*/ false) { @Override public ImmutableSet getChildren() { throw new UnsupportedOperationException(toString()); @@ -448,6 +470,7 @@ public static final class Builder { private final ImmutableSortedMap.Builder childData = childDataBuilder(); private ArchivedRepresentation archivedRepresentation; + private PathFragment symlinkTargetExecPath; private final SpecialArtifact parent; Builder(SpecialArtifact parent) { @@ -514,11 +537,18 @@ public Builder setArchivedRepresentation(ArchivedRepresentation archivedRepresen return this; } + public Builder setSymlinkTargetExecPath(PathFragment symlinkTargetExecPath) { + this.symlinkTargetExecPath = symlinkTargetExecPath; + return this; + } + /** Builds the final {@link TreeArtifactValue}. */ public TreeArtifactValue build() { ImmutableSortedMap finalChildData = childData.buildOrThrow(); - if (finalChildData.isEmpty() && archivedRepresentation == null) { + if (finalChildData.isEmpty() + && archivedRepresentation == null + && symlinkTargetExecPath == null) { return EMPTY; } @@ -550,6 +580,7 @@ public TreeArtifactValue build() { finalChildData, totalChildSize, archivedRepresentation, + symlinkTargetExecPath, entirelyRemote); } } diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java index a7d26a88619bc1..5ead997ad4b498 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java @@ -134,6 +134,10 @@ public void putTreeArtifact_addsTreeArtifactAndAllChildren() { assertContainsEqualMetadata(tree, treeValue.getMetadata()); assertContainsSameInstance(child1, child1Metadata); assertContainsSameInstance(child2, child2Metadata); + + assertThat(map.getMetadata(tree)).isEqualTo(treeValue.getMetadata()); + assertThat(map.getTreeMetadata(tree.getExecPath())).isEqualTo(treeValue); + assertThat(map.getTreeMetadata(child1.getExecPath())).isNull(); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java index 7e8be67a8ee5b6..d416fdfbe9acc4 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java @@ -36,6 +36,7 @@ import java.nio.charset.StandardCharsets; import java.util.Map; import java.util.Optional; +import javax.annotation.Nullable; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -194,7 +195,8 @@ private FileArtifactValue createLocalMetadata(Artifact artifact, String content) return FileArtifactValue.createForTesting(artifact.getPath()); } - private RemoteFileArtifactValue createRemoteMetadata(Artifact artifact, String content) { + private RemoteFileArtifactValue createRemoteMetadata( + Artifact artifact, String content, @Nullable PathFragment symlinkTargetExecPath) { byte[] bytes = content.getBytes(StandardCharsets.UTF_8); byte[] digest = artifact @@ -204,7 +206,11 @@ private RemoteFileArtifactValue createRemoteMetadata(Artifact artifact, String c .getHashFunction() .hashBytes(bytes) .asBytes(); - return new RemoteFileArtifactValue(digest, bytes.length, 1, "action-id"); + return new RemoteFileArtifactValue(digest, bytes.length, 1, "action-id", symlinkTargetExecPath); + } + + private RemoteFileArtifactValue createRemoteMetadata(Artifact artifact, String content) { + return createRemoteMetadata(artifact, content, /* symlinkTargetExecPath= */ null); } private TreeArtifactValue createTreeMetadata( @@ -239,6 +245,21 @@ public void putAndGet_savesRemoteFileMetadata() { assertThat(entry.getOutputFile(artifact)).isEqualTo(metadata); } + @Test + public void putAndGet_savesRemoteFileMetadata_withSymlinkTargetExecPath() { + String key = "key"; + ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + Artifact artifact = ActionsTestUtil.DUMMY_ARTIFACT; + RemoteFileArtifactValue metadata = + createRemoteMetadata(artifact, "content", PathFragment.create("/execroot/some/path")); + entry.addOutputFile(artifact, metadata, /*saveFileMetadata=*/ true); + + cache.put(key, entry); + entry = cache.get(key); + + assertThat(entry.getOutputFile(artifact)).isEqualTo(metadata); + } + @Test public void putAndGet_ignoresLocalFileMetadata() throws IOException { String key = "key"; diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java index 50ad0bda12c74d..691dc10877ca94 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java @@ -109,6 +109,7 @@ import com.google.protobuf.ByteString; import java.io.IOException; import java.io.OutputStream; +import java.io.UnsupportedEncodingException; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; @@ -267,6 +268,20 @@ public static SpecialArtifact createTreeArtifactWithGeneratingAction( return treeArtifact; } + public static SpecialArtifact createTreeArtifactWithGeneratingAction( + ArtifactRoot root, String path) { + return createTreeArtifactWithGeneratingAction( + root, root.getExecPath().getRelative(PathFragment.create(path))); + } + + public static SpecialArtifact createUnresolvedSymlinkArtifact(ArtifactRoot root, String path) { + return SpecialArtifact.create( + root, + root.getExecPath().getRelative(PathFragment.create(path)), + NULL_ARTIFACT_OWNER, + SpecialArtifactType.UNRESOLVED_SYMLINK); + } + public static void assertNoArtifactEndingWith(RuleConfiguredTarget target, String path) { Pattern endPattern = Pattern.compile(path + "$"); for (ActionAnalysisMetadata action : target.getActions()) { @@ -999,6 +1014,11 @@ public FileArtifactValue getMetadata(ActionInput input) throws IOException { throw new UnsupportedOperationException(); } + @Override + public TreeArtifactValue getTreeMetadata(SpecialArtifact artifact) throws IOException { + throw new UnsupportedEncodingException(); + } + @Override public ActionInput getInput(String execPath) { throw new UnsupportedOperationException(); diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/BUILD b/src/test/java/com/google/devtools/build/lib/exec/util/BUILD index 1190919374363b..8f005070b43f97 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/exec/util/BUILD @@ -45,6 +45,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_registry", "//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_resolver", "//src/main/java/com/google/devtools/build/lib/exec:symlink_tree_strategy", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/common/options", diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/FakeActionInputFileCache.java b/src/test/java/com/google/devtools/build/lib/exec/util/FakeActionInputFileCache.java index 12e9d5dcf6729a..9ea1288bfa1cf5 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/FakeActionInputFileCache.java +++ b/src/test/java/com/google/devtools/build/lib/exec/util/FakeActionInputFileCache.java @@ -15,8 +15,10 @@ import com.google.common.base.Preconditions; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.MetadataProvider; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import java.io.IOException; import java.util.HashMap; import java.util.Map; @@ -36,6 +38,11 @@ public FileArtifactValue getMetadata(ActionInput input) throws IOException { return Preconditions.checkNotNull(inputs.get(input)); } + @Override + public TreeArtifactValue getTreeMetadata(SpecialArtifact artifact) throws IOException { + throw new UnsupportedOperationException(); + } + @Override public ActionInput getInput(String execPath) { throw new UnsupportedOperationException(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java index 96a3f87d61f33f..904481af610184 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java @@ -24,11 +24,13 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.hash.HashCode; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ActionInputMap; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; @@ -42,12 +44,15 @@ import com.google.devtools.build.lib.remote.common.BulkTransferException; import com.google.devtools.build.lib.remote.util.StaticMetadataProvider; import com.google.devtools.build.lib.remote.util.TempPathGenerator; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; +import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import java.io.File; import java.io.IOException; import java.util.HashMap; import java.util.Map; @@ -55,6 +60,7 @@ import java.util.concurrent.Semaphore; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Supplier; +import javax.annotation.Nullable; import org.junit.Before; import org.junit.Test; @@ -80,15 +86,22 @@ public void setUp() throws IOException { } protected Artifact createRemoteArtifact( - Artifact a, + String pathFragment, String contents, + @Nullable PathFragment symlinkTargetExecPath, Map metadata, Map cas) { + Path p = artifactRoot.getRoot().getRelative(pathFragment); + Artifact a = ActionsTestUtil.createArtifact(artifactRoot, p); byte[] contentsBytes = contents.getBytes(UTF_8); HashCode hashCode = HASH_FUNCTION.getHashFunction().hashBytes(contentsBytes); - FileArtifactValue f = + RemoteFileArtifactValue f = new RemoteFileArtifactValue( - hashCode.asBytes(), contentsBytes.length, /* locationIndex= */ 1, "action-id"); + hashCode.asBytes(), + contentsBytes.length, + /* locationIndex= */ 1, + "action-id", + symlinkTargetExecPath); metadata.put(a, f); cas.put(hashCode, contentsBytes); return a; @@ -99,9 +112,51 @@ protected Artifact createRemoteArtifact( String contents, Map metadata, Map cas) { - Path p = artifactRoot.getRoot().getRelative(pathFragment); - Artifact a = ActionsTestUtil.createArtifact(artifactRoot, p); - return createRemoteArtifact(a, contents, metadata, cas); + return createRemoteArtifact( + pathFragment, contents, /* symlinkTargetExecPath= */ null, metadata, cas); + } + + protected Pair> createRemoteTreeArtifact( + String pathFragment, + Map contentMap, + @Nullable PathFragment symlinkTargetExecPath, + Map metadata, + Map treeMetadata, + Map cas) + throws IOException { + SpecialArtifact parent = createTreeArtifactWithGeneratingAction(artifactRoot, pathFragment); + parent.getPath().createDirectoryAndParents(); + parent.getPath().chmod(0555); + TreeArtifactValue.Builder treeBuilder = TreeArtifactValue.newBuilder(parent); + for (Map.Entry entry : contentMap.entrySet()) { + byte[] contentsBytes = entry.getValue().getBytes(UTF_8); + HashCode hashCode = HASH_FUNCTION.getHashFunction().hashBytes(contentsBytes); + TreeFileArtifact child = + TreeFileArtifact.createTreeOutput(parent, PathFragment.create(entry.getKey())); + FileArtifactValue childValue = + new RemoteFileArtifactValue( + hashCode.asBytes(), contentsBytes.length, /* locationIndex= */ 1, "action-id"); + treeBuilder.putChild(child, childValue); + metadata.put(child, childValue); + cas.put(hashCode, contentsBytes); + } + if (symlinkTargetExecPath != null) { + treeBuilder.setSymlinkTargetExecPath(symlinkTargetExecPath); + } + TreeArtifactValue treeValue = treeBuilder.build(); + treeMetadata.put(parent, treeValue); + return Pair.of(parent, treeValue.getChildren().asList()); + } + + protected Pair> createRemoteTreeArtifact( + String pathFragment, + Map contentMap, + Map metadata, + Map treeMetadata, + Map cas) + throws IOException { + return createRemoteTreeArtifact( + pathFragment, contentMap, /* symlinkTargetExecPath= */ null, metadata, treeMetadata, cas); } protected abstract AbstractActionInputPrefetcher createPrefetcher(Map cas); @@ -121,44 +176,98 @@ public void prefetchFiles_downloadRemoteFiles() throws Exception { assertReadableNonWritableAndExecutable(a1.getPath()); assertThat(FileSystemUtils.readContent(a2.getPath(), UTF_8)).isEqualTo("fizz buzz"); assertReadableNonWritableAndExecutable(a2.getPath()); - assertThat(prefetcher.downloadedFiles()).hasSize(2); - assertThat(prefetcher.downloadedFiles()).containsAtLeast(a1.getPath(), a2.getPath()); + assertThat(prefetcher.downloadedFiles()).containsExactly(a1.getPath(), a2.getPath()); assertThat(prefetcher.downloadsInProgress()).isEmpty(); } @Test - public void prefetchFiles_downloadTrees() throws Exception { + public void prefetchFiles_downloadRemoteFiles_withSymlinkTargetExecPath() throws Exception { Map metadata = new HashMap<>(); Map cas = new HashMap<>(); - SpecialArtifact parent = - createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("root/dir")); - parent.getPath().createDirectoryAndParents(); - parent.getPath().chmod(0555); - Artifact a1 = - createRemoteArtifact( - TreeFileArtifact.createTreeOutput(parent, "file1"), "content1", metadata, cas); - Artifact a2 = - createRemoteArtifact( - TreeFileArtifact.createTreeOutput(parent, "nested_dir/file2"), - "content2", - metadata, - cas); + PathFragment targetExecPath = artifactRoot.getExecPath().getChild("target"); + Artifact a = createRemoteArtifact("file", "hello world", targetExecPath, metadata, cas); MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); wait(prefetcher.prefetchFiles(metadata.keySet(), metadataProvider)); + assertThat(a.getPath().isSymbolicLink()).isTrue(); + assertThat(a.getPath().readSymbolicLink()) + .isEqualTo(execRoot.getRelative(targetExecPath).asFragment()); + assertThat(FileSystemUtils.readContent(a.getPath(), UTF_8)).isEqualTo("hello world"); + assertThat(prefetcher.downloadedFiles()) + .containsExactly(a.getPath(), execRoot.getRelative(targetExecPath)); + assertThat(prefetcher.downloadsInProgress()).isEmpty(); + } + + @Test + public void prefetchFiles_downloadRemoteTrees() throws Exception { + Map metadata = new HashMap<>(); + Map treeMetadata = new HashMap<>(); + Map cas = new HashMap<>(); + Pair> tree = + createRemoteTreeArtifact( + "dir", + ImmutableMap.of("file1", "content1", "nested_dir/file2", "content2"), + metadata, + treeMetadata, + cas); + SpecialArtifact parent = tree.getFirst(); + Artifact firstChild = tree.getSecond().get(0); + Artifact secondChild = tree.getSecond().get(1); + + MetadataProvider metadataProvider = new StaticMetadataProvider(metadata, treeMetadata); + AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); + + wait(prefetcher.prefetchFiles(tree.getSecond(), metadataProvider)); + assertReadableNonWritableAndExecutable(parent.getPath()); - assertThat(FileSystemUtils.readContent(a1.getPath(), UTF_8)).isEqualTo("content1"); - assertReadableNonWritableAndExecutable(a1.getPath()); - assertThat(FileSystemUtils.readContent(a2.getPath(), UTF_8)).isEqualTo("content2"); - assertReadableNonWritableAndExecutable(a2.getPath()); - assertThat(prefetcher.downloadedFiles()).hasSize(1); + assertThat(FileSystemUtils.readContent(firstChild.getPath(), UTF_8)).isEqualTo("content1"); + assertReadableNonWritableAndExecutable(firstChild.getPath()); + assertThat(FileSystemUtils.readContent(secondChild.getPath(), UTF_8)).isEqualTo("content2"); + assertReadableNonWritableAndExecutable(secondChild.getPath()); assertThat(prefetcher.downloadedFiles()).containsExactly(parent.getPath()); assertThat(prefetcher.downloadsInProgress()).isEmpty(); assertReadableNonWritableAndExecutable(parent.getPath().getRelative("nested_dir")); } + @Test + public void prefetchFiles_downloadRemoteTrees_withSymlinkTargetExecPath() throws Exception { + Map metadata = new HashMap<>(); + Map treeMetadata = new HashMap<>(); + Map cas = new HashMap<>(); + PathFragment targetExecPath = artifactRoot.getExecPath().getChild("target"); + Pair> tree = + createRemoteTreeArtifact( + "dir", + ImmutableMap.of("file1", "content1", "nested_dir/file2", "content2"), + targetExecPath, + metadata, + treeMetadata, + cas); + SpecialArtifact parent = tree.getFirst(); + Artifact firstChild = tree.getSecond().get(0); + Artifact secondChild = tree.getSecond().get(1); + + MetadataProvider metadataProvider = new StaticMetadataProvider(metadata, treeMetadata); + AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); + + wait(prefetcher.prefetchFiles(tree.getSecond(), metadataProvider)); + + assertThat(parent.getPath().isSymbolicLink()).isTrue(); + assertThat(parent.getPath().readSymbolicLink()) + .isEqualTo(execRoot.getRelative(targetExecPath).asFragment()); + assertReadableNonWritableAndExecutable(parent.getPath()); + assertThat(FileSystemUtils.readContent(firstChild.getPath(), UTF_8)).isEqualTo("content1"); + assertReadableNonWritableAndExecutable(firstChild.getPath()); + assertThat(FileSystemUtils.readContent(secondChild.getPath(), UTF_8)).isEqualTo("content2"); + assertReadableNonWritableAndExecutable(secondChild.getPath()); + assertThat(prefetcher.downloadedFiles()) + .containsExactly(parent.getPath(), execRoot.getRelative(targetExecPath)); + assertThat(prefetcher.downloadsInProgress()).isEmpty(); + assertReadableNonWritableAndExecutable(parent.getPath().getRelative("nested_dir")); + } + @Test public void prefetchFiles_missingFiles_fails() throws Exception { Map metadata = new HashMap<>(); 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 ea01ccbce96776..9d0411ebcc121b 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -78,6 +78,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/remote/util", "//src/main/java/com/google/devtools/build/lib/runtime/commands", "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", + "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", "//src/main/java/com/google/devtools/build/lib/util:exit_code", "//src/main/java/com/google/devtools/build/lib/util/io", 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 d36ab2713919c0..221a8af1801286 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 @@ -360,6 +360,292 @@ public void changeOutputMode_invalidateActions() throws Exception { } } + @Test + public void symlinkToSourceFile() throws Exception { + addRemoteModeOptions(); + addOutputModeOptions(); + + write( + "a/defs.bzl", + "def _impl(ctx):", + " if ctx.attr.chain_length < 1:", + " fail('chain_length must be > 0')", + "", + " file = ctx.file.target", + "", + " for i in range(ctx.attr.chain_length):", + " sym = ctx.actions.declare_file(ctx.label.name + '.sym' + str(i))", + " ctx.actions.symlink(output = sym, target_file = file)", + " file = sym", + "", + " out = ctx.actions.declare_file(ctx.label.name + '.out')", + " ctx.actions.run_shell(", + " inputs = [sym],", + " outputs = [out],", + " command = '[[ hello == $(cat $1) ]] && touch $2',", + " arguments = [sym.path, out.path],", + " execution_requirements = {'no-remote': ''} if ctx.attr.local else {},", + " )", + "", + " return DefaultInfo(files = depset([out]))", + "", + "my_rule = rule(", + " implementation = _impl,", + " attrs = {", + " 'target': attr.label(allow_single_file = True),", + " 'chain_length': attr.int(),", + " 'local': attr.bool(),", + " },", + ")"); + + write( + "a/BUILD", + "load(':defs.bzl', 'my_rule')", + "", + "my_rule(name = 'one_local', target = 'src.txt', local = True, chain_length = 1)", + "my_rule(name = 'two_local', target = 'src.txt', local = True, chain_length = 2)", + "my_rule(name = 'one_remote', target = 'src.txt', local = False, chain_length = 1)", + "my_rule(name = 'two_remote', target = 'src.txt', local = False, chain_length =" + " 2)"); + + write("a/src.txt", "hello"); + + buildTarget("//a:one_local"); + buildTarget("//a:two_local"); + buildTarget("//a:one_remote"); + buildTarget("//a:two_remote"); + } + + @Test + public void symlinkToGeneratedFile() throws Exception { + addRemoteModeOptions(); + addOutputModeOptions(); + + write( + "a/defs.bzl", + "def _impl(ctx):", + " if ctx.attr.chain_length < 1:", + " fail('chain_length must be > 0')", + "", + " file = ctx.actions.declare_file(ctx.label.name + '.file')", + // Use ctx.actions.run_shell instead of ctx.actions.write, so that it runs remotely. + " ctx.actions.run_shell(", + " outputs = [file],", + " command = 'echo hello > $1',", + " arguments = [file.path],", + " )", + "", + " for i in range(ctx.attr.chain_length):", + " sym = ctx.actions.declare_file(ctx.label.name + '.sym' + str(i))", + " ctx.actions.symlink(output = sym, target_file = file)", + " file = sym", + "", + " out = ctx.actions.declare_file(ctx.label.name + '.out')", + " ctx.actions.run_shell(", + " inputs = [sym],", + " outputs = [out],", + " command = '[[ hello == $(cat $1) ]] && touch $2',", + " arguments = [sym.path, out.path],", + " execution_requirements = {'no-remote': ''} if ctx.attr.local else {},", + " )", + "", + " return DefaultInfo(files = depset([out]))", + "", + "my_rule = rule(", + " implementation = _impl,", + " attrs = {", + " 'chain_length': attr.int(),", + " 'local': attr.bool(),", + " },", + ")"); + + write( + "a/BUILD", + "load(':defs.bzl', 'my_rule')", + "", + "my_rule(name = 'one_local', local = True, chain_length = 1)", + "my_rule(name = 'two_local', local = True, chain_length = 2)", + "my_rule(name = 'one_remote', local = False, chain_length = 1)", + "my_rule(name = 'two_remote', local = False, chain_length = 2)"); + + buildTarget("//a:one_local"); + buildTarget("//a:two_local"); + buildTarget("//a:one_remote"); + buildTarget("//a:two_remote"); + } + + @Test + public void symlinkToDirectory() throws Exception { + addRemoteModeOptions(); + addOutputModeOptions(); + + write( + "a/defs.bzl", + "def _impl(ctx):", + " if ctx.attr.chain_length < 1:", + " fail('chain_length must be > 0')", + "", + " dir = ctx.actions.declare_directory(ctx.label.name + '.dir')", + " ctx.actions.run_shell(", + " outputs = [dir],", + " command = 'mkdir -p $1/some/path && echo hello > $1/some/path/inside.txt',", + " arguments = [dir.path],", + " )", + "", + " for i in range(ctx.attr.chain_length):", + " sym = ctx.actions.declare_directory(ctx.label.name + '.sym' + str(i))", + " ctx.actions.symlink(output = sym, target_file = dir)", + " dir = sym", + "", + " out = ctx.actions.declare_file(ctx.label.name + '.out')", + " ctx.actions.run_shell(", + " inputs = [sym],", + " outputs = [out],", + " command = '[[ hello == $(cat $1/some/path/inside.txt) ]] && touch $2',", + " arguments = [sym.path, out.path],", + " execution_requirements = {'no-remote': ''} if ctx.attr.local else {},", + " )", + "", + " return DefaultInfo(files = depset([out]))", + "", + "my_rule = rule(", + " implementation = _impl,", + " attrs = {", + " 'chain_length': attr.int(),", + " 'local': attr.bool()", + " },", + ")"); + + write( + "a/BUILD", + "load(':defs.bzl', 'my_rule')", + "", + "my_rule(name = 'one_local', local = True, chain_length = 1)", + "my_rule(name = 'two_local', local = True, chain_length = 2)", + "my_rule(name = 'one_remote', local = False, chain_length = 1)", + "my_rule(name = 'two_remote', local = False, chain_length = 2)"); + + buildTarget("//a:one_local"); + buildTarget("//a:two_local"); + buildTarget("//a:one_remote"); + buildTarget("//a:two_remote"); + } + + @Test + public void symlinkToNestedFile() throws Exception { + addRemoteModeOptions(); + addOutputModeOptions(); + + write( + "a/defs.bzl", + "def _impl(ctx):", + " if ctx.attr.chain_length < 1:", + " fail('chain_length must be > 0')", + "", + " dir = ctx.actions.declare_directory(ctx.label.name + '.dir')", + " file = ctx.actions.declare_file(ctx.label.name + '.dir/some/path/inside.txt')", + " ctx.actions.run_shell(", + " outputs = [dir, file],", + " command = 'mkdir -p $1/some/path && echo hello > $1/some/path/inside.txt',", + " arguments = [dir.path],", + " )", + "", + " for i in range(ctx.attr.chain_length):", + " sym = ctx.actions.declare_file(ctx.label.name + '.sym' + str(i))", + " ctx.actions.symlink(output = sym, target_file = file)", + " file = sym", + "", + " out = ctx.actions.declare_file(ctx.label.name + '.out')", + " ctx.actions.run_shell(", + " inputs = [sym],", + " outputs = [out],", + " command = '[[ hello == $(cat $1) ]] && touch $2',", + " arguments = [sym.path, out.path],", + " execution_requirements = {'no-remote': ''} if ctx.attr.local else {},", + " )", + "", + " return DefaultInfo(files = depset([out]))", + "", + "my_rule = rule(", + " implementation = _impl,", + " attrs = {", + " 'chain_length': attr.int(),", + " 'local': attr.bool(),", + " },", + ")"); + + write( + "a/BUILD", + "load(':defs.bzl', 'my_rule')", + "", + "my_rule(name = 'one_local', local = True, chain_length = 1)", + "my_rule(name = 'two_local', local = True, chain_length = 2)", + "my_rule(name = 'one_remote', local = False, chain_length = 1)", + "my_rule(name = 'two_remote', local = False, chain_length = 2)"); + + buildTarget("//a:one_local"); + buildTarget("//a:two_local"); + buildTarget("//a:one_remote"); + buildTarget("//a:two_remote"); + } + + @Test + public void symlinkToNestedDirectory() throws Exception { + addRemoteModeOptions(); + addOutputModeOptions(); + + write( + "a/defs.bzl", + "def _impl(ctx):", + " if ctx.attr.chain_length < 1:", + " fail('chain_length must be > 0')", + "", + " dir = ctx.actions.declare_directory(ctx.label.name + '.dir')", + " subdir = ctx.actions.declare_directory(ctx.label.name + '.dir/some/path')", + " ctx.actions.run_shell(", + " outputs = [dir, subdir],", + " command = 'mkdir -p $1/some/path && echo hello > $1/some/path/inside.txt',", + " arguments = [dir.path],", + " )", + "", + " for i in range(ctx.attr.chain_length):", + " sym = ctx.actions.declare_directory(ctx.label.name + '.sym' + str(i))", + " ctx.actions.symlink(output = sym, target_file = subdir)", + " subdir = sym", + "", + " out = ctx.actions.declare_file(ctx.label.name + '.out')", + " ctx.actions.run_shell(", + " inputs = [sym],", + " outputs = [out],", + " command = '[[ hello == $(cat $1/inside.txt) ]] && touch $2',", + " arguments = [sym.path, out.path],", + " execution_requirements = {'no-remote': ''} if ctx.attr.local else {},", + " )", + "", + " return DefaultInfo(files = depset([out]))", + "", + "my_rule = rule(", + " implementation = _impl,", + " attrs = {", + " 'chain_length': attr.int(),", + " 'local': attr.bool(),", + " },", + ")"); + + write( + "a/BUILD", + "load(':defs.bzl', 'my_rule')", + "", + "my_rule(name = 'one_local', local = True, chain_length = 1)", + "my_rule(name = 'two_local', local = True, chain_length = 2)", + "my_rule(name = 'one_remote', local = False, chain_length = 1)", + "my_rule(name = 'two_remote', local = False, chain_length = 2)"); + + buildTarget("//a:one_local"); + buildTarget("//a:two_local"); + buildTarget("//a:one_remote"); + buildTarget("//a:two_remote"); + } + private static class ActionEventCollector { private final List actionExecutedEvents = new ArrayList<>(); private final List cachedActionEvents = new ArrayList<>(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java index 8234fe5161c761..f14b37925f7f9c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java @@ -25,6 +25,7 @@ import build.bazel.remote.execution.v2.Digest; import com.google.bytestream.ByteStreamProto.WriteRequest; import com.google.bytestream.ByteStreamProto.WriteResponse; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.eventbus.EventBus; @@ -361,6 +362,7 @@ public void remoteFileShouldNotBeUploaded_actionFs() throws Exception { execRoot.asFragment(), outputRoot.getRoot().asPath().relativeTo(execRoot).getPathString(), outputs, + ImmutableList.of(artifact), actionInputFetcher); Path remotePath = remoteFs.getPath(artifact.getPath().getPathString()); assertThat(remotePath.getFileSystem()).isEqualTo(remoteFs); diff --git a/src/test/java/com/google/devtools/build/lib/remote/FakeActionInputFileCache.java b/src/test/java/com/google/devtools/build/lib/remote/FakeActionInputFileCache.java index a876bc4bacb959..adb1f462d8a70e 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/FakeActionInputFileCache.java +++ b/src/test/java/com/google/devtools/build/lib/remote/FakeActionInputFileCache.java @@ -20,10 +20,12 @@ import com.google.common.collect.HashBiMap; import com.google.common.hash.HashCode; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileContentsProxy; import com.google.devtools.build.lib.actions.MetadataProvider; import com.google.devtools.build.lib.remote.util.DigestUtil; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; @@ -52,6 +54,11 @@ public FileArtifactValue getMetadata(ActionInput input) throws IOException { HashCode.fromString(hexDigest).asBytes(), FileContentsProxy.create(stat), stat.getSize()); } + @Override + public TreeArtifactValue getTreeMetadata(SpecialArtifact artifact) throws IOException { + throw new UnsupportedOperationException(); + } + @Override public ActionInput getInput(String execPath) { throw new UnsupportedOperationException(); 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 061ab11524b7f7..20cdd95de97467 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 @@ -14,34 +14,47 @@ package com.google.devtools.build.lib.remote; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.hash.HashCode; import com.google.common.util.concurrent.Futures; import com.google.devtools.build.lib.actions.ActionInputMap; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; +import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; +import com.google.devtools.build.lib.actions.cache.MetadataInjector; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.Map; +import java.util.Optional; +import javax.annotation.Nullable; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; /** Tests for {@link RemoteActionFileSystem} */ @RunWith(JUnit4.class) @@ -50,6 +63,7 @@ public final class RemoteActionFileSystemTest { private static final DigestHashFunction HASH_FUNCTION = DigestHashFunction.SHA256; private final RemoteActionInputFetcher inputFetcher = mock(RemoteActionInputFetcher.class); + private final MetadataInjector metadataInjector = mock(MetadataInjector.class); private final FileSystem fs = new InMemoryFileSystem(HASH_FUNCTION); private final Path execRoot = fs.getPath("/exec"); private final ArtifactRoot outputRoot = @@ -94,33 +108,77 @@ public void testGetInputStream() throws Exception { } @Test - public void testCreateSymbolicLink() throws InterruptedException, IOException { + public void testCreateSymbolicLink_fileArtifact() throws IOException { // arrange ActionInputMap inputs = new ActionInputMap(1); Artifact remoteArtifact = createRemoteArtifact("remote-file", "remote contents", inputs); - Path symlink = outputRoot.getRoot().getRelative("symlink"); - FileSystem actionFs = newRemoteActionFileSystem(inputs); - doAnswer( - invocationOnMock -> { - FileSystemUtils.writeContent( - remoteArtifact.getPath(), StandardCharsets.UTF_8, "remote contents"); - return Futures.immediateFuture(null); - }) - .when(inputFetcher) - .downloadFile(eq(remoteArtifact.getPath()), eq(inputs.getMetadata(remoteArtifact))); + Artifact outputArtifact = ActionsTestUtil.createArtifact(outputRoot, "out"); + Iterable outputs = ImmutableList.of(outputArtifact); + FileSystem actionFs = newRemoteActionFileSystem(inputs, outputs); // act - Path symlinkActionFs = actionFs.getPath(symlink.getPathString()); - symlinkActionFs.createSymbolicLink(actionFs.getPath(remoteArtifact.getPath().asFragment())); + PathFragment targetPath = remoteArtifact.getPath().asFragment(); + Path symlinkActionFs = actionFs.getPath(outputArtifact.getPath().asFragment()); + symlinkActionFs.createSymbolicLink(actionFs.getPath(targetPath)); // assert + ArgumentCaptor metadataCaptor = + ArgumentCaptor.forClass(FileArtifactValue.class); + verify(metadataInjector).injectFile(eq(outputArtifact), metadataCaptor.capture()); + assertThat(metadataCaptor.getValue()).isInstanceOf(RemoteFileArtifactValue.class); + assertThat(((RemoteFileArtifactValue) metadataCaptor.getValue()).getSymlinkTargetExecPath()) + .hasValue(targetPath.relativeTo(execRoot.asFragment())); + verifyNoMoreInteractions(metadataInjector); + assertThat(symlinkActionFs.getFileSystem()).isSameInstanceAs(actionFs); - verify(inputFetcher) - .downloadFile(eq(remoteArtifact.getPath()), eq(inputs.getMetadata(remoteArtifact))); - String symlinkTargetContents = - FileSystemUtils.readContent(symlinkActionFs, StandardCharsets.UTF_8); - assertThat(symlinkTargetContents).isEqualTo("remote contents"); - verifyNoMoreInteractions(inputFetcher); + } + + /** Returns a remote tree artifact and puts its metadata into the action input map. */ + @Test + public void testCreateSymbolicLink_treeArtifact() throws IOException { + // arrange + ActionInputMap inputs = new ActionInputMap(1); + ImmutableMap contentMap = + ImmutableMap.of("foo", "foo contents", "bar", "bar contents"); + Artifact remoteArtifact = createRemoteTreeArtifact("remote-dir", contentMap, inputs); + SpecialArtifact outputArtifact = + ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, "out"); + Iterable outputs = ImmutableList.of(outputArtifact); + FileSystem actionFs = newRemoteActionFileSystem(inputs, outputs); + + // act + PathFragment targetPath = remoteArtifact.getPath().asFragment(); + Path symlinkActionFs = actionFs.getPath(outputArtifact.getPath().asFragment()); + symlinkActionFs.createSymbolicLink(actionFs.getPath(targetPath)); + + // assert + ArgumentCaptor metadataCaptor = + ArgumentCaptor.forClass(TreeArtifactValue.class); + verify(metadataInjector).injectTree(eq(outputArtifact), metadataCaptor.capture()); + assertThat(metadataCaptor.getValue().getSymlinkTargetExecPath()) + .hasValue(targetPath.relativeTo(execRoot.asFragment())); + verifyNoMoreInteractions(metadataInjector); + + assertThat(symlinkActionFs.getFileSystem()).isSameInstanceAs(actionFs); + } + + @Test + public void testCreateSymbolicLink_symlinkArtifact() throws IOException { + // arrange + ActionInputMap inputs = new ActionInputMap(1); + SpecialArtifact outputArtifact = + ActionsTestUtil.createUnresolvedSymlinkArtifact(outputRoot, "out"); + Iterable outputs = ImmutableList.of(outputArtifact); + FileSystem actionFs = newRemoteActionFileSystem(inputs, outputs); + PathFragment targetPath = PathFragment.create("some/path"); + + // act + Path symlinkActionFs = actionFs.getPath(outputArtifact.getPath().asFragment()); + symlinkActionFs.createSymbolicLink(targetPath); + + // assert + assertThat(symlinkActionFs.getFileSystem()).isSameInstanceAs(actionFs); + assertThat(symlinkActionFs.readSymbolicLink()).isEqualTo(targetPath); } @Test @@ -153,19 +211,27 @@ public void testDeleteLocalFile() throws Exception { } private RemoteActionFileSystem newRemoteActionFileSystem(ActionInputMap inputs) { - return new RemoteActionFileSystem( - fs, - execRoot.asFragment(), - outputRoot.getRoot().asPath().relativeTo(execRoot).getPathString(), - inputs, - inputFetcher); + return newRemoteActionFileSystem(inputs, ImmutableList.of()); + } + + private RemoteActionFileSystem newRemoteActionFileSystem( + ActionInputMap inputs, Iterable outputs) { + RemoteActionFileSystem remoteActionFileSystem = + new RemoteActionFileSystem( + fs, + execRoot.asFragment(), + outputRoot.getRoot().asPath().relativeTo(execRoot).getPathString(), + inputs, + outputs, + inputFetcher); + remoteActionFileSystem.updateContext(metadataInjector); + return remoteActionFileSystem; } /** Returns a remote artifact and puts its metadata into the action input map. */ private Artifact createRemoteArtifact( String pathFragment, String contents, ActionInputMap inputs) { - Path p = outputRoot.getRoot().asPath().getRelative(pathFragment); - Artifact a = ActionsTestUtil.createArtifact(outputRoot, p); + Artifact a = ActionsTestUtil.createArtifact(outputRoot, pathFragment); byte[] b = contents.getBytes(StandardCharsets.UTF_8); HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b); FileArtifactValue f = @@ -174,6 +240,29 @@ private Artifact createRemoteArtifact( return a; } + private SpecialArtifact createRemoteTreeArtifact( + String pathFragment, Map contentMap, ActionInputMap inputs) { + SpecialArtifact a = + ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputRoot, pathFragment); + inputs.putTreeArtifact(a, createTreeArtifactValue(a, contentMap), /* depOwner= */ null); + return a; + } + + private TreeArtifactValue createTreeArtifactValue( + SpecialArtifact a, Map contentMap) { + TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder(a); + int i = 0; + for (Map.Entry entry : contentMap.entrySet()) { + TreeFileArtifact child = TreeFileArtifact.createTreeOutput(a, entry.getKey()); + byte[] b = entry.getValue().getBytes(StandardCharsets.UTF_8); + HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b); + FileArtifactValue childMeta = + new RemoteFileArtifactValue(h.asBytes(), b.length, /* locationIndex= */ ++i, "action-id"); + builder.putChild(child, childMeta); + } + return builder.build(); + } + /** Returns a local artifact and puts its metadata into the action input map. */ private Artifact createLocalArtifact(String pathFragment, String contents, ActionInputMap inputs) throws IOException { diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/BUILD b/src/test/java/com/google/devtools/build/lib/remote/util/BUILD index ef1abf5595e292..8da2e520c0e4be 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/util/BUILD @@ -25,6 +25,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/remote", "//src/main/java/com/google/devtools/build/lib/remote/common", "//src/main/java/com/google/devtools/build/lib/remote/util", + "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/util/io", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/StaticMetadataProvider.java b/src/test/java/com/google/devtools/build/lib/remote/util/StaticMetadataProvider.java index 1d121664d9717a..0e7293f6535097 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/util/StaticMetadataProvider.java +++ b/src/test/java/com/google/devtools/build/lib/remote/util/StaticMetadataProvider.java @@ -15,8 +15,10 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.MetadataProvider; +import com.google.devtools.build.lib.skyframe.TreeArtifactValue; import java.util.Map; import javax.annotation.Nullable; @@ -24,9 +26,17 @@ public final class StaticMetadataProvider implements MetadataProvider { private final ImmutableMap metadata; + private final ImmutableMap treeMetadata; public StaticMetadataProvider(Map metadata) { + this(metadata, /* treeMetadata= */ ImmutableMap.of()); + } + + public StaticMetadataProvider( + Map metadata, + Map treeMetadata) { this.metadata = ImmutableMap.copyOf(metadata); + this.treeMetadata = ImmutableMap.copyOf(treeMetadata); } @Nullable @@ -35,6 +45,12 @@ public FileArtifactValue getMetadata(ActionInput input) { return metadata.get(input); } + @Nullable + @Override + public TreeArtifactValue getTreeMetadata(SpecialArtifact artifact) { + return treeMetadata.get(artifact); + } + @Nullable @Override public ActionInput getInput(String execPath) { diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java index 6223c04c4f00cf..b002179a1c0ffb 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java @@ -109,6 +109,17 @@ public void createsCorrectValueWithArchivedRepresentation() { .hasValue(ArchivedRepresentation.create(archivedTreeArtifact, archivedArtifactMetadata)); } + @Test + public void createsCorrectValueWithSymlinkTargetExecPath() { + PathFragment targetPath = PathFragment.create("some/target/path"); + SpecialArtifact parent = createTreeArtifact("bin/tree"); + + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(parent).setSymlinkTargetExecPath(targetPath).build(); + + assertThat(tree.getSymlinkTargetExecPath()).hasValue(targetPath); + } + @Test public void empty() { TreeArtifactValue emptyTree = TreeArtifactValue.empty(); diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index be8a26abcddada..6f1ee829b583c2 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -15,6 +15,7 @@ package com.google.devtools.build.remote.worker; import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; +import static java.util.stream.Collectors.joining; import build.bazel.remote.execution.v2.Action; import build.bazel.remote.execution.v2.ActionResult; @@ -28,8 +29,10 @@ import build.bazel.remote.execution.v2.Platform.Property; import build.bazel.remote.execution.v2.RequestMetadata; import build.bazel.remote.execution.v2.WaitExecutionRequest; +import com.google.common.base.Charsets; import com.google.common.base.Throwables; import com.google.common.flogger.GoogleLogger; +import com.google.common.io.CharStreams; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; @@ -62,6 +65,7 @@ import java.io.ByteArrayOutputStream; import java.io.File; import java.io.IOException; +import java.io.InputStreamReader; import java.nio.file.FileAlreadyExistsException; import java.time.Duration; import java.time.Instant; @@ -71,6 +75,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.TreeSet; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutionException;