Strip the execution root from target paths in FilesetOutputSymlink. RELNOTES: None. PiperOrigin-RevId: 215754628
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetOutputSymlink.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetOutputSymlink.java index fd89e58..73a9499 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FilesetOutputSymlink.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FilesetOutputSymlink.java
@@ -26,8 +26,16 @@ public abstract PathFragment getName(); /** - * Target of the symlink. This may be relative to the target's location if the target itself is a - * relative symlink. We can override it by using FilesetEntry.symlinks = 'dereference'. + * Target of the symlink. + * + * <p>This path is one of the following: + * + * <ol> + * <li>Relative to the execution root, in which case {@link #isRelativeToExecRoot} will return + * {@code true}. + * <li>An absolute path to the source tree. + * <li>A relative path that should be considered relative to the link. + * </ol> */ public abstract PathFragment getTargetPath(); @@ -41,8 +49,21 @@ /** true if the target is a generated artifact */ public abstract boolean isGeneratedTarget(); + /** Returns {@code true} if this symlink is relative to the execution root. */ + public abstract boolean isRelativeToExecRoot(); + + /** + * Reconstitutes the original target path of this symlink. + * + * <p>This method essentially performs the inverse of what is done in {@link #create}. If the + * execution root was stripped originally, it is re-prepended. + */ + public final PathFragment reconstituteTargetPath(PathFragment execRoot) { + return isRelativeToExecRoot() ? execRoot.getRelative(getTargetPath()) : getTargetPath(); + } + @Override - public String toString() { + public final String toString() { if (getMetadata() == STRIPPED_METADATA) { return String.format( "FilesetOutputSymlink(%s -> %s)", @@ -55,19 +76,60 @@ } @VisibleForTesting - public static FilesetOutputSymlink createForTesting(PathFragment name, PathFragment target) { - return new AutoValue_FilesetOutputSymlink(name, target, STRIPPED_METADATA, false); + public static FilesetOutputSymlink createForTesting( + PathFragment name, PathFragment target, PathFragment execRoot) { + return create(name, target, STRIPPED_METADATA, false, execRoot); + } + + @VisibleForTesting + public static FilesetOutputSymlink createAlreadyRelativizedForTesting( + PathFragment name, PathFragment target, boolean isRelativeToExecRoot) { + return createAlreadyRelativized(name, target, STRIPPED_METADATA, false, isRelativeToExecRoot); } /** + * Creates a {@link FilesetOutputSymlink}. + * + * <p>To facilitate cross-device sharing, {@code target} will have the machine-local {@code + * execRoot} stripped if necessary. If this happens, {@link #isRelativeToExecRoot} will return + * {@code true}. + * * @param name relative path under the Fileset's output directory, including FilesetEntry.destdir * with and FilesetEntry.strip_prefix applied (if applicable) * @param target relative or absolute value of the link * @param metadata metadata corresponding to the target. * @param isGeneratedTarget true if the target is generated. + * @param execRoot the execution root */ public static FilesetOutputSymlink create( - PathFragment name, PathFragment target, Object metadata, boolean isGeneratedTarget) { - return new AutoValue_FilesetOutputSymlink(name, target, metadata, isGeneratedTarget); + PathFragment name, + PathFragment target, + Object metadata, + boolean isGeneratedTarget, + PathFragment execRoot) { + boolean isRelativeToExecRoot = false; + // Check if the target is under the execution root. This is not always the case because the + // target may point to a source artifact or it may point to another symlink, in which case the + // target path is already relative. + if (target.startsWith(execRoot)) { + target = target.relativeTo(execRoot); + isRelativeToExecRoot = true; + } + return createAlreadyRelativized( + name, target, metadata, isGeneratedTarget, isRelativeToExecRoot); + } + + /** + * Same as {@link #create}, except assumes that {@code target} already had the execution root + * stripped if necessary. + */ + public static FilesetOutputSymlink createAlreadyRelativized( + PathFragment name, + PathFragment target, + Object metadata, + boolean isGeneratedTarget, + boolean isRelativeToExecRoot) { + return new AutoValue_FilesetOutputSymlink( + name, target, metadata, isGeneratedTarget, isRelativeToExecRoot); } }
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 133aaf3..53bfb09 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,6 +22,7 @@ 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; @@ -358,7 +359,8 @@ FilesetManifest.constructFilesetManifest( artifactExpander.getFileset(fileset), fileset.getExecPath(), - RelativeSymlinkBehavior.IGNORE); + RelativeSymlinkBehavior.IGNORE, + extractExecRoot(fileset)); for (PathFragment relativePath : filesetManifest.getEntries().keySet()) { expandedValues.add(new FilesetSymlinkFile(fileset, relativePath)); } @@ -367,6 +369,14 @@ } } + // 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 40e3842..220beec 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
@@ -16,6 +16,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.base.Preconditions; +import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; import com.google.common.io.LineProcessor; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -75,16 +76,17 @@ public static FilesetManifest constructFilesetManifest( List<FilesetOutputSymlink> outputSymlinks, PathFragment targetPrefix, - RelativeSymlinkBehavior relSymlinkbehavior) + RelativeSymlinkBehavior relSymlinkBehavior, + PathFragment execRoot) 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()); - String artifact = outputSymlink.getTargetPath().getPathString(); - artifact = artifact.isEmpty() ? null : artifact; - addSymlinkEntry(artifact, fullLocation, relSymlinkbehavior, entries, relativeLinks); + PathFragment linkTarget = outputSymlink.reconstituteTargetPath(execRoot); + String artifact = Strings.emptyToNull(linkTarget.getPathString()); + addSymlinkEntry(artifact, fullLocation, relSymlinkBehavior, entries, relativeLinks); if (outputSymlink.getMetadata() instanceof FileArtifactValue) { artifactValues.put(artifact, (FileArtifactValue) outputSymlink.getMetadata()); }
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 113ff88..25d4c81 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
@@ -182,7 +182,7 @@ ImmutableList<FilesetOutputSymlink> outputSymlinks = filesetMappings.get(fileset); FilesetManifest filesetManifest = FilesetManifest.constructFilesetManifest( - outputSymlinks, fileset.getExecPath(), relSymlinkBehavior); + outputSymlinks, fileset.getExecPath(), relSymlinkBehavior, execRoot.asFragment()); for (Map.Entry<PathFragment, String> mapping : filesetManifest.getEntries().entrySet()) { String value = mapping.getValue();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java index aa155b1..5312ca9 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunction.java
@@ -47,6 +47,12 @@ } } + private final PathFragment execRoot; + + public FilesetEntryFunction(PathFragment execRoot) { + this.execRoot = execRoot; + } + @Override public SkyValue compute(SkyKey key, Environment env) throws FilesetEntryFunctionException, InterruptedException { @@ -180,7 +186,7 @@ } /** Stores an output symlink unless it would overwrite an existing one. */ - private static void maybeStoreSymlink( + private void maybeStoreSymlink( PathFragment linkName, PathFragment linkTarget, Object metadata, @@ -190,7 +196,8 @@ linkName = destPath.getRelative(linkName); if (!result.containsKey(linkName)) { result.put( - linkName, FilesetOutputSymlink.create(linkName, linkTarget, metadata, isGenerated)); + linkName, + FilesetOutputSymlink.create(linkName, linkTarget, metadata, isGenerated, execRoot)); } }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index 300cd4e..bf7ae1d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -552,7 +552,9 @@ this.actionExecutionFunction = actionExecutionFunction; map.put(SkyFunctions.RECURSIVE_FILESYSTEM_TRAVERSAL, new RecursiveFilesystemTraversalFunction()); - map.put(SkyFunctions.FILESET_ENTRY, new FilesetEntryFunction()); + map.put( + SkyFunctions.FILESET_ENTRY, + new FilesetEntryFunction(directories.getExecRoot().asFragment())); map.put( SkyFunctions.ACTION_TEMPLATE_EXPANSION, new ActionTemplateExpansionFunction(actionKeyContext));
diff --git a/src/test/java/com/google/devtools/build/lib/actions/FilesetOutputSymlinkTest.java b/src/test/java/com/google/devtools/build/lib/actions/FilesetOutputSymlinkTest.java new file mode 100644 index 0000000..bc526c6 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/actions/FilesetOutputSymlinkTest.java
@@ -0,0 +1,61 @@ +// Copyright 2018 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.actions; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.devtools.build.lib.vfs.PathFragment; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link FilesetOutputSymlink}. */ +@RunWith(JUnit4.class) +public final class FilesetOutputSymlinkTest { + + private static final PathFragment EXEC_ROOT = PathFragment.create("/example/execroot"); + + private static final FilesetOutputSymlink createSymlinkTo(String path) { + return FilesetOutputSymlink.create( + PathFragment.create("any/path"), PathFragment.create(path), new Object(), false, EXEC_ROOT); + } + + @Test + public void stripsExecRootFromTarget() throws Exception { + FilesetOutputSymlink symlink = createSymlinkTo("/example/execroot/some/path"); + PathFragment targetPath = symlink.getTargetPath(); + assertThat(targetPath.getPathString()).isEqualTo("some/path"); + } + + @Test + public void reconstitutesTargetPath_underExecRoot() throws Exception { + FilesetOutputSymlink symlink = createSymlinkTo("/example/execroot/some/path"); + PathFragment targetPath = symlink.reconstituteTargetPath(EXEC_ROOT); + assertThat(targetPath.getPathString()).isEqualTo("/example/execroot/some/path"); + } + + @Test + public void reconstitutesTargetPath_differentExecRoot() throws Exception { + FilesetOutputSymlink symlink = createSymlinkTo("/example/execroot/some/path"); + PathFragment targetPath = symlink.reconstituteTargetPath(PathFragment.create("/new/execroot")); + assertThat(targetPath.getPathString()).isEqualTo("/new/execroot/some/path"); + } + + @Test + public void reconstitutesTargetPath_notUnderExecRoot() throws Exception { + FilesetOutputSymlink symlink = createSymlinkTo("some/path"); + PathFragment targetPath = symlink.reconstituteTargetPath(EXEC_ROOT); + assertThat(targetPath.getPathString()).isEqualTo("some/path"); + } +}
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 b7fd511..5b59a16 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
@@ -388,16 +388,16 @@ .containsEntry(PathFragment.create("out/foo/bar"), ActionInputHelper.fromPath("/some")); } - private FilesetOutputSymlink filesetSymlink(String from, String to) { + private static FilesetOutputSymlink filesetSymlink(String from, String to) { return FilesetOutputSymlink.createForTesting( - PathFragment.create(from), PathFragment.create(to)); + PathFragment.create(from), PathFragment.create(to), PathFragment.create("/root")); } private ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> simpleFilesetManifest() { return ImmutableMap.of( createFileset("out"), ImmutableList.of( - filesetSymlink("workspace/bar", "foo"), filesetSymlink("workspace/foo", "/foo/bar"))); + filesetSymlink("workspace/bar", "foo"), filesetSymlink("workspace/foo", "/root/bar"))); } private SpecialArtifact createFileset(String execPath) { @@ -426,7 +426,7 @@ expander.addFilesetManifests(simpleFilesetManifest(), entries); assertThat(entries) .containsExactly( - PathFragment.create("out/workspace/foo"), ActionInputHelper.fromPath("/foo/bar")); + PathFragment.create("out/workspace/foo"), ActionInputHelper.fromPath("/root/bar")); } @Test @@ -436,7 +436,7 @@ expander.addFilesetManifests(simpleFilesetManifest(), entries); assertThat(entries) .containsExactly( - PathFragment.create("out/workspace/bar"), ActionInputHelper.fromPath("/foo/bar"), - PathFragment.create("out/workspace/foo"), ActionInputHelper.fromPath("/foo/bar")); + PathFragment.create("out/workspace/bar"), ActionInputHelper.fromPath("/root/bar"), + PathFragment.create("out/workspace/foo"), ActionInputHelper.fromPath("/root/bar")); } }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java index 5e5ff14..f82b763 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java
@@ -59,10 +59,8 @@ import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; -import java.util.Arrays; import java.util.Collection; import java.util.HashMap; -import java.util.List; import java.util.Map; import java.util.Set; import java.util.UUID; @@ -124,7 +122,8 @@ new BlacklistedPackagePrefixesFunction( /*hardcodedBlacklistedPackagePrefixes=*/ ImmutableSet.of(), /*additionalBlacklistedPackagePrefixesFile=*/ PathFragment.EMPTY_FRAGMENT)); - skyFunctions.put(SkyFunctions.FILESET_ENTRY, new FilesetEntryFunction()); + skyFunctions.put( + SkyFunctions.FILESET_ENTRY, new FilesetEntryFunction(rootDirectory.asFragment())); skyFunctions.put(SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction()); differencer = new SequencedRecordingDifferencer(); @@ -191,31 +190,32 @@ return result.get(key); } - private static FilesetOutputSymlink symlink(String from, Artifact to) { - return FilesetOutputSymlink.createForTesting( - PathFragment.create(from), to.getPath().asFragment()); + private FilesetOutputSymlink symlink(String from, Artifact to) { + return symlink(PathFragment.create(from), to.getPath().asFragment()); } - private static FilesetOutputSymlink symlink(String from, String to) { - return FilesetOutputSymlink.createForTesting( - PathFragment.create(from), PathFragment.create(to)); + private FilesetOutputSymlink symlink(String from, String to) { + return symlink(PathFragment.create(from), PathFragment.create(to)); } - private static FilesetOutputSymlink symlink(String from, RootedPath to) { - return FilesetOutputSymlink.createForTesting( - PathFragment.create(from), to.asPath().asFragment()); + private FilesetOutputSymlink symlink(String from, RootedPath to) { + return symlink(PathFragment.create(from), to.asPath().asFragment()); + } + + private FilesetOutputSymlink symlink(PathFragment from, PathFragment to) { + return FilesetOutputSymlink.createForTesting(from, to, rootDirectory.asFragment()); } private void assertSymlinksCreatedInOrder( FilesetTraversalParams request, FilesetOutputSymlink... expectedSymlinks) throws Exception { - List<FilesetOutputSymlink> expected = Arrays.asList(expectedSymlinks); Collection<FilesetOutputSymlink> actual = Collections2.transform( evalFilesetTraversal(request).getSymlinks(), // Strip the metadata from the actual results. (input) -> - FilesetOutputSymlink.createForTesting(input.getName(), input.getTargetPath())); - assertThat(actual).containsExactlyElementsIn(expected).inOrder(); + FilesetOutputSymlink.createAlreadyRelativizedForTesting( + input.getName(), input.getTargetPath(), input.isRelativeToExecRoot())); + assertThat(actual).containsExactlyElementsIn(expectedSymlinks).inOrder(); } private static Label label(String label) throws Exception {