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..244c130786cd86 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,11 @@ private FileArtifactValue getMetadataFromTreeArtifacts(PathFragment execPath) { return entry != null ? entry.getValue() : null; } + @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 f08bdb949f834a..d04d3853dced8f 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 @@ -36,6 +36,7 @@ import java.io.IOException; import java.util.Arrays; import java.util.Objects; +import java.util.Optional; import javax.annotation.Nullable; /** @@ -172,6 +173,17 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue lastKnown) { return true; } + /** + * Optional symlink target path. + * + *

If present, 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.empty(); + } + /** * Marker interface for singleton implementations of this class. * @@ -311,6 +323,16 @@ public static FileArtifactValue createProxy(byte[] digest) { return createForNormalFile(digest, /*proxy=*/ null, /*size=*/ 0); } + /** + * Creates a FileArtifactValue used as a 'proxy' input for a TreeArtifactValue. These are used in + * {@link ActionCacheChecker}. + */ + public static FileArtifactValue createProxyForTreeArtifact( + byte[] digest, boolean isRemote, @Nullable PathFragment symlinkTargetExecPath) { + Preconditions.checkNotNull(digest); + return new TreeArtifactProxyFileArtifactValue(digest, isRemote, symlinkTargetExecPath); + } + private static String bytesToString(byte[] bytes) { return "0x" + BaseEncoding.base16().omitPadding().encode(bytes); } @@ -533,12 +555,100 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue o) { } } + private static final class TreeArtifactProxyFileArtifactValue extends FileArtifactValue { + private final byte[] digest; + private final boolean isRemote; + @Nullable private final PathFragment symlinkTargetExecPath; + + private TreeArtifactProxyFileArtifactValue( + byte[] digest, boolean isRemote, @Nullable PathFragment symlinkTargetExecPath) { + this.digest = digest; + this.isRemote = isRemote; + this.symlinkTargetExecPath = symlinkTargetExecPath; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof TreeArtifactProxyFileArtifactValue)) { + return false; + } + TreeArtifactProxyFileArtifactValue that = (TreeArtifactProxyFileArtifactValue) o; + return Arrays.equals(digest, that.digest) + && Objects.equals(symlinkTargetExecPath, that.symlinkTargetExecPath); + } + + @Override + public int hashCode() { + return Objects.hash(Arrays.hashCode(digest), symlinkTargetExecPath); + } + + @Override + public FileStateType getType() { + // TODO(tjgq): Technically a directory, but I'm unsure about the implications of changing it. + return FileStateType.REGULAR_FILE; + } + + @Override + public byte[] getDigest() { + return digest; + } + + @Override + @Nullable + public FileContentsProxy getContentsProxy() { + return null; + } + + @Override + public long getSize() { + return 0; + } + + @Override + public boolean wasModifiedSinceDigest(Path path) { + return false; + } + + @Override + public long getModifiedTime() { + throw new UnsupportedOperationException(); + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add( + "digest", + digest == null ? "(null)" : BaseEncoding.base16().lowerCase().encode(digest)) + .add("symlinkTargetExecPath", symlinkTargetExecPath) + .toString(); + } + + @Override + protected boolean couldBeModifiedByMetadata(FileArtifactValue o) { + return false; + } + + @Override + public boolean isRemote() { + return isRemote; + } + + @Override + public Optional getSymlinkTargetExecPath() { + return Optional.ofNullable(symlinkTargetExecPath); + } + } + /** Metadata for remotely stored files. */ public static class RemoteFileArtifactValue extends FileArtifactValue { - private final byte[] digest; - private final long size; - private final int locationIndex; - private final String actionId; + protected final byte[] digest; + protected final long size; + protected final int locationIndex; + protected final String actionId; private RemoteFileArtifactValue(byte[] digest, long size, int locationIndex, String actionId) { this.digest = Preconditions.checkNotNull(digest, actionId); @@ -556,6 +666,19 @@ public static RemoteFileArtifactValue create(byte[] digest, long size, int locat return new RemoteFileArtifactValue(digest, size, locationIndex, /* actionId= */ ""); } + public static RemoteFileArtifactValue create( + byte[] digest, + long size, + int locationIndex, + String actionId, + @Nullable PathFragment symlinkTargetExecPath) { + if (symlinkTargetExecPath != null) { + return new SymlinkToRemoteFileArtifactValue( + digest, size, locationIndex, actionId, symlinkTargetExecPath); + } + return new RemoteFileArtifactValue(digest, size, locationIndex, actionId); + } + @Override public boolean equals(Object o) { if (!(o instanceof RemoteFileArtifactValue)) { @@ -602,7 +725,7 @@ public String getActionId() { @Override public long getModifiedTime() { throw new UnsupportedOperationException( - "RemoteFileArifactValue doesn't support getModifiedTime"); + "RemoteFileArtifactValue doesn't support getModifiedTime"); } @Override @@ -626,6 +749,58 @@ public String toString() { .add("digest", bytesToString(digest)) .add("size", size) .add("locationIndex", locationIndex) + .add("actionId", actionId) + .toString(); + } + } + + /** An artifact that should be materialized as a symlink to another remote artifact. */ + public static final class SymlinkToRemoteFileArtifactValue extends RemoteFileArtifactValue { + private PathFragment symlinkTargetExecPath; + + private SymlinkToRemoteFileArtifactValue( + byte[] digest, + long size, + int locationIndex, + String actionId, + PathFragment symlinkTargetExecPath) { + super(digest, size, locationIndex, actionId); + this.symlinkTargetExecPath = Preconditions.checkNotNull(symlinkTargetExecPath); + } + + @Override + public Optional getSymlinkTargetExecPath() { + return Optional.ofNullable(symlinkTargetExecPath); + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof SymlinkToRemoteFileArtifactValue)) { + return false; + } + + SymlinkToRemoteFileArtifactValue that = (SymlinkToRemoteFileArtifactValue) o; + return Arrays.equals(digest, that.digest) + && size == that.size + && locationIndex == that.locationIndex + && Objects.equals(actionId, that.actionId) + && Objects.equals(symlinkTargetExecPath, that.symlinkTargetExecPath); + } + + @Override + public int hashCode() { + return Objects.hash( + Arrays.hashCode(digest), size, locationIndex, actionId, 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/cache/ActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java index ff1b13c85be8e7..65866cacb65082 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/ActionCache.java @@ -109,9 +109,10 @@ final class Entry { public abstract static class SerializableTreeArtifactValue { public static SerializableTreeArtifactValue create( ImmutableMap childValues, - Optional archivedFileValue) { + Optional archivedFileValue, + Optional symlinkTargetExecPath) { return new AutoValue_ActionCache_Entry_SerializableTreeArtifactValue( - childValues, archivedFileValue); + childValues, archivedFileValue, symlinkTargetExecPath); } /** @@ -138,17 +139,25 @@ public static Optional createSerializable( .filter(ar -> ar.archivedFileValue().isRemote()) .map(ar -> (RemoteFileArtifactValue) ar.archivedFileValue()); - if (childValues.isEmpty() && archivedFileValue.isEmpty()) { + Optional symlinkTargetExecPath = treeMetadata.getSymlinkTargetExecPath(); + + if (childValues.isEmpty() + && archivedFileValue.isEmpty() + && symlinkTargetExecPath.isEmpty()) { return Optional.empty(); } - return Optional.of(SerializableTreeArtifactValue.create(childValues, archivedFileValue)); + return Optional.of( + SerializableTreeArtifactValue.create( + childValues, archivedFileValue, symlinkTargetExecPath)); } // A map from parentRelativePath to the file metadata public abstract ImmutableMap childValues(); public abstract Optional archivedFileValue(); + + public abstract Optional symlinkTargetExecPath(); } public Entry(String key, Map usedClientEnv, boolean discoversInputs) { 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 6fc3d4956d65ca..0a7a592d490bd0 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; @@ -75,7 +76,7 @@ public class CompactPersistentActionCache implements ActionCache { private static final int NO_INPUT_DISCOVERY_COUNT = -1; - private static final int VERSION = 13; + private static final int VERSION = 14; private static final class ActionMap extends PersistentMap { private final Clock clock; @@ -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,18 @@ private static RemoteFileArtifactValue decodeRemoteMetadata( String actionId = getStringForIndex(indexer, VarInt.getVarInt(source)); - return RemoteFileArtifactValue.create(digest, size, locationIndex, actionId); + PathFragment symlinkTargetExecPath = null; + int numSymlinkTargetExecPath = VarInt.getVarInt(source); + if (numSymlinkTargetExecPath > 0) { + if (numSymlinkTargetExecPath != 1) { + throw new IOException("Invalid number of symlink target paths"); + } + symlinkTargetExecPath = + PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source))); + } + + return RemoteFileArtifactValue.create( + digest, size, locationIndex, actionId, symlinkTargetExecPath); } /** @return action data encoded as a byte[] array. */ @@ -497,6 +517,7 @@ private static byte[] encode(StringIndexer indexer, ActionCache.Entry entry) thr int maxOutputFilesSize = VarInt.MAX_VARINT_SIZE // entry.getOutputFiles().size() + (VarInt.MAX_VARINT_SIZE // execPath + + (2 * VarInt.MAX_VARINT_SIZE) // symlinkTargetExecPath optional + MAX_REMOTE_METADATA_SIZE) * entry.getOutputFiles().size(); @@ -513,9 +534,12 @@ private static byte[] encode(StringIndexer indexer, ActionCache.Entry entry) thr + MAX_REMOTE_METADATA_SIZE) * value.childValues().size(); - maxOutputTreesSize += VarInt.MAX_VARINT_SIZE; // value.archivedFileValue() optional maxOutputTreesSize += - value.archivedFileValue().map(ignored -> MAX_REMOTE_METADATA_SIZE).orElse(0); + VarInt.MAX_VARINT_SIZE // value.archivedFileValue() optional + + value.archivedFileValue().map(ignored -> MAX_REMOTE_METADATA_SIZE).orElse(0); + maxOutputTreesSize += + VarInt.MAX_VARINT_SIZE // value.symlinkTargetExecPath() optional + + value.symlinkTargetExecPath().map(ignored -> VarInt.MAX_VARINT_SIZE).orElse(0); } // Estimate the size of the buffer: @@ -578,6 +602,15 @@ private static byte[] encode(StringIndexer indexer, ActionCache.Entry entry) thr } else { VarInt.putVarInt(0, sink); } + + Optional symlinkTargetExecPath = + serializableTreeArtifactValue.symlinkTargetExecPath(); + if (symlinkTargetExecPath.isPresent()) { + VarInt.putVarInt(1, sink); + VarInt.putVarInt(indexer.getOrCreateIndex(symlinkTargetExecPath.get().toString()), sink); + } else { + VarInt.putVarInt(0, sink); + } } return sink.toByteArray(); @@ -654,8 +687,20 @@ private static ActionCache.Entry decode(StringIndexer indexer, byte[] data) thro archivedFileValue = Optional.of(decodeRemoteMetadata(indexer, source)); } + Optional symlinkTargetPath = Optional.empty(); + int numSymlinkTargetExecPath = VarInt.getVarInt(source); + if (numSymlinkTargetExecPath > 0) { + if (numSymlinkTargetExecPath != 1) { + throw new IOException("Invalid number of symlink target paths"); + } + symlinkTargetPath = + Optional.of( + PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source)))); + } + SerializableTreeArtifactValue value = - SerializableTreeArtifactValue.create(childValues.buildOrThrow(), archivedFileValue); + SerializableTreeArtifactValue.create( + childValues.buildOrThrow(), archivedFileValue, symlinkTargetPath); outputTrees.put(treeKey, value); } 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 04f1152e80d54e..5e3834cd2120e9 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 @@ -38,9 +38,9 @@ import com.google.devtools.build.lib.remote.util.TempPathGenerator; 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; import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; @@ -118,6 +118,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,34 +139,75 @@ 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(); + + FileArtifactValue treeMetadata = provider.getMetadata(tree); + // TODO(tjgq): Only download individual files that were requested within the tree. + // This isn't straightforward because multiple tree artifacts may share the same output tree + // when a ctx.actions.symlink is involved. + if (treeMetadata == null || !shouldDownloadAnyTreeFiles(treeFiles, treeMetadata)) { + 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 boolean shouldDownloadAnyTreeFiles( + Iterable treeFiles, FileArtifactValue metadata) { + for (TreeFileArtifact treeFile : treeFiles) { + if (shouldDownloadFile(treeFile.getPath(), metadata)) { + return true; + } + } + return false; + } + + 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 = Flowable.fromIterable(treeFiles) .flatMapSingle( treeFile -> { - Path path = treeRoot.getRelative(treeFile.getParentRelativePath()); FileArtifactValue metadata = provider.getMetadata(treeFile); - if (!shouldDownloadFile(path, metadata)) { - return Single.just(TransferResult.ok()); - } Path tempPath = tempPathGenerator.generateTempPath(); treeFileTmpPathMap.put(treeFile, tempPath); @@ -182,10 +224,11 @@ 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); + // Even though the root directory for a tree artifact is created prior to action + // execution, we might be prefetching to a different directory that doesn't yet + // exist (when FileArtifactValue#getSymlinkTargetExecPath() is present). + // In any case, we need to make it writable to move files into it. + createWritableDirectory(treeRoot); dirs.add(treeRoot); for (Map.Entry entry : treeFileTmpPathMap.entrySet()) { @@ -200,8 +243,7 @@ private Completable prefetchInputTree( break; } if (dirs.add(dir)) { - dir.createDirectory(); - dir.setWritable(true); + createWritableDirectory(dir); } } checkState(dir.equals(path)); @@ -230,20 +272,33 @@ 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); return Completable.complete(); } + PathFragment execPath = input.getExecPath(); + FileArtifactValue metadata = metadataProvider.getMetadata(input); - if (metadata == null) { + if (metadata == null || !shouldDownloadFile(execRoot.getRelative(execPath), metadata)) { return Completable.complete(); } - Path path = execRoot.getRelative(input.getExecPath()); - return downloadFileRx(path, metadata, priority); + PathFragment prefetchExecPath = metadata.getSymlinkTargetExecPath().orElse(execPath); + + Completable prefetch = + downloadFileNoCheckRx(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; } /** @@ -256,7 +311,11 @@ public Completable downloadFileRx(Path path, FileArtifactValue metadata, Priorit if (!shouldDownloadFile(path, metadata)) { return Completable.complete(); } + return downloadFileNoCheckRx(path, metadata, priority); + } + private Completable downloadFileNoCheckRx( + Path path, FileArtifactValue metadata, Priority priority) { if (path.isSymbolicLink()) { try { path = path.getRelative(path.readSymbolicLink()); @@ -327,6 +386,20 @@ private void deletePartialDownload(Path path) { } } + private void createWritableDirectory(Path dir) throws IOException { + dir.createDirectory(); + dir.setWritable(true); + } + + 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..cc5c95abcf8ac5 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,74 @@ 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)"); + } + + Artifact output = outputArtifactByExecPath.get(linkPath.relativeTo(execRoot)); + checkNotNull(output, "RemoteActionFileSystem can only create a symlink to an action output"); + + // 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. + if (output.isTreeArtifact()) { + TreeArtifactValue metadata = getRemoteInputTreeMetadata(targetFragment); + if (metadata != null) { + 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; + } + } + + // If the output is a symlink, or if the target artifact isn't stored remotely, fall back to + // creating an actual symlink in the local filesystem. super.createSymbolicLink(linkPath, targetFragment); } + private RemoteFileArtifactValue createRemoteFileArtifactValueForSymlink( + RemoteFileArtifactValue original, PathFragment targetPath) { + return RemoteFileArtifactValue.create( + 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), entry.getValue()); + } + original + .getArchivedRepresentation() + .ifPresent( + archivedRepresentation -> builder.setArchivedRepresentation(archivedRepresentation)); + // 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 +440,23 @@ 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); + // TODO(tjgq): Do we need to worry about "partially remote" tree artifacts? + 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 +474,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/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java index 117cca18c371c5..b546ef865aa5f7 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 @@ -30,6 +30,7 @@ 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.FileContentsProxy; import com.google.devtools.build.lib.actions.FileStateType; import com.google.devtools.build.lib.actions.HasDigest; import com.google.devtools.build.lib.actions.cache.MetadataDigestUtils; @@ -178,6 +179,7 @@ public static ArchivedRepresentation create( EMPTY_MAP, 0L, /*archivedRepresentation=*/ null, + /*symlinkTargetExecPath=*/ null, /*entirelyRemote=*/ false); private final byte[] digest; @@ -190,6 +192,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,16 +208,19 @@ 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; } public FileArtifactValue getMetadata() { - return FileArtifactValue.createProxy(digest); + return FileArtifactValue.createProxyForTreeArtifact( + digest, entirelyRemote, symlinkTargetExecPath); } ImmutableSet getChildPaths() { @@ -233,6 +247,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() { @@ -294,6 +313,8 @@ public String toString() { return MoreObjects.toStringHelper(this) .add("digest", digest) .add("childData", childData) + .add("archivedRepresentation", archivedRepresentation) + .add("symlinkTargetExecPath", symlinkTargetExecPath) .toString(); } @@ -313,7 +334,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 +474,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 +541,19 @@ public Builder setArchivedRepresentation(ArchivedRepresentation archivedRepresen return this; } + @CanIgnoreReturnValue + 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 +585,7 @@ public TreeArtifactValue build() { finalChildData, totalChildSize, archivedRepresentation, + symlinkTargetExecPath, entirelyRemote); } } 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 2244795ea915aa..a270a7b84b0f43 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 @@ -447,14 +447,21 @@ public void testDeletedConstantMetadataOutputCausesReexecution() throws Exceptio } private RemoteFileArtifactValue createRemoteFileMetadata(String content) { + return createRemoteFileMetadata(content, /* symlinkTargetExecPath= */ null); + } + + private RemoteFileArtifactValue createRemoteFileMetadata( + String content, @Nullable PathFragment symlinkTargetExecPath) { byte[] bytes = content.getBytes(UTF_8); - return RemoteFileArtifactValue.create(digest(bytes), bytes.length, 1, "action-id"); + return RemoteFileArtifactValue.create( + digest(bytes), bytes.length, 1, "action-id", symlinkTargetExecPath); } private static TreeArtifactValue createTreeMetadata( SpecialArtifact parent, ImmutableMap children, - Optional archivedArtifactValue) { + Optional archivedArtifactValue, + Optional symlinkTargetExecPath) { TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder(parent); for (Map.Entry entry : children.entrySet()) { builder.putChild( @@ -466,6 +473,7 @@ private static TreeArtifactValue createTreeMetadata( builder.setArchivedRepresentation( TreeArtifactValue.ArchivedRepresentation.create(artifact, metadata)); }); + symlinkTargetExecPath.ifPresent(execPath -> builder.setSymlinkTargetExecPath(execPath)); return builder.build(); } @@ -616,6 +624,30 @@ public void saveOutputMetadata_localMetadataIsSameAsRemoteMetadata_cached() thro assertThat(entry.getOutputFile(output)).isEqualTo(createRemoteFileMetadata(content)); } + @Test + public void saveOutputMetadata_localMetadataIsSameAsRemoteMetadata_withTargetExecPath_cached() + throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + Artifact output = createArtifact(artifactRoot, "bin/dummy"); + String content = "content"; + PathFragment symlinkTargetExecPath = PathFragment.create("/target/path"); + Action action = + new InjectOutputFileMetadataAction( + output, createRemoteFileMetadata(content, symlinkTargetExecPath)); + runAction(action); + assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + + writeContentAsLatin1(output.getPath(), content); + // Cached since local metadata is same as remote metadata + runAction(action); + + assertStatistics(1, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputFile(output)) + .isEqualTo(createRemoteFileMetadata(content, symlinkTargetExecPath)); + } + @Test public void saveOutputMetadata_localMetadataIsDifferentFromRemoteMetadata_notCached() throws Exception { @@ -655,7 +687,7 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataSaved() throws Exc "file2", createRemoteFileMetadata("content2")); Action action = new InjectOutputTreeMetadataAction( - output, createTreeMetadata(output, children, Optional.empty())); + output, createTreeMetadata(output, children, Optional.empty(), Optional.empty())); runAction(action); @@ -663,7 +695,8 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataSaved() throws Exc ActionCache.Entry entry = cache.get(output.getExecPathString()); assertThat(entry).isNotNull(); assertThat(entry.getOutputTree(output)) - .isEqualTo(SerializableTreeArtifactValue.create(children, Optional.empty())); + .isEqualTo( + SerializableTreeArtifactValue.create(children, Optional.empty(), Optional.empty())); assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); } @@ -676,7 +709,38 @@ public void saveOutputMetadata_treeMetadata_remoteArchivedArtifactSaved() throws new InjectOutputTreeMetadataAction( output, createTreeMetadata( - output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("content")))); + output, + ImmutableMap.of(), + Optional.of(createRemoteFileMetadata("content")), + Optional.empty())); + + runAction(action); + + assertThat(output.getPath().exists()).isFalse(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputTree(output)) + .isEqualTo( + SerializableTreeArtifactValue.create( + ImmutableMap.of(), + Optional.of(createRemoteFileMetadata("content")), + Optional.empty())); + assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + } + + @Test + public void saveOutputMetadata_treeMetadata_symlinkTargetExecPathSaved() throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + Action action = + new InjectOutputTreeMetadataAction( + output, + createTreeMetadata( + output, + ImmutableMap.of(), + Optional.empty(), + Optional.of(PathFragment.create("/target/path")))); runAction(action); @@ -686,7 +750,9 @@ public void saveOutputMetadata_treeMetadata_remoteArchivedArtifactSaved() throws assertThat(entry.getOutputTree(output)) .isEqualTo( SerializableTreeArtifactValue.create( - ImmutableMap.of(), Optional.of(createRemoteFileMetadata("content")))); + ImmutableMap.of(), + Optional.empty(), + Optional.of(PathFragment.create("/target/path")))); assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); } @@ -697,7 +763,8 @@ public void saveOutputMetadata_emptyTreeMetadata_notSaved() throws Exception { createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); Action action = new InjectOutputTreeMetadataAction( - output, createTreeMetadata(output, ImmutableMap.of(), Optional.empty())); + output, + createTreeMetadata(output, ImmutableMap.of(), Optional.empty(), Optional.empty())); MetadataHandler metadataHandler = new FakeMetadataHandler(); runAction(action); @@ -733,7 +800,7 @@ public void saveOutputMetadata_treeMetadata_localFileMetadataNotSaved() throws E fileSystem.getPath("/file2").delete(); Action action = new InjectOutputTreeMetadataAction( - output, createTreeMetadata(output, children, Optional.empty())); + output, createTreeMetadata(output, children, Optional.empty(), Optional.empty())); runAction(action); @@ -743,7 +810,9 @@ public void saveOutputMetadata_treeMetadata_localFileMetadataNotSaved() throws E assertThat(entry.getOutputTree(output)) .isEqualTo( SerializableTreeArtifactValue.create( - ImmutableMap.of("file1", createRemoteFileMetadata("content1")), Optional.empty())); + ImmutableMap.of("file1", createRemoteFileMetadata("content1")), + Optional.empty(), + Optional.empty())); assertStatistics(0, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); } @@ -759,7 +828,8 @@ public void saveOutputMetadata_treeMetadata_localArchivedArtifactNotSaved() thro createTreeMetadata( output, ImmutableMap.of(), - Optional.of(FileArtifactValue.createForTesting(fileSystem.getPath("/archive"))))); + Optional.of(FileArtifactValue.createForTesting(fileSystem.getPath("/archive"))), + Optional.empty())); fileSystem.getPath("/archive").delete(); runAction(action); @@ -782,7 +852,7 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataLoaded() throws Ex "file2", createRemoteFileMetadata("content2")); Action action = new InjectOutputTreeMetadataAction( - output, createTreeMetadata(output, children, Optional.empty())); + output, createTreeMetadata(output, children, Optional.empty(), Optional.empty())); MetadataHandler metadataHandler = new FakeMetadataHandler(); runAction(action); @@ -797,7 +867,8 @@ public void saveOutputMetadata_treeMetadata_remoteFileMetadataLoaded() throws Ex /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), /*isRemoteCacheEnabled=*/ true); - TreeArtifactValue expectedMetadata = createTreeMetadata(output, children, Optional.empty()); + TreeArtifactValue expectedMetadata = + createTreeMetadata(output, children, Optional.empty(), Optional.empty()); assertThat(token).isNull(); assertThat(output.getPath().exists()).isFalse(); ActionCache.Entry entry = cache.get(output.getExecPathString()); @@ -823,8 +894,8 @@ public void saveOutputMetadata_treeMetadata_localFileMetadataLoaded() throws Exc Action action = new InjectOutputTreeMetadataAction( output, - createTreeMetadata(output, children1, Optional.empty()), - createTreeMetadata(output, children2, Optional.empty())); + createTreeMetadata(output, children1, Optional.empty(), Optional.empty()), + createTreeMetadata(output, children2, Optional.empty(), Optional.empty())); MetadataHandler metadataHandler = new FakeMetadataHandler(); runAction(action); @@ -845,6 +916,7 @@ public void saveOutputMetadata_treeMetadata_localFileMetadataLoaded() throws Exc ImmutableMap.of( "file1", createRemoteFileMetadata("content1"), "file2", createRemoteFileMetadata("modified_remote")), + Optional.empty(), Optional.empty()); ActionCache.Entry entry = cache.get(output.getExecPathString()); assertThat(entry).isNotNull(); @@ -862,9 +934,15 @@ public void saveOutputMetadata_treeMetadata_localArchivedArtifactLoaded() throws new InjectOutputTreeMetadataAction( output, createTreeMetadata( - output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("content"))), + output, + ImmutableMap.of(), + Optional.of(createRemoteFileMetadata("content")), + Optional.empty()), createTreeMetadata( - output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("modified")))); + output, + ImmutableMap.of(), + Optional.of(createRemoteFileMetadata("modified")), + Optional.empty())); MetadataHandler metadataHandler = new FakeMetadataHandler(); runAction(action); @@ -881,7 +959,10 @@ public void saveOutputMetadata_treeMetadata_localArchivedArtifactLoaded() throws assertThat(output.getPath().exists()).isFalse(); TreeArtifactValue expectedMetadata = createTreeMetadata( - output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("modified"))); + output, + ImmutableMap.of(), + Optional.of(createRemoteFileMetadata("modified")), + Optional.empty()); ActionCache.Entry entry = cache.get(output.getExecPathString()); assertThat(entry).isNotNull(); assertThat(entry.getOutputTree(output)) @@ -908,7 +989,63 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th "file2", createRemoteFileMetadata("content2")); Action action = new InjectOutputTreeMetadataAction( - output, createTreeMetadata(output, children, Optional.empty())); + output, createTreeMetadata(output, children, Optional.empty(), Optional.empty())); + MetadataHandler metadataHandler = new FakeMetadataHandler(); + + runAction(action); + writeContentAsLatin1(output.getPath().getRelative("file1"), "content1"); + // Cache hit + Token token = + cacheChecker.getTokenIfNeedToExecute( + action, + /*resolvedCacheArtifacts=*/ null, + /*clientEnv=*/ ImmutableMap.of(), + /*handler=*/ null, + metadataHandler, + /*artifactExpander=*/ null, + /*remoteDefaultPlatformProperties=*/ ImmutableMap.of(), + /*isRemoteCacheEnabled=*/ true); + + assertThat(token).isNull(); + assertStatistics(1, new MissDetailsBuilder().set(MissReason.NOT_CACHED, 1).build()); + assertThat(output.getPath().exists()).isTrue(); + ActionCache.Entry entry = cache.get(output.getExecPathString()); + assertThat(entry).isNotNull(); + assertThat(entry.getOutputTree(output)) + .isEqualTo( + SerializableTreeArtifactValue.create(children, Optional.empty(), Optional.empty())); + assertThat(metadataHandler.getTreeArtifactValue(output)) + .isEqualTo( + createTreeMetadata( + output, + ImmutableMap.of( + "file1", + FileArtifactValue.createForTesting(output.getPath().getRelative("file1")), + "file2", + createRemoteFileMetadata("content2")), + Optional.empty(), + Optional.empty())); + } + + @Test + public void + saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_withSymlinkTargetExecPath_cached() + throws Exception { + cacheChecker = createActionCacheChecker(/*storeOutputMetadata=*/ true); + SpecialArtifact output = + createTreeArtifactWithGeneratingAction(artifactRoot, PathFragment.create("bin/dummy")); + ImmutableMap children = + ImmutableMap.of( + "file1", createRemoteFileMetadata("content1"), + "file2", createRemoteFileMetadata("content2")); + Action action = + new InjectOutputTreeMetadataAction( + output, + createTreeMetadata( + output, + children, + Optional.empty(), + Optional.of(PathFragment.create("/target/path")))); MetadataHandler metadataHandler = new FakeMetadataHandler(); runAction(action); @@ -931,7 +1068,9 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th ActionCache.Entry entry = cache.get(output.getExecPathString()); assertThat(entry).isNotNull(); assertThat(entry.getOutputTree(output)) - .isEqualTo(SerializableTreeArtifactValue.create(children, Optional.empty())); + .isEqualTo( + SerializableTreeArtifactValue.create( + children, Optional.empty(), Optional.of(PathFragment.create("/target/path")))); assertThat(metadataHandler.getTreeArtifactValue(output)) .isEqualTo( createTreeMetadata( @@ -941,6 +1080,7 @@ public void saveOutputMetadata_treeMetadataWithSameLocalFileMetadata_cached() th FileArtifactValue.createForTesting(output.getPath().getRelative("file1")), "file2", createRemoteFileMetadata("content2")), + Optional.empty(), Optional.empty())); } @@ -954,7 +1094,10 @@ public void saveOutputMetadata_treeMetadataWithSameLocalArchivedArtifact_cached( new InjectOutputTreeMetadataAction( output, createTreeMetadata( - output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("content")))); + output, + ImmutableMap.of(), + Optional.of(createRemoteFileMetadata("content")), + Optional.empty())); MetadataHandler metadataHandler = new FakeMetadataHandler(); runAction(action); @@ -966,7 +1109,10 @@ public void saveOutputMetadata_treeMetadataWithSameLocalArchivedArtifact_cached( assertThat(output.getPath().exists()).isFalse(); TreeArtifactValue expectedMetadata = createTreeMetadata( - output, ImmutableMap.of(), Optional.of(createRemoteFileMetadata("content"))); + output, + ImmutableMap.of(), + Optional.of(createRemoteFileMetadata("content")), + Optional.empty()); ActionCache.Entry entry = cache.get(output.getExecPathString()); assertThat(entry).isNotNull(); assertThat(entry.getOutputTree(output)) 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 6bb87ee6331d28..725f4bcdc8ab3c 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,13 +206,19 @@ private RemoteFileArtifactValue createRemoteMetadata(Artifact artifact, String c .getHashFunction() .hashBytes(bytes) .asBytes(); - return RemoteFileArtifactValue.create(digest, bytes.length, 1, "action-id"); + return RemoteFileArtifactValue.create( + digest, bytes.length, 1, "action-id", symlinkTargetExecPath); + } + + private RemoteFileArtifactValue createRemoteMetadata(Artifact artifact, String content) { + return createRemoteMetadata(artifact, content, /* symlinkTargetExecPath= */ null); } private TreeArtifactValue createTreeMetadata( SpecialArtifact parent, ImmutableMap children, - Optional archivedArtifactValue) { + Optional archivedArtifactValue, + Optional symlinkTargetExecPath) { TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder(parent); for (Map.Entry entry : children.entrySet()) { builder.putChild( @@ -222,6 +230,9 @@ private TreeArtifactValue createTreeMetadata( builder.setArchivedRepresentation( TreeArtifactValue.ArchivedRepresentation.create(artifact, metadata)); }); + if (symlinkTargetExecPath.isPresent()) { + builder.setSymlinkTargetExecPath(symlinkTargetExecPath.get()); + } return builder.build(); } @@ -239,6 +250,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"; @@ -274,6 +300,7 @@ public void putAndGet_treeMetadata_onlySavesRemoteFileMetadata() throws IOExcept Artifact.TreeFileArtifact.createTreeOutput( artifact, PathFragment.create("file2")), "content2")), + Optional.empty(), Optional.empty()); entry.addOutputTree(artifact, metadata, /* saveTreeMetadata= */ true); @@ -289,6 +316,7 @@ public void putAndGet_treeMetadata_onlySavesRemoteFileMetadata() throws IOExcept Artifact.TreeFileArtifact.createTreeOutput( artifact, PathFragment.create("file1")), "content1")), + Optional.empty(), Optional.empty())); } @@ -301,7 +329,10 @@ public void putAndGet_treeMetadata_savesRemoteArchivedArtifact() { artifactRoot, PathFragment.create("bin/dummy")); TreeArtifactValue metadata = createTreeMetadata( - artifact, ImmutableMap.of(), Optional.of(createRemoteMetadata(artifact, "content"))); + artifact, + ImmutableMap.of(), + Optional.of(createRemoteMetadata(artifact, "content")), + Optional.empty()); entry.addOutputTree(artifact, metadata, /* saveTreeMetadata= */ true); cache.put(key, entry); @@ -310,7 +341,9 @@ public void putAndGet_treeMetadata_savesRemoteArchivedArtifact() { assertThat(entry.getOutputTree(artifact)) .isEqualTo( SerializableTreeArtifactValue.create( - ImmutableMap.of(), Optional.of(createRemoteMetadata(artifact, "content")))); + ImmutableMap.of(), + Optional.of(createRemoteMetadata(artifact, "content")), + Optional.empty())); } @Test @@ -326,7 +359,8 @@ public void putAndGet_treeMetadata_ignoresLocalArchivedArtifact() throws IOExcep ImmutableMap.of(), Optional.of( createLocalMetadata( - ActionsTestUtil.createArtifact(artifactRoot, "bin/archive"), "content"))); + ActionsTestUtil.createArtifact(artifactRoot, "bin/archive"), "content")), + Optional.empty()); entry.addOutputTree(artifact, metadata, /* saveTreeMetadata= */ true); cache.put(key, entry); @@ -335,6 +369,28 @@ public void putAndGet_treeMetadata_ignoresLocalArchivedArtifact() throws IOExcep assertThat(entry.getOutputTree(artifact)).isNull(); } + @Test + public void putAndGet_treeMetadata_savesSymlinkTargetExecPath() { + String key = "key"; + PathFragment symlinkTargetExecPath = PathFragment.create("/execroot/some/path"); + ActionCache.Entry entry = new ActionCache.Entry(key, ImmutableMap.of(), false); + SpecialArtifact artifact = + ActionsTestUtil.createTreeArtifactWithGeneratingAction( + artifactRoot, PathFragment.create("bin/dummy")); + TreeArtifactValue metadata = + createTreeMetadata( + artifact, ImmutableMap.of(), Optional.empty(), Optional.of(symlinkTargetExecPath)); + entry.addOutputTree(artifact, metadata, /* saveTreeMetadata= */ true); + + cache.put(key, entry); + entry = cache.get(key); + + assertThat(entry.getOutputTree(artifact)) + .isEqualTo( + SerializableTreeArtifactValue.create( + ImmutableMap.of(), Optional.empty(), Optional.of(symlinkTargetExecPath))); + } + private static void assertKeyEquals(ActionCache cache1, ActionCache cache2, String key) { Object entry = cache1.get(key); assertThat(entry).isNotNull(); 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 46326b223b03b5..b1212bc4a79406 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 @@ -267,6 +267,12 @@ 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 createUnresolvedSymlinkArtifactWithExecPath( ArtifactRoot root, PathFragment execPath) { return SpecialArtifact.create( 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 623f493b04ac77..80b76c83d53625 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 @@ -42,6 +42,8 @@ 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; @@ -55,6 +57,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 +83,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 = RemoteFileArtifactValue.create( - 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 +109,49 @@ 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 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())); + RemoteFileArtifactValue childValue = + RemoteFileArtifactValue.create( + 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(); + metadata.put(parent, treeValue.getMetadata()); + return Pair.of(parent, treeValue.getChildren().asList()); + } + + protected Pair> createRemoteTreeArtifact( + String pathFragment, + Map contentMap, + Map metadata, + Map cas) + throws IOException { + return createRemoteTreeArtifact( + pathFragment, contentMap, /* symlinkTargetExecPath= */ null, metadata, cas); } protected abstract AbstractActionInputPrefetcher createPrefetcher(Map cas); @@ -121,44 +171,94 @@ 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", + 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 cas = new HashMap<>(); + Pair> tree = + createRemoteTreeArtifact( + "dir", + ImmutableMap.of("file1", "content1", "nested_dir/file2", "content2"), metadata, cas); + SpecialArtifact parent = tree.getFirst(); + Artifact firstChild = tree.getSecond().get(0); + Artifact secondChild = tree.getSecond().get(1); + MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); AbstractActionInputPrefetcher prefetcher = createPrefetcher(cas); - wait(prefetcher.prefetchFiles(metadata.keySet(), metadataProvider)); + 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 cas = new HashMap<>(); + PathFragment targetExecPath = artifactRoot.getExecPath().getChild("target"); + Pair> tree = + createRemoteTreeArtifact( + "dir", + ImmutableMap.of("file1", "content1", "nested_dir/file2", "content2"), + targetExecPath, + metadata, + cas); + SpecialArtifact parent = tree.getFirst(); + Artifact firstChild = tree.getSecond().get(0); + Artifact secondChild = tree.getSecond().get(1); + + MetadataProvider metadataProvider = new StaticMetadataProvider(metadata); + 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 5cc44a9323e2af..07bb3249055948 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; @@ -362,6 +363,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/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java index e5dcbfa208cb7e..98fbd75d1ac3f2 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,44 @@ 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 org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.ArgumentCaptor; /** Tests for {@link RemoteActionFileSystem} */ @RunWith(JUnit4.class) @@ -50,6 +60,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 +105,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"); + ImmutableList 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((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"); + ImmutableList 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"); + ImmutableList 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,27 +208,59 @@ 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 = + RemoteFileArtifactValue f = RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 1, "action-id"); inputs.putWithNoDepOwner(a, f); 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); + RemoteFileArtifactValue childMeta = + RemoteFileArtifactValue.create( + 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/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java index d3bd5e25628f50..0fd24c855f1ddb 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,18 @@ 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); + assertThat(tree.getMetadata().getSymlinkTargetExecPath()).hasValue(targetPath); + } + @Test public void empty() { TreeArtifactValue emptyTree = TreeArtifactValue.empty();