From dfe0aeef53bd24f46539cb3c6d1ec7282fbb812d Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 7 Oct 2024 09:58:24 -0700 Subject: [PATCH] Use a visitor pattern to consume fileset symlinks. Instead of constructing a `FilesetManifest`, callers use a `Visitor` that is invoked for every symlink in the fileset. This allows for an optimization: when the fileset has no relative symlinks (or we are ignoring relative symlinks), we can do a single-pass visitation without building up maps. Additionally, callers are responsible for concatenating the fileset-relative and execroot-relative paths to their roots. This reduces garbage from concatenating paths, since some callers don't need them. PiperOrigin-RevId: 683222046 Change-Id: I80f04306e7a617d3c3e630c56d96b2c968eb1d42 --- .../google/devtools/build/lib/actions/BUILD | 1 + .../build/lib/actions/CompletionContext.java | 20 +- .../build/lib/actions/FilesetOutputTree.java | 327 ++++++++---------- .../starlark/StarlarkCustomCommandLine.java | 15 +- .../build/lib/exec/SpawnInputExpander.java | 18 +- .../skyframe/ActionInputMetadataProvider.java | 27 +- .../lib/actions/FilesetOutputTreeTest.java | 201 +++++------ .../lib/exec/SpawnInputExpanderTest.java | 2 +- .../ActionOutputMetadataStoreTest.java | 2 +- 9 files changed, 263 insertions(+), 350 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD index 2b5cce94374c53..1bcd190f718c6d 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -407,6 +407,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", "//third_party:flogger", "//third_party:guava", + "//third_party:jsr305", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java b/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java index 871ee2db550971..d4d51f64fbbc83 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CompletionContext.java @@ -23,7 +23,6 @@ import com.google.common.collect.Maps; import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; -import com.google.devtools.build.lib.actions.FilesetOutputTree.FilesetManifest; import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehaviorWithoutError; import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.bugreport.BugReporter; @@ -175,19 +174,12 @@ public void visitArtifacts(Iterable artifacts, ArtifactReceiver receiv private void visitFileset(Artifact filesetArtifact, ArtifactReceiver receiver) { FilesetOutputTree filesetOutput = filesets.get(filesetArtifact); - FilesetManifest filesetManifest = - filesetOutput.constructFilesetManifestWithoutError( - PathFragment.EMPTY_FRAGMENT, - fullyResolveFilesetLinks - ? RelativeSymlinkBehaviorWithoutError.RESOLVE_FULLY - : RelativeSymlinkBehaviorWithoutError.RESOLVE); - - for (Map.Entry mapping : filesetManifest.getEntries().entrySet()) { - String targetFile = mapping.getValue(); - PathFragment locationInFileset = mapping.getKey(); - receiver.acceptFilesetMapping( - filesetArtifact, locationInFileset, execRoot.getRelative(targetFile)); - } + filesetOutput.visitSymlinks( + fullyResolveFilesetLinks + ? RelativeSymlinkBehaviorWithoutError.RESOLVE_FULLY + : RelativeSymlinkBehaviorWithoutError.RESOLVE, + (name, target, metadata) -> + receiver.acceptFilesetMapping(filesetArtifact, name, execRoot.getRelative(target))); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetOutputTree.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetOutputTree.java index ba5b229bf77c57..98726276edd32e 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FilesetOutputTree.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FilesetOutputTree.java @@ -25,79 +25,132 @@ import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; import java.io.IOException; -import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.Map; +import javax.annotation.Nullable; /** A collection of {@link FilesetOutputSymlink}s comprising the output tree of a fileset. */ public final class FilesetOutputTree { private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); - public static final FilesetOutputTree EMPTY = new FilesetOutputTree(ImmutableList.of()); + public static final FilesetOutputTree EMPTY = new FilesetOutputTree(ImmutableList.of(), false); private static final int MAX_SYMLINK_TRAVERSALS = 256; public static FilesetOutputTree create(ImmutableList symlinks) { - return symlinks.isEmpty() ? EMPTY : new FilesetOutputTree(symlinks); + return symlinks.isEmpty() + ? EMPTY + : new FilesetOutputTree( + symlinks, symlinks.stream().anyMatch(FilesetOutputTree::isRelativeSymlink)); } private final ImmutableList symlinks; + private final boolean hasRelativeSymlinks; - private FilesetOutputTree(ImmutableList symlinks) { + private FilesetOutputTree( + ImmutableList symlinks, boolean hasRelativeSymlinks) { this.symlinks = checkNotNull(symlinks); + this.hasRelativeSymlinks = hasRelativeSymlinks; + } + + /** Receiver for the symlinks in a fileset's output tree. */ + @FunctionalInterface + public interface Visitor { + + /** + * Called for each symlink in the fileset's output tree. + * + * @param name path of the symlink relative to the fileset's root; equivalent to {@link + * FilesetOutputSymlink#getName} + * @param target symlink target; either an absolute path if the symlink points to a source file + * or an execroot-relative path if the symlink points to an output + * @param metadata a {@link FileArtifactValue} representing the target's metadata if available, + * or {@code null} + */ + void acceptSymlink(PathFragment name, PathFragment target, @Nullable FileArtifactValue metadata) + throws E1, E2; } /** - * Constructs a {@link FilesetManifest} for this fileset tree, processing relative symlinks - * according to {@code relSymlinkBehavior}. Use when {@link RelativeSymlinkBehavior#ERROR} is - * guaranteed not to be the behavior. + * Visits the symlinks in this fileset tree, handling relative symlinks according to the given + * {@link RelativeSymlinkBehavior}. */ - public FilesetManifest constructFilesetManifestWithoutError( - PathFragment targetPrefix, RelativeSymlinkBehaviorWithoutError relSymlinkBehavior) { - try { - return constructFilesetManifest(targetPrefix, relSymlinkBehavior.target); - } catch (ForbiddenRelativeSymlinkException e) { - throw new IllegalStateException( - "Can't throw forbidden symlink exception unless behavior is ERROR: " - + relSymlinkBehavior - + ", " - + targetPrefix - + ", " - + symlinks, - e); - } + public void visitSymlinks( + RelativeSymlinkBehavior relSymlinkBehavior, Visitor visitor) + throws ForbiddenRelativeSymlinkException, E1, E2 { + var relSymlinkBehaviorWithoutError = + switch (relSymlinkBehavior) { + case RESOLVE_FULLY -> RelativeSymlinkBehaviorWithoutError.RESOLVE_FULLY; + case RESOLVE -> RelativeSymlinkBehaviorWithoutError.RESOLVE; + case IGNORE -> RelativeSymlinkBehaviorWithoutError.IGNORE; + case ERROR -> { + if (hasRelativeSymlinks) { + FilesetOutputSymlink relativeLink = + symlinks.stream().filter(FilesetOutputTree::isRelativeSymlink).findFirst().get(); + throw new ForbiddenRelativeSymlinkException(relativeLink); + } + yield RelativeSymlinkBehaviorWithoutError.IGNORE; + } + }; + visitSymlinks(relSymlinkBehaviorWithoutError, visitor); } /** - * Constructs a {@link FilesetManifest} for this fileset tree, processing relative symlinks - * according to {@code relSymlinkBehavior}. + * Visits the symlinks in this fileset tree, handling relative symlinks according to the given + * {@link RelativeSymlinkBehaviorWithoutError}. */ - public FilesetManifest constructFilesetManifest( - PathFragment targetPrefix, RelativeSymlinkBehavior relSymlinkBehavior) - throws ForbiddenRelativeSymlinkException { - LinkedHashMap entries = new LinkedHashMap<>(); + public void visitSymlinks( + RelativeSymlinkBehaviorWithoutError relSymlinkBehavior, Visitor visitor) + throws E1, E2 { + // Fast path: if we don't need to resolve relative symlinks (either because there are none or + // because we are ignoring them), perform a single-pass visitation. This is expected to be the + // common case. + if (!hasRelativeSymlinks || relSymlinkBehavior == RelativeSymlinkBehaviorWithoutError.IGNORE) { + for (FilesetOutputSymlink symlink : symlinks) { + if (!isRelativeSymlink(symlink)) { + visitor.acceptSymlink( + symlink.getName(), + symlink.getTargetPath(), + symlink.getMetadata() instanceof FileArtifactValue metadata ? metadata : null); + } + } + return; + } + + // Symlink name to resolved target path. Relative target paths are relative to the exec root. + Map resolvedLinks = new LinkedHashMap<>(); + // Symlink name to relative target path. Map relativeLinks = new HashMap<>(); + // Resolved target path to metadata. Map artifactValues = new HashMap<>(); + for (FilesetOutputSymlink outputSymlink : symlinks) { - PathFragment fullLocation = targetPrefix.getRelative(outputSymlink.getName()); + PathFragment name = outputSymlink.getName(); String targetPath = outputSymlink.getTargetPath().getPathString(); - if (isRelativeSymlink(outputSymlink)) { - addRelativeSymlinkEntry(targetPath, fullLocation, relSymlinkBehavior, relativeLinks); - } else { - // Symlinks are already deduplicated by name in SkyframeFilesetManifestAction. - checkState( - entries.put(fullLocation, targetPath) == null, - "Duplicate fileset entry at %s", - fullLocation); - } - if (outputSymlink.getMetadata() instanceof FileArtifactValue) { - artifactValues.put(targetPath, (FileArtifactValue) outputSymlink.getMetadata()); + var map = isRelativeSymlink(outputSymlink) ? relativeLinks : resolvedLinks; + + // Symlinks are already deduplicated by name in SkyframeFilesetManifestAction. + checkState(map.put(name, targetPath) == null, "Duplicate fileset entry at %s", name); + + if (outputSymlink.getMetadata() instanceof FileArtifactValue metadata) { + artifactValues.put(targetPath, metadata); } } - resolveRelativeSymlinks(entries, relativeLinks, targetPrefix.isAbsolute(), relSymlinkBehavior); - return new FilesetManifest(entries, artifactValues); + + if (relSymlinkBehavior == RelativeSymlinkBehaviorWithoutError.RESOLVE_FULLY) { + fullyResolveRelativeSymlinks(resolvedLinks, relativeLinks); + } else { + resolveRelativeSymlinks(resolvedLinks, relativeLinks); + } + + for (var entry : resolvedLinks.entrySet()) { + PathFragment name = entry.getKey(); + String target = entry.getValue(); + FileArtifactValue metadata = artifactValues.get(target); + visitor.acceptSymlink(name, PathFragment.create(target), metadata); + } } public ImmutableList symlinks() { @@ -130,21 +183,24 @@ public boolean equals(Object o) { @Override public String toString() { - return MoreObjects.toStringHelper(this).add("symlinks", symlinks).toString(); + return MoreObjects.toStringHelper(this) + .add("symlinks", symlinks) + .add("hasRelativeSymlinks", hasRelativeSymlinks) + .toString(); } /** Mode that determines how to handle relative target paths. */ public enum RelativeSymlinkBehavior { /** Ignore any relative target paths. */ IGNORE, - /** Give an error if a relative target path is encountered. */ ERROR, - - /** Resolve all relative target paths. */ + /** Resolve relative target paths that (transitively) point to another file in the fileset. */ RESOLVE, - - /** Fully resolve all relative paths, even those pointing to internal directories. */ + /** + * Fully resolve all relative paths, even those pointing to internal directories. Then do a + * virtual filesystem traversal to find all paths to all files in the fileset. + */ RESOLVE_FULLY } @@ -154,48 +210,20 @@ public enum RelativeSymlinkBehavior { */ public enum RelativeSymlinkBehaviorWithoutError { /** Ignore any relative target paths. */ - IGNORE(RelativeSymlinkBehavior.IGNORE), - + IGNORE, /** Resolve all relative target paths. */ - RESOLVE(RelativeSymlinkBehavior.RESOLVE), - + RESOLVE, /** Fully resolve all relative paths, even those pointing to internal directories. */ - RESOLVE_FULLY(RelativeSymlinkBehavior.RESOLVE_FULLY); - - private final RelativeSymlinkBehavior target; - - RelativeSymlinkBehaviorWithoutError(RelativeSymlinkBehavior target) { - this.target = target; - } + RESOLVE_FULLY } private static boolean isRelativeSymlink(FilesetOutputSymlink symlink) { return !symlink.getTargetPath().isAbsolute() && !symlink.isRelativeToExecRoot(); } - /** Potentially adds the relative symlink to the map, depending on {@code relSymlinkBehavior}. */ - private static void addRelativeSymlinkEntry( - String targetPath, - PathFragment fullLocation, - RelativeSymlinkBehavior relSymlinkBehavior, - Map relativeLinks) - throws ForbiddenRelativeSymlinkException { - switch (relSymlinkBehavior) { - case ERROR -> throw new ForbiddenRelativeSymlinkException(targetPath); - case RESOLVE, RESOLVE_FULLY -> - checkState( - relativeLinks.put(fullLocation, targetPath) == null, - "Duplicate fileset entry at %s", - fullLocation); - case IGNORE -> {} - } - } - - /** Fully resolve relative symlinks including internal directory symlinks. */ + /** Fully resolves relative symlinks, including internal directory symlinks. */ private static void fullyResolveRelativeSymlinks( - Map entries, - Map relativeLinks, - boolean absolute) { + Map resolvedLinks, Map relativeLinks) { try { // Construct an in-memory Filesystem containing all the non-relative-symlink entries in the // Fileset. Treat these as regular files in the filesystem whose contents are the "real" @@ -206,7 +234,7 @@ private static void fullyResolveRelativeSymlinks( // (Choice of digest function is irrelevant). InMemoryFileSystem fs = new InMemoryFileSystem(DigestHashFunction.SHA256); Path root = fs.getPath("/"); - for (Map.Entry e : entries.entrySet()) { + for (Map.Entry e : resolvedLinks.entrySet()) { PathFragment location = e.getKey(); Path locationPath = root.getRelative(location); locationPath.getParentDirectory().createDirectoryAndParents(); @@ -216,27 +244,26 @@ private static void fullyResolveRelativeSymlinks( PathFragment location = e.getKey(); Path locationPath = fs.getPath("/").getRelative(location); PathFragment value = PathFragment.create(checkNotNull(e.getValue(), e)); - checkState(!value.isAbsolute(), e); locationPath.getParentDirectory().createDirectoryAndParents(); locationPath.createSymbolicLink(value); } - addSymlinks(root, entries, absolute); + addSymlinks(root, resolvedLinks); } catch (IOException e) { throw new IllegalStateException("InMemoryFileSystem can't throw", e); } } - private static void addSymlinks(Path root, Map entries, boolean absolute) + private static void addSymlinks(Path root, Map resolvedLinks) throws IOException { for (Path path : root.getDirectoryEntries()) { try { if (path.isDirectory()) { - addSymlinks(path, entries, absolute); + addSymlinks(path, resolvedLinks); } else { String contents = new String(FileSystemUtils.readContentAsLatin1(path)); - entries.put(absolute ? path.asFragment() : path.asFragment().toRelative(), contents); + resolvedLinks.put(path.asFragment().toRelative(), contents); } } catch (IOException e) { logger.atWarning().log("Symlink %s is dangling or cyclic: %s", path, e.getMessage()); @@ -244,106 +271,56 @@ private static void addSymlinks(Path root, Map entries, bo } } - /** - * Resolves relative symlinks and puts them in the {@code entries} map. - * - *

Note that {@code relativeLinks} should only contain entries in {@link - * RelativeSymlinkBehavior#RESOLVE} or {@link RelativeSymlinkBehavior#RESOLVE_FULLY} mode. - */ + /** Resolves relative symlinks and puts them in the {@code resolvedLinks} map. */ private static void resolveRelativeSymlinks( - Map entries, - Map relativeLinks, - boolean absolute, - RelativeSymlinkBehavior relSymlinkBehavior) { - if (relativeLinks.isEmpty()) { - return; - } - if (relSymlinkBehavior == RelativeSymlinkBehavior.RESOLVE_FULLY) { - fullyResolveRelativeSymlinks(entries, relativeLinks, absolute); - } else if (relSymlinkBehavior == RelativeSymlinkBehavior.RESOLVE) { - for (Map.Entry e : relativeLinks.entrySet()) { - PathFragment location = e.getKey(); - String actual = e.getValue(); - checkState(!actual.startsWith("/"), e); - PathFragment actualLocation = location; - - // Recursively resolve relative symlinks. - LinkedHashSet seen = new LinkedHashSet<>(); - int traversals = 0; - do { - actualLocation = actualLocation.getParentDirectory().getRelative(actual); - actual = relativeLinks.get(actualLocation); - } while (++traversals <= MAX_SYMLINK_TRAVERSALS && actual != null && seen.add(actual)); - - if (traversals >= MAX_SYMLINK_TRAVERSALS) { - logger.atWarning().log( - "Symlink %s is part of a chain of length at least %d" - + " which exceeds Blaze's maximum allowable symlink chain length", - location, traversals); - } else if (actual != null) { + Map resolvedLinks, Map relativeLinks) { + for (Map.Entry e : relativeLinks.entrySet()) { + PathFragment location = e.getKey(); + String actual = e.getValue(); + PathFragment actualLocation = location; + + // Recursively resolve relative symlinks. + LinkedHashSet seen = new LinkedHashSet<>(); + int traversals = 0; + do { + actualLocation = actualLocation.getParentDirectory().getRelative(actual); + actual = relativeLinks.get(actualLocation); + } while (++traversals <= MAX_SYMLINK_TRAVERSALS && actual != null && seen.add(actual)); + + if (traversals >= MAX_SYMLINK_TRAVERSALS) { + logger.atWarning().log( + "Symlink %s is part of a chain of length at least %d" + + " which exceeds Blaze's maximum allowable symlink chain length", + location, traversals); + } else if (actual != null) { + // TODO(b/113128395): throw here. + logger.atWarning().log("Symlink %s forms a symlink cycle: %s", location, seen); + } else { + String resolvedTarget = resolvedLinks.get(actualLocation); + if (resolvedTarget == null) { + // We've found a relative symlink that points out of the fileset. We should really + // always throw here, but current behavior is that we tolerate such symlinks when they + // occur in runfiles, which is the only time this code is hit. // TODO(b/113128395): throw here. - logger.atWarning().log("Symlink %s forms a symlink cycle: %s", location, seen); + logger.atWarning().log( + "Symlink %s (transitively) points to %s that is not in this fileset (or was" + + " pruned because of a cycle)", + location, actualLocation); } else { - String resolvedTarget = entries.get(actualLocation); - if (resolvedTarget == null) { - // We've found a relative symlink that points out of the fileset. We should really - // always throw here, but current behavior is that we tolerate such symlinks when they - // occur in runfiles, which is the only time this code is hit. - // TODO(b/113128395): throw here. - logger.atWarning().log( - "Symlink %s (transitively) points to %s that is not in this fileset (or was pruned" - + " because of a cycle)", - location, actualLocation); - } else { - // We have successfully resolved the symlink. - entries.put(location, resolvedTarget); - } + // We have successfully resolved the symlink. + resolvedLinks.put(location, resolvedTarget); } } } } - /** Representation of a Fileset manifest. */ - public static final class FilesetManifest { - private final Map entries; - private final Map artifactValues; - - private FilesetManifest( - Map entries, Map artifactValues) { - this.entries = Collections.unmodifiableMap(entries); - this.artifactValues = artifactValues; - } - - /** - * Returns a mapping of symlink name to its target path. - * - *

Values in this map can be: - * - *

    - *
  • An absolute path. - *
  • A relative path, which should be considered relative to the exec root. - *
- */ - public Map getEntries() { - return entries; - } - - /** - * Returns a mapping of target path to {@link FileArtifactValue}. - * - *

The keyset of this map is a subset of the values in the map returned by {@link - * #getEntries}. - */ - public Map getArtifactValues() { - return artifactValues; - } - } - /** Exception indicating that a relative symlink was encountered but not permitted. */ public static final class ForbiddenRelativeSymlinkException extends ForbiddenActionInputException { - private ForbiddenRelativeSymlinkException(String symlinkTarget) { - super("Fileset symlink " + symlinkTarget + " is not absolute"); + private ForbiddenRelativeSymlinkException(FilesetOutputSymlink relativeLink) { + super( + "Fileset symlink %s -> %s is not absolute" + .formatted(relativeLink.getName(), relativeLink.getTargetPath())); } } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java index 5697cb61f33120..88808bd6b7df35 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java @@ -38,7 +38,6 @@ import com.google.devtools.build.lib.actions.CommandLines; import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput; import com.google.devtools.build.lib.actions.FilesetOutputTree; -import com.google.devtools.build.lib.actions.FilesetOutputTree.FilesetManifest; import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehaviorWithoutError; import com.google.devtools.build.lib.actions.PathMapper; import com.google.devtools.build.lib.actions.SingleStringArgFormatter; @@ -498,15 +497,15 @@ private static void expandFileset( fileset), e); } - FilesetManifest filesetManifest = - filesetOutput.constructFilesetManifestWithoutError( - fileset.getExecPath(), RelativeSymlinkBehaviorWithoutError.IGNORE); - for (PathFragment relativePath : filesetManifest.getEntries().keySet()) { - PathFragment mappedRelativePath = pathMapper.map(relativePath); - expandedValues.add(new FilesetSymlinkFile(fileset, mappedRelativePath)); - } + PathFragment mappedExecPath = pathMapper.map(fileset.getExecPath()); + filesetOutput.visitSymlinks( + RelativeSymlinkBehaviorWithoutError.IGNORE, + (name, target, metadata) -> + expandedValues.add( + new FilesetSymlinkFile(fileset, mappedExecPath.getRelative(name)))); } + private int addToFingerprint( List arguments, int argi, diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java index d274cda9b4c831..bf59f18b821833 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java @@ -28,7 +28,6 @@ import com.google.devtools.build.lib.actions.ArtifactExpander; import com.google.devtools.build.lib.actions.ArtifactExpander.MissingExpansionException; import com.google.devtools.build.lib.actions.FilesetOutputTree; -import com.google.devtools.build.lib.actions.FilesetOutputTree.FilesetManifest; import com.google.devtools.build.lib.actions.FilesetOutputTree.ForbiddenRelativeSymlinkException; import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehavior; import com.google.devtools.build.lib.actions.ForbiddenActionInputException; @@ -188,15 +187,14 @@ private void addFilesetManifest( PathFragment baseDirectory) throws ForbiddenRelativeSymlinkException { Preconditions.checkArgument(filesetArtifact.isFileset(), filesetArtifact); - FilesetManifest filesetManifest = - filesetOutput.constructFilesetManifest(location, relSymlinkBehavior); - - for (Map.Entry mapping : filesetManifest.getEntries().entrySet()) { - String value = mapping.getValue(); - ActionInput artifact = ActionInputHelper.fromPath(execRoot.getRelative(value).asFragment()); - // TODO(bazel-team): Add path mapping support for filesets. - addMapping(inputMap, mapping.getKey(), artifact, baseDirectory); - } + filesetOutput.visitSymlinks( + relSymlinkBehavior, + (name, target, metadata) -> + addMapping( + inputMap, + location.getRelative(name), + ActionInputHelper.fromPath(execRoot.getRelative(target).asFragment()), + baseDirectory)); } private void addInputs( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMetadataProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMetadataProvider.java index a334dad6ff783d..68bab8c8f1b950 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMetadataProvider.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMetadataProvider.java @@ -24,7 +24,6 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FilesetOutputTree; -import com.google.devtools.build.lib.actions.FilesetOutputTree.FilesetManifest; import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehaviorWithoutError; import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.RunfilesArtifactValue; @@ -64,24 +63,20 @@ final class ActionInputMetadataProvider implements InputMetadataProvider { Map filesets) { this.execRoot = execRoot; this.inputArtifactData = inputArtifactData; - this.filesetMapping = Suppliers.memoize(() -> createFilesetMapping(filesets, execRoot)); + this.filesetMapping = Suppliers.memoize(() -> createFilesetMapping(filesets)); } private static ImmutableMap createFilesetMapping( - Map filesets, PathFragment execRoot) { + Map filesets) { Map filesetMap = new HashMap<>(); for (FilesetOutputTree filesetOutput : filesets.values()) { - FilesetManifest manifest = - filesetOutput.constructFilesetManifestWithoutError( - execRoot, RelativeSymlinkBehaviorWithoutError.RESOLVE); - manifest - .getArtifactValues() - .forEach( - (targetPath, metadata) -> { - if (metadata.getDigest() != null) { - filesetMap.put(targetPath, metadata); - } - }); + filesetOutput.visitSymlinks( + RelativeSymlinkBehaviorWithoutError.RESOLVE, + (name, target, metadata) -> { + if (metadata != null && metadata.getDigest() != null) { + filesetMap.put(target.getPathString(), metadata); + } + }); } return ImmutableMap.copyOf(filesetMap); } @@ -96,9 +91,7 @@ public FileArtifactValue getInputMetadata(ActionInput actionInput) throws IOExce return filesetMapping.get().get(filesetKeyPath.getPathString()); } - FileArtifactValue value; - - value = inputArtifactData.getInputMetadata(artifact); + FileArtifactValue value = inputArtifactData.getInputMetadata(artifact); if (value != null) { return checkExists(value, artifact); } diff --git a/src/test/java/com/google/devtools/build/lib/actions/FilesetOutputTreeTest.java b/src/test/java/com/google/devtools/build/lib/actions/FilesetOutputTreeTest.java index 21b4f04f8fe8e0..9e3d07f2b80c0a 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/FilesetOutputTreeTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/FilesetOutputTreeTest.java @@ -15,17 +15,17 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehavior.ERROR; -import static com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehavior.IGNORE; -import static com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehavior.RESOLVE_FULLY; +import static com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehaviorWithoutError.IGNORE; +import static com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehaviorWithoutError.RESOLVE_FULLY; import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.actions.FilesetOutputTree.FilesetManifest; import com.google.devtools.build.lib.actions.FilesetOutputTree.ForbiddenRelativeSymlinkException; import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehavior; -import com.google.devtools.build.lib.actions.FilesetOutputTreeTest.ManifestCommonTests; -import com.google.devtools.build.lib.actions.FilesetOutputTreeTest.OneOffManifestTests; -import com.google.devtools.build.lib.actions.FilesetOutputTreeTest.ResolvingManifestTests; +import com.google.devtools.build.lib.actions.FilesetOutputTree.RelativeSymlinkBehaviorWithoutError; +import com.google.devtools.build.lib.actions.FilesetOutputTreeTest.CommonTests; +import com.google.devtools.build.lib.actions.FilesetOutputTreeTest.OneOffTests; +import com.google.devtools.build.lib.actions.FilesetOutputTreeTest.ResolvingTests; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; @@ -36,11 +36,7 @@ /** Tests for {@link FilesetOutputTree}. */ @RunWith(Suite.class) -@Suite.SuiteClasses({ - ManifestCommonTests.class, - OneOffManifestTests.class, - ResolvingManifestTests.class -}) +@Suite.SuiteClasses({CommonTests.class, OneOffTests.class, ResolvingTests.class}) public final class FilesetOutputTreeTest { private static final PathFragment EXEC_ROOT = PathFragment.create("/root"); @@ -50,78 +46,75 @@ private static FilesetOutputSymlink symlink(String from, String to) { PathFragment.create(from), PathFragment.create(to), EXEC_ROOT); } + private static VisitedSymlink visitedSymlink(String from, String to) { + return new VisitedSymlink(PathFragment.create(from), PathFragment.create(to)); + } + + private record VisitedSymlink(PathFragment name, PathFragment target) {} + private static FilesetOutputTree createTree(FilesetOutputSymlink... symlinks) { return FilesetOutputTree.create(ImmutableList.copyOf(symlinks)); } - /** Manifest tests that apply to all relative symlink behavior. */ + private static ImmutableList visit( + FilesetOutputTree filesetOutput, RelativeSymlinkBehavior relSymlinkBehavior) + throws ForbiddenRelativeSymlinkException { + var visited = ImmutableList.builder(); + filesetOutput.visitSymlinks( + relSymlinkBehavior, + (name, target, metadata) -> visited.add(new VisitedSymlink(name, target))); + return visited.build(); + } + + private static ImmutableList visit( + FilesetOutputTree filesetOutput, RelativeSymlinkBehaviorWithoutError relSymlinkBehavior) { + var visited = ImmutableList.builder(); + filesetOutput.visitSymlinks( + relSymlinkBehavior, + (name, target, metadata) -> visited.add(new VisitedSymlink(name, target))); + return visited.build(); + } + + /** Tests that apply to all relative symlink behaviors. */ @RunWith(TestParameterInjector.class) - public static final class ManifestCommonTests { + public static final class CommonTests { @TestParameter private RelativeSymlinkBehavior behavior; @Test - public void emptyManifest() throws Exception { - FilesetManifest manifest = - FilesetOutputTree.EMPTY.constructFilesetManifest( - PathFragment.create("out/foo"), behavior); - - assertThat(manifest.getEntries()).isEmpty(); - assertThat(manifest.getArtifactValues()).isEmpty(); + public void empty() throws Exception { + assertThat(visit(FilesetOutputTree.EMPTY, behavior)).isEmpty(); } @Test - public void manifestWithSingleFile() throws Exception { + public void singleFile() throws Exception { FilesetOutputTree filesetOutput = createTree(symlink("bar", "/dir/file")); - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out/foo"), behavior); - - assertThat(manifest.getEntries()) - .containsExactly(PathFragment.create("out/foo/bar"), "/dir/file"); + assertThat(visit(filesetOutput, behavior)) + .containsExactly(visitedSymlink("bar", "/dir/file")); } @Test - public void manifestWithTwoFiles() throws Exception { + public void twoFiles() throws Exception { FilesetOutputTree filesetOutput = createTree(symlink("bar", "/dir/file"), symlink("baz", "/dir/file")); - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out/foo"), behavior); - - assertThat(manifest.getEntries()) - .containsExactly( - PathFragment.create("out/foo/bar"), "/dir/file", - PathFragment.create("out/foo/baz"), "/dir/file"); + assertThat(visit(filesetOutput, behavior)) + .containsExactly(visitedSymlink("bar", "/dir/file"), visitedSymlink("baz", "/dir/file")); } @Test - public void manifestWithDirectory() throws Exception { - FilesetOutputTree filesetOutput = createTree(symlink("bar", "/some")); - - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out/foo"), behavior); - - assertThat(manifest.getEntries()) - .containsExactly(PathFragment.create("out/foo/bar"), "/some"); - } - - @Test - public void manifestWithExecRootRelativePath() throws Exception { + public void symlinkWithExecRootRelativePath() throws Exception { FilesetOutputTree filesetOutput = createTree(symlink("bar", EXEC_ROOT.getRelative("foo/bar").getPathString())); - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out/foo"), behavior); - - assertThat(manifest.getEntries()) - .containsExactly(PathFragment.create("out/foo/bar"), "foo/bar"); + assertThat(visit(filesetOutput, behavior)).containsExactly(visitedSymlink("bar", "foo/bar")); } } - /** Manifest tests that apply to a specific relative symlink behavior. */ + /** Tests that apply to a specific relative symlink behavior. */ @RunWith(JUnit4.class) - public static final class OneOffManifestTests { + public static final class OneOffTests { @Test public void canonicalEmptyInstance() { @@ -129,81 +122,62 @@ public void canonicalEmptyInstance() { } @Test - public void manifestWithErrorOnRelativeSymlink() { + public void errorOnRelativeSymlink() { FilesetOutputTree filesetOutput = createTree(symlink("bar", "foo"), symlink("foo", "/foo/bar")); var e = - assertThrows( - ForbiddenRelativeSymlinkException.class, - () -> filesetOutput.constructFilesetManifest(PathFragment.create("out/foo"), ERROR)); - assertThat(e).hasMessageThat().contains("Fileset symlink foo is not absolute"); + assertThrows(ForbiddenRelativeSymlinkException.class, () -> visit(filesetOutput, ERROR)); + assertThat(e).hasMessageThat().contains("Fileset symlink bar -> foo is not absolute"); } @Test - public void manifestWithIgnoredRelativeSymlink() throws Exception { + public void ignoredRelativeSymlink() { FilesetOutputTree filesetOutput = createTree(symlink("bar", "foo"), symlink("foo", "/foo/bar")); - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out/foo"), IGNORE); - - assertThat(manifest.getEntries()) - .containsExactly(PathFragment.create("out/foo/foo"), "/foo/bar"); + assertThat(visit(filesetOutput, IGNORE)).containsExactly(visitedSymlink("foo", "/foo/bar")); } @Test - public void manifestWithResolvedRelativeDirectorySymlink() throws Exception { + public void resolvedRelativeDirectorySymlink() { FilesetOutputTree filesetOutput = createTree(symlink("foo/subdir/f1", "/foo/subdir/f1"), symlink("foo/bar", "subdir")); - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out"), RESOLVE_FULLY); - - assertThat(manifest.getEntries()) + assertThat(visit(filesetOutput, RESOLVE_FULLY)) .containsExactly( - PathFragment.create("out/foo/subdir/f1"), "/foo/subdir/f1", - PathFragment.create("out/foo/bar/f1"), "/foo/subdir/f1"); + visitedSymlink("foo/subdir/f1", "/foo/subdir/f1"), + visitedSymlink("foo/bar/f1", "/foo/subdir/f1")); } } - /** Manifest tests that apply resolving relative symlink behavior. */ + /** Tests that apply resolving relative symlink behaviors. */ @RunWith(TestParameterInjector.class) - public static final class ResolvingManifestTests { + public static final class ResolvingTests { @TestParameter({"RESOLVE", "RESOLVE_FULLY"}) - private RelativeSymlinkBehavior behavior; + private RelativeSymlinkBehaviorWithoutError behavior; @Test - public void manifestWithResolvedRelativeSymlink() throws Exception { + public void resolvedRelativeSymlink() { FilesetOutputTree filesetOutput = createTree(symlink("bar", "foo"), symlink("foo", "/foo/bar")); - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out/foo"), behavior); - - assertThat(manifest.getEntries()) - .containsExactly( - PathFragment.create("out/foo/bar"), "/foo/bar", - PathFragment.create("out/foo/foo"), "/foo/bar"); + assertThat(visit(filesetOutput, behavior)) + .containsExactly(visitedSymlink("bar", "/foo/bar"), visitedSymlink("foo", "/foo/bar")); } @Test - public void manifestWithResolvedRelativeSymlinkWithDotSlash() throws Exception { + public void resolvedRelativeSymlinkWithDotSlash() { FilesetOutputTree filesetOutput = createTree(symlink("bar", "./foo"), symlink("foo", "/foo/bar")); - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out/foo"), behavior); - - assertThat(manifest.getEntries()) - .containsExactly( - PathFragment.create("out/foo/bar"), "/foo/bar", - PathFragment.create("out/foo/foo"), "/foo/bar"); + assertThat(visit(filesetOutput, behavior)) + .containsExactly(visitedSymlink("bar", "/foo/bar"), visitedSymlink("foo", "/foo/bar")); } @Test - public void manifestWithSymlinkCycle() throws Exception { + public void symlinkCycle() { FilesetOutputTree filesetOutput = createTree( symlink("bar", "foo"), @@ -211,68 +185,47 @@ public void manifestWithSymlinkCycle() throws Exception { symlink("biz", "bar"), symlink("reg", "/file")); - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out"), behavior); - - assertThat(manifest.getEntries()).containsExactly(PathFragment.create("out/reg"), "/file"); + assertThat(visit(filesetOutput, behavior)).containsExactly(visitedSymlink("reg", "/file")); } @Test - public void unboundedSymlinkDescendant() throws Exception { + public void unboundedSymlinkDescendant() { FilesetOutputTree filesetOutput = createTree(symlink("p", "a/b"), symlink("a/b", "../b/c"), symlink("reg", "/file")); - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out"), behavior); - - assertThat(manifest.getEntries()).containsExactly(PathFragment.create("out/reg"), "/file"); + assertThat(visit(filesetOutput, behavior)).containsExactly(visitedSymlink("reg", "/file")); } @Test - public void unboundedSymlinkAncestor() throws Exception { + public void unboundedSymlinkAncestor() { FilesetOutputTree filesetOutput = createTree(symlink("a/b", "c/d"), symlink("a/c/d", ".././a"), symlink("reg", "/file")); - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out"), behavior); - - assertThat(manifest.getEntries()).containsExactly(PathFragment.create("out/reg"), "/file"); + assertThat(visit(filesetOutput, behavior)).containsExactly(visitedSymlink("reg", "/file")); } @Test - public void manifestWithResolvedRelativeSymlinkWithDotDotSlash() throws Exception { + public void resolvedRelativeSymlinkWithDotDotSlash() { FilesetOutputTree filesetOutput = createTree(symlink("bar/bar", "../foo/foo"), symlink("foo/foo", "/foo/bar")); - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out/foo"), behavior); - - assertThat(manifest.getEntries()) + assertThat(visit(filesetOutput, behavior)) .containsExactly( - PathFragment.create("out/foo/bar/bar"), "/foo/bar", - PathFragment.create("out/foo/foo/foo"), "/foo/bar"); + visitedSymlink("bar/bar", "/foo/bar"), visitedSymlink("foo/foo", "/foo/bar")); } @Test - public void manifestWithUnresolvableRelativeSymlink() throws Exception { + public void unresolvableRelativeSymlink() { FilesetOutputTree filesetOutput = createTree(symlink("bar", "foo")); - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out/foo"), behavior); - - assertThat(manifest.getEntries()).isEmpty(); - assertThat(manifest.getArtifactValues()).isEmpty(); + assertThat(visit(filesetOutput, behavior)).isEmpty(); } @Test - public void manifestWithUnresolvableRelativeSymlinkToRelativeSymlink() throws Exception { + public void unresolvableRelativeSymlinkToRelativeSymlink() { FilesetOutputTree filesetOutput = createTree(symlink("bar", "foo"), symlink("foo", "baz")); - FilesetManifest manifest = - filesetOutput.constructFilesetManifest(PathFragment.create("out/foo"), behavior); - - assertThat(manifest.getEntries()).isEmpty(); - assertThat(manifest.getArtifactValues()).isEmpty(); + assertThat(visit(filesetOutput, behavior)).isEmpty(); } } } diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java index c0f6b41bf40792..7a00c66299462a 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnInputExpanderTest.java @@ -515,7 +515,7 @@ public void testManifestWithErrorOnRelativeSymlink() { expander.addFilesetManifests( filesetMappings, inputMap, PathFragment.EMPTY_FRAGMENT)); - assertThat(e).hasMessageThat().contains("Fileset symlink foo is not absolute"); + assertThat(e).hasMessageThat().contains("Fileset symlink workspace/bar -> foo is not absolute"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java index 04b0dc0153f0a0..eae6cdef453648 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java @@ -585,7 +585,7 @@ public void getMetadataFromFilesetMapping() throws Exception { private FilesetOutputSymlink createFilesetOutputSymlink(HasDigest digest, String identifier) { return FilesetOutputSymlink.create( PathFragment.create(identifier + "_symlink"), - PathFragment.create(identifier), + execRoot.getRelative(identifier).asFragment(), digest, execRoot.asFragment(), /* enclosingTreeArtifact= */ null);