Do not reconstitute absolute path until after FilesetManifest. This way callers aren't required to pass in the exec root. RELNOTES: None. PiperOrigin-RevId: 217500815
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java index 53bfb09..133aaf3 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java
@@ -22,7 +22,6 @@ import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; -import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CommandLineItem; @@ -359,8 +358,7 @@ FilesetManifest.constructFilesetManifest( artifactExpander.getFileset(fileset), fileset.getExecPath(), - RelativeSymlinkBehavior.IGNORE, - extractExecRoot(fileset)); + RelativeSymlinkBehavior.IGNORE); for (PathFragment relativePath : filesetManifest.getEntries().keySet()) { expandedValues.add(new FilesetSymlinkFile(fileset, relativePath)); } @@ -369,14 +367,6 @@ } } - // TODO(b/117267351): Pass in the exec root more cleanly, or put relative paths in the manifest. - private static PathFragment extractExecRoot(Artifact fileset) { - Preconditions.checkArgument(!fileset.isSourceArtifact(), fileset); - ArtifactRoot root = fileset.getRoot(); - PathFragment rootFrag = root.getRoot().asPath().asFragment(); - return rootFrag.subFragment(0, rootFrag.segmentCount() - root.getExecPath().segmentCount()); - } - private int addToFingerprint( List<Object> arguments, int argi,
diff --git a/src/main/java/com/google/devtools/build/lib/exec/FilesetManifest.java b/src/main/java/com/google/devtools/build/lib/exec/FilesetManifest.java index fa4e993..7dd1a98 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/FilesetManifest.java +++ b/src/main/java/com/google/devtools/build/lib/exec/FilesetManifest.java
@@ -51,17 +51,15 @@ public static FilesetManifest constructFilesetManifest( List<FilesetOutputSymlink> outputSymlinks, PathFragment targetPrefix, - RelativeSymlinkBehavior relSymlinkBehavior, - PathFragment execRoot) + RelativeSymlinkBehavior relSymlinkBehavior) throws IOException { LinkedHashMap<PathFragment, String> entries = new LinkedHashMap<>(); Map<PathFragment, String> relativeLinks = new HashMap<>(); Map<String, FileArtifactValue> artifactValues = new HashMap<>(); for (FilesetOutputSymlink outputSymlink : outputSymlinks) { PathFragment fullLocation = targetPrefix.getRelative(outputSymlink.getName()); - PathFragment linkTarget = outputSymlink.reconstituteTargetPath(execRoot); - String artifact = Strings.emptyToNull(linkTarget.getPathString()); - if (!linkTarget.isAbsolute() && !linkTarget.isEmpty()) { + String artifact = Strings.emptyToNull(outputSymlink.getTargetPath().getPathString()); + if (isRelativeSymlink(outputSymlink)) { addRelativeSymlinkEntry(artifact, fullLocation, relSymlinkBehavior, relativeLinks); } else if (!entries.containsKey(fullLocation)) { // Keep consistent behavior: no overwriting. entries.put(fullLocation, artifact); @@ -70,7 +68,14 @@ artifactValues.put(artifact, (FileArtifactValue) outputSymlink.getMetadata()); } } - return constructFilesetManifest(entries, relativeLinks, artifactValues); + resolveRelativeSymlinks(entries, relativeLinks); + return new FilesetManifest(entries, artifactValues); + } + + private static boolean isRelativeSymlink(FilesetOutputSymlink symlink) { + return !symlink.getTargetPath().isEmpty() + && !symlink.getTargetPath().isAbsolute() + && !symlink.isRelativeToExecRoot(); } /** Potentially adds the relative symlink to the map, depending on {@code relSymlinkBehavior}. */ @@ -95,12 +100,14 @@ private static final int MAX_SYMLINK_TRAVERSALS = 256; - private static FilesetManifest constructFilesetManifest( - Map<PathFragment, String> entries, - Map<PathFragment, String> relativeLinks, - Map<String, FileArtifactValue> artifactValues) { - // Resolve relative symlinks. Note that relativeLinks only contains entries in RESOLVE mode. - // We must find targets for these symlinks that are not inside the Fileset itself. + /** + * Resolves relative symlinks and puts them in the {@code entries} map. + * + * <p>Note that {@code relativeLinks} should only contain entries in {@link + * RelativeSymlinkBehavior#RESOLVE} mode. + */ + private static void resolveRelativeSymlinks( + Map<PathFragment, String> entries, Map<PathFragment, String> relativeLinks) { for (Map.Entry<PathFragment, String> e : relativeLinks.entrySet()) { PathFragment location = e.getKey(); String value = e.getValue(); @@ -142,22 +149,37 @@ entries.put(location, entries.get(actualLocation)); } } - return new FilesetManifest(entries, artifactValues); } private final Map<PathFragment, String> entries; private final Map<String, FileArtifactValue> artifactValues; - private FilesetManifest(Map<PathFragment, String> entries, - Map<String, FileArtifactValue> artifactValues) { + private FilesetManifest( + Map<PathFragment, String> entries, Map<String, FileArtifactValue> artifactValues) { this.entries = Collections.unmodifiableMap(entries); this.artifactValues = artifactValues; } + /** + * Returns a mapping of symlink name to its target path. + * + * <p>Values in this map can be: + * + * <ul> + * <li>An absolute path. + * <li>A relative path, which should be considered relative to the exec root. + * <li>{@code null}, which represents an empty file. + * </ul> + */ public Map<PathFragment, String> getEntries() { return entries; } + /** + * Returns a mapping of target path to {@link FileArtifactValue}. + * + * <p>The keyset of this map is a subset of the values in the map returned by {@link #getEntries}. + */ public Map<String, FileArtifactValue> getArtifactValues() { return artifactValues; }
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 cf53a85..81a8170 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
@@ -163,11 +163,14 @@ ImmutableList<FilesetOutputSymlink> outputSymlinks = filesetMappings.get(fileset); FilesetManifest filesetManifest = FilesetManifest.constructFilesetManifest( - outputSymlinks, fileset.getExecPath(), relSymlinkBehavior, execRoot.asFragment()); + outputSymlinks, fileset.getExecPath(), relSymlinkBehavior); for (Map.Entry<PathFragment, String> mapping : filesetManifest.getEntries().entrySet()) { String value = mapping.getValue(); - ActionInput artifact = value == null ? EMPTY_FILE : ActionInputHelper.fromPath(value); + ActionInput artifact = + value == null + ? EMPTY_FILE + : ActionInputHelper.fromPath(execRoot.getRelative(value).getPathString()); addMapping(inputMappings, mapping.getKey(), artifact); } }
diff --git a/src/test/java/com/google/devtools/build/lib/exec/FilesetManifestTest.java b/src/test/java/com/google/devtools/build/lib/exec/FilesetManifestTest.java index 08b2386..8652148 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/FilesetManifestTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/FilesetManifestTest.java
@@ -44,8 +44,7 @@ List<FilesetOutputSymlink> symlinks = ImmutableList.of(); FilesetManifest manifest = - FilesetManifest.constructFilesetManifest( - symlinks, PathFragment.create("out/foo"), IGNORE, EXEC_ROOT); + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), IGNORE); assertThat(manifest.getEntries()).isEmpty(); } @@ -55,8 +54,7 @@ List<FilesetOutputSymlink> symlinks = ImmutableList.of(filesetSymlink("bar", "/dir/file")); FilesetManifest manifest = - FilesetManifest.constructFilesetManifest( - symlinks, PathFragment.create("out/foo"), IGNORE, EXEC_ROOT); + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), IGNORE); assertThat(manifest.getEntries()) .containsExactly(PathFragment.create("out/foo/bar"), "/dir/file"); @@ -68,8 +66,7 @@ ImmutableList.of(filesetSymlink("bar", "/dir/file"), filesetSymlink("baz", "/dir/file")); FilesetManifest manifest = - FilesetManifest.constructFilesetManifest( - symlinks, PathFragment.create("out/foo"), IGNORE, EXEC_ROOT); + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), IGNORE); assertThat(manifest.getEntries()) .containsExactly( @@ -82,8 +79,7 @@ List<FilesetOutputSymlink> symlinks = ImmutableList.of(filesetSymlink("bar", "/some")); FilesetManifest manifest = - FilesetManifest.constructFilesetManifest( - symlinks, PathFragment.create("out/foo"), IGNORE, EXEC_ROOT); + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), IGNORE); assertThat(manifest.getEntries()).containsExactly(PathFragment.create("out/foo/bar"), "/some"); } @@ -94,8 +90,7 @@ List<FilesetOutputSymlink> symlinks = ImmutableList.of(filesetSymlink("bar", "")); FilesetManifest manifest = - FilesetManifest.constructFilesetManifest( - symlinks, PathFragment.create("out/foo"), IGNORE, EXEC_ROOT); + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), IGNORE); assertThat(manifest.getEntries()).containsExactly(PathFragment.create("out/foo/bar"), null); } @@ -106,8 +101,7 @@ ImmutableList.of(filesetSymlink("bar", "foo"), filesetSymlink("foo", "/foo/bar")); try { - FilesetManifest.constructFilesetManifest( - symlinks, PathFragment.create("out/foo"), ERROR, EXEC_ROOT); + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), ERROR); fail("Expected to throw"); } catch (IOException e) { assertThat(e).hasMessageThat().isEqualTo("runfiles target is not absolute: foo"); @@ -120,8 +114,7 @@ ImmutableList.of(filesetSymlink("bar", "foo"), filesetSymlink("foo", "/foo/bar")); FilesetManifest manifest = - FilesetManifest.constructFilesetManifest( - symlinks, PathFragment.create("out/foo"), IGNORE, EXEC_ROOT); + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), IGNORE); assertThat(manifest.getEntries()) .containsExactly(PathFragment.create("out/foo/foo"), "/foo/bar"); @@ -133,8 +126,7 @@ ImmutableList.of(filesetSymlink("bar", "foo"), filesetSymlink("foo", "/foo/bar")); FilesetManifest manifest = - FilesetManifest.constructFilesetManifest( - symlinks, PathFragment.create("out/foo"), RESOLVE, EXEC_ROOT); + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), RESOLVE); assertThat(manifest.getEntries()) .containsExactly( @@ -148,8 +140,7 @@ ImmutableList.of(filesetSymlink("bar", "./foo"), filesetSymlink("foo", "/foo/bar")); FilesetManifest manifest = - FilesetManifest.constructFilesetManifest( - symlinks, PathFragment.create("out/foo"), RESOLVE, EXEC_ROOT); + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), RESOLVE); assertThat(manifest.getEntries()) .containsExactly( @@ -164,8 +155,7 @@ filesetSymlink("bar/bar", "../foo/foo"), filesetSymlink("foo/foo", "/foo/bar")); FilesetManifest manifest = - FilesetManifest.constructFilesetManifest( - symlinks, PathFragment.create("out/foo"), RESOLVE, EXEC_ROOT); + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), RESOLVE); assertThat(manifest.getEntries()) .containsExactly( @@ -178,8 +168,7 @@ List<FilesetOutputSymlink> symlinks = ImmutableList.of(filesetSymlink("bar", "foo")); FilesetManifest manifest = - FilesetManifest.constructFilesetManifest( - symlinks, PathFragment.create("out/foo"), RESOLVE, EXEC_ROOT); + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), RESOLVE); assertThat(manifest.getEntries()).isEmpty(); assertThat(manifest.getArtifactValues()).isEmpty(); @@ -191,8 +180,7 @@ ImmutableList.of(filesetSymlink("bar", "foo"), filesetSymlink("foo", "baz")); FilesetManifest manifest = - FilesetManifest.constructFilesetManifest( - symlinks, PathFragment.create("out/foo"), RESOLVE, EXEC_ROOT); + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), RESOLVE); assertThat(manifest.getEntries()).isEmpty(); assertThat(manifest.getArtifactValues()).isEmpty(); @@ -205,10 +193,21 @@ ImmutableList.of(filesetSymlink("bar", "/foo/bar"), filesetSymlink("bar", "/baz")); FilesetManifest manifest = - FilesetManifest.constructFilesetManifest( - symlinks, PathFragment.create("out/foo"), IGNORE, EXEC_ROOT); + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), IGNORE); assertThat(manifest.getEntries()) .containsExactly(PathFragment.create("out/foo/bar"), "/foo/bar"); } + + @Test + public void testManifestWithExecRootRelativePath() throws Exception { + List<FilesetOutputSymlink> symlinks = + ImmutableList.of(filesetSymlink("bar", EXEC_ROOT.getRelative("foo/bar").getPathString())); + + FilesetManifest manifest = + FilesetManifest.constructFilesetManifest(symlinks, PathFragment.create("out/foo"), IGNORE); + + assertThat(manifest.getEntries()) + .containsExactly(PathFragment.create("out/foo/bar"), "foo/bar"); + } }