Allows symlink tree actions to construct fileset links without consum…

…ing the input manifest. Instead, construct the links directly from the Fileset links propagated through the action execution values.

PiperOrigin-RevId: 292183775

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

@@ -373,7 +373,7 @@ private static Artifact createRunfilesAction(
                 inputManifest,
                 runfiles,
                 outputManifest,
-                /*filesetTree=*/ false));
+                /*filesetRoot=*/ null));
     return outputManifest;
   }
 

src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java

@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.util.Fingerprint;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import javax.annotation.Nullable;
 
 /**
@@ -40,12 +41,12 @@
 @AutoCodec
 public final class SymlinkTreeAction extends AbstractAction {
 
-  private static final String GUID = "63412bda-4026-4c8e-a3ad-7deb397728d4";
+  private static final String GUID = "7a16371c-cd4a-494d-b622-963cd89f5212";
 
   @Nullable private final Artifact inputManifest;
   private final Runfiles runfiles;
   private final Artifact outputManifest;
-  private final boolean filesetTree;
+  @Nullable private final String filesetRoot;
   private final boolean enableRunfiles;
   private final boolean inprocessSymlinkCreation;
   private final boolean skipRunfilesManifests;
@@ -59,21 +60,21 @@ public final class SymlinkTreeAction extends AbstractAction {
    * @param runfiles the input runfiles
    * @param outputManifest the generated symlink tree manifest (must have "MANIFEST" base name).
    *     Symlink tree root will be set to the artifact's parent directory.
-   * @param filesetTree true if this is fileset symlink tree
+   * @param filesetRoot non-null if this is a fileset symlink tree
    */
   public SymlinkTreeAction(
       ActionOwner owner,
       BuildConfiguration config,
       Artifact inputManifest,
       @Nullable Runfiles runfiles,
       Artifact outputManifest,
-      boolean filesetTree) {
+      String filesetRoot) {
     this(
         owner,
         inputManifest,
         runfiles,
         outputManifest,
-        filesetTree,
+        filesetRoot,
         config.getActionEnvironment(),
         config.runfilesEnabled(),
         config.inprocessSymlinkCreation(),
@@ -90,37 +91,37 @@ public SymlinkTreeAction(
    * @param runfiles the input runfiles
    * @param outputManifest the generated symlink tree manifest (must have "MANIFEST" base name).
    *     Symlink tree root will be set to the artifact's parent directory.
-   * @param filesetTree true if this is fileset symlink tree,
+   * @param filesetRoot non-null if this is a fileset symlink tree,
    */
   @AutoCodec.Instantiator
   public SymlinkTreeAction(
       ActionOwner owner,
       Artifact inputManifest,
       @Nullable Runfiles runfiles,
       Artifact outputManifest,
-      boolean filesetTree,
+      @Nullable String filesetRoot,
       ActionEnvironment env,
       boolean enableRunfiles,
       boolean inprocessSymlinkCreation,
       boolean skipRunfilesManifests) {
     super(
         owner,
-        skipRunfilesManifests && enableRunfiles && !filesetTree
+        skipRunfilesManifests && enableRunfiles && (filesetRoot == null)
             ? NestedSetBuilder.emptySet(Order.STABLE_ORDER)
             : NestedSetBuilder.create(Order.STABLE_ORDER, inputManifest),
         ImmutableSet.of(outputManifest),
         env);
     Preconditions.checkArgument(outputManifest.getPath().getBaseName().equals("MANIFEST"));
     Preconditions.checkArgument(
-        (runfiles == null) == filesetTree, "Runfiles must be null iff this is a fileset action");
-    this.inputManifest =
-        skipRunfilesManifests && enableRunfiles && !filesetTree ? null : inputManifest;
+        (runfiles == null) == (filesetRoot != null),
+        "Runfiles must be null iff this is a fileset action");
     this.runfiles = runfiles;
     this.outputManifest = outputManifest;
-    this.filesetTree = filesetTree;
+    this.filesetRoot = filesetRoot;
     this.enableRunfiles = enableRunfiles;
     this.inprocessSymlinkCreation = inprocessSymlinkCreation;
-    this.skipRunfilesManifests = skipRunfilesManifests && enableRunfiles && !filesetTree;
+    this.skipRunfilesManifests = skipRunfilesManifests && enableRunfiles && (filesetRoot == null);
+    this.inputManifest = this.skipRunfilesManifests ? null : inputManifest;
   }
 
   public Artifact getInputManifest() {
@@ -137,7 +138,11 @@ public Artifact getOutputManifest() {
   }
 
   public boolean isFilesetTree() {
-    return filesetTree;
+    return filesetRoot != null;
+  }
+
+  public PathFragment getFilesetRoot() {
+    return PathFragment.create(filesetRoot);
   }
 
   public boolean isRunfilesEnabled() {
@@ -148,25 +153,21 @@ public boolean inprocessSymlinkCreation() {
     return inprocessSymlinkCreation;
   }
 
-  public boolean skipRunfilesManifests() {
-    return skipRunfilesManifests;
-  }
-
   @Override
   public String getMnemonic() {
     return "SymlinkTree";
   }
 
   @Override
   protected String getRawProgressMessage() {
-    return (filesetTree ? "Creating Fileset tree " : "Creating runfiles tree ")
+    return (isFilesetTree() ? "Creating Fileset tree " : "Creating runfiles tree ")
         + outputManifest.getExecPath().getParentDirectory().getPathString();
   }
 
   @Override
   protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) {
     fp.addString(GUID);
-    fp.addBoolean(filesetTree);
+    fp.addNullableString(filesetRoot);
     fp.addBoolean(enableRunfiles);
     fp.addBoolean(inprocessSymlinkCreation);
     fp.addBoolean(skipRunfilesManifests);

src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java

@@ -13,14 +13,16 @@
 // limitations under the License.
 package com.google.devtools.build.lib.exec;
 
-import static java.nio.charset.StandardCharsets.ISO_8859_1;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
 import com.google.devtools.build.lib.shell.Command;
 import com.google.devtools.build.lib.shell.CommandException;
 import com.google.devtools.build.lib.util.CommandBuilder;
@@ -33,9 +35,7 @@
 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 java.io.BufferedReader;
 import java.io.IOException;
-import java.io.InputStreamReader;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -184,32 +184,13 @@ Command createCommand(Path execRoot, BinTools binTools, Map<String, String> shel
         .build();
   }
 
-  static Map<PathFragment, PathFragment> readSymlinksFromFilesetManifest(Path manifest)
-      throws IOException {
-    Map<PathFragment, PathFragment> result = new HashMap<>();
-    try (BufferedReader reader =
-        new BufferedReader(
-            new InputStreamReader(
-                // ISO_8859 is used to write the manifest in {Runfiles,Fileset}ManifestAction.
-                manifest.getInputStream(), ISO_8859_1))) {
-      String line;
-      int lineNumber = 0;
-      while ((line = reader.readLine()) != null) {
-        // If the input has metadata (for fileset), they appear in every other line.
-        if (++lineNumber % 2 == 0) {
-          continue;
-        }
-        int spaceIndex = line.indexOf(' ');
-        result.put(
-            PathFragment.create(line.substring(0, spaceIndex)),
-            PathFragment.create(line.substring(spaceIndex + 1)));
-      }
-      if (lineNumber % 2 != 0) {
-        throw new IOException(
-            "Possibly corrupted manifest file '" + manifest.getPathString() + "'");
-      }
+  static ImmutableMap<PathFragment, PathFragment> processFilesetLinks(
+      ImmutableList<FilesetOutputSymlink> links, PathFragment root, PathFragment execRoot) {
+    Map<PathFragment, PathFragment> symlinks = new HashMap<>();
+    for (FilesetOutputSymlink symlink : links) {
+      symlinks.put(root.getRelative(symlink.getName()), symlink.reconstituteTargetPath(execRoot));
     }
-    return result;
+    return ImmutableMap.copyOf(symlinks);
   }
 
   private static final class Directory {

src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java

@@ -80,12 +80,14 @@ public void createSymlinks(
           } else {
             Preconditions.checkState(action.isFilesetTree());
             Preconditions.checkNotNull(inputManifest);
-            try {
-              symlinks = SymlinkTreeHelper.readSymlinksFromFilesetManifest(inputManifest);
-            } catch (IOException e) {
-              throw new EnvironmentalExecException(
-                  "Failed to read from manifest '" + inputManifest.getPathString() + "'", e);
-            }
+
+            symlinks =
+                SymlinkTreeHelper.processFilesetLinks(
+                    actionExecutionContext
+                        .getArtifactExpander()
+                        .getFileset(action.getInputManifest()),
+                    action.getFilesetRoot(),
+                    actionExecutionContext.getExecRoot().asFragment());
           }
 
           outputService.createSymlinkTree(

src/main/java/com/google/devtools/build/lib/rules/android/NativeLibs.java

@@ -193,7 +193,7 @@ ManifestAndRunfiles createApkBuilderSymlinks(RuleContext ruleContext) {
             inputManifest,
             runfiles,
             outputManifest,
-            /*filesetTree=*/ false));
+            /* filesetRoot = */ null));
     return new ManifestAndRunfiles(outputManifest, runfiles);
   }
 

src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeActionTest.java

@@ -69,7 +69,7 @@ public void testComputeKey() throws Exception {
                         ? new Runfiles.Builder("TESTING", false).addArtifact(runfile).build()
                         : new Runfiles.Builder("TESTING", false).addArtifact(runfile2).build(),
                     outputManifest,
-                    /*filesetTree=*/ false,
+                    /*filesetRoot=*/ null,
                     createActionEnvironment(
                         attributesToFlip.contains(RunfilesActionAttributes.FIXED_ENVIRONMENT),
                         attributesToFlip.contains(RunfilesActionAttributes.VARIABLE_ENVIRONMENT)),
@@ -84,7 +84,7 @@ public void testComputeKey() throws Exception {
                     inputManifest,
                     /*runfiles=*/ null,
                     outputManifest,
-                    /*filesetTree=*/ true,
+                    /*filesetRoot=*/ "root",
                     createActionEnvironment(
                         attributesToFlip.contains(FilesetActionAttributes.FIXED_ENVIRONMENT),
                         attributesToFlip.contains(FilesetActionAttributes.VARIABLE_ENVIRONMENT)),
@@ -102,7 +102,7 @@ public void testComputeKey() throws Exception {
                         ? new Runfiles.Builder("TESTING", false).addArtifact(runfile).build()
                         : new Runfiles.Builder("TESTING", false).addArtifact(runfile2).build(),
                     outputManifest,
-                    /*filesetTree=*/ false,
+                    /*filesetRoot=*/ null,
                     createActionEnvironment(
                         attributesToFlip.contains(SkipManifestAttributes.FIXED_ENVIRONMENT),
                         attributesToFlip.contains(SkipManifestAttributes.VARIABLE_ENVIRONMENT)),
@@ -130,7 +130,7 @@ public void testNullRunfilesThrows() {
                 inputManifest,
                 /*runfiles=*/ null,
                 outputManifest,
-                /*filesetTree=*/ false,
+                /*filesetRoot=*/ null,
                 createActionEnvironment(false, false),
                 /*enableRunfiles=*/ true,
                 /*inprocessSymlinkCreation=*/ false,

src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java

@@ -15,18 +15,16 @@
 package com.google.devtools.build.lib.exec;
 
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
+import com.google.devtools.build.lib.actions.HasDigest;
 import com.google.devtools.build.lib.shell.Command;
 import com.google.devtools.build.lib.vfs.FileSystem;
-import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
-import java.io.FileNotFoundException;
-import java.io.IOException;
 import java.util.Map;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -56,50 +54,53 @@ public void checkCreatedSpawn() {
   }
 
   @Test
-  public void readManifest() throws Exception {
-    Path execRoot = fs.getPath("/my/workspace");
-    execRoot.createDirectoryAndParents();
-    Path inputManifestPath = execRoot.getRelative("input_manifest");
-    FileSystemUtils.writeContentAsLatin1(inputManifestPath, "from to\nmetadata");
+  public void readManifest() {
+    PathFragment execRoot = PathFragment.create("/my/workspace");
+
+    FilesetOutputSymlink link =
+        FilesetOutputSymlink.createForTesting(
+            PathFragment.create("from"), PathFragment.create("to"), execRoot);
+
     Map<PathFragment, PathFragment> symlinks =
-        SymlinkTreeHelper.readSymlinksFromFilesetManifest(inputManifestPath);
-    assertThat(symlinks).containsExactly(PathFragment.create("from"), PathFragment.create("to"));
+        SymlinkTreeHelper.processFilesetLinks(
+            ImmutableList.of(link), PathFragment.create("root"), execRoot);
+    assertThat(symlinks)
+        .containsExactly(PathFragment.create("root/from"), PathFragment.create("to"));
   }
 
   @Test
-  public void readMultilineManifest() throws Exception {
-    Path execRoot = fs.getPath("/my/workspace");
-    execRoot.createDirectoryAndParents();
-    Path inputManifestPath = execRoot.getRelative("input_manifest");
-    FileSystemUtils.writeContentAsLatin1(
-        inputManifestPath, "from to\nmetadata\n/foo /bar\nmetadata");
+  public void readMultilineManifest() {
+    PathFragment execRoot = PathFragment.create("/my/workspace");
+
+    FilesetOutputSymlink link1 =
+        FilesetOutputSymlink.createForTesting(
+            PathFragment.create("from"), PathFragment.create("to"), execRoot);
+    FilesetOutputSymlink link2 =
+        FilesetOutputSymlink.createForTesting(
+            PathFragment.create("foo"), PathFragment.create("/bar"), execRoot);
+    FilesetOutputSymlink link3 =
+        FilesetOutputSymlink.createAlreadyRelativized(
+            PathFragment.create("rel"), PathFragment.create("path"), HasDigest.EMPTY, true, true);
+    FilesetOutputSymlink link4 =
+        FilesetOutputSymlink.createAlreadyRelativized(
+            PathFragment.create("rel2"),
+            PathFragment.create("/path"),
+            HasDigest.EMPTY,
+            false,
+            false);
+
     Map<PathFragment, PathFragment> symlinks =
-        SymlinkTreeHelper.readSymlinksFromFilesetManifest(inputManifestPath);
+        SymlinkTreeHelper.processFilesetLinks(
+            ImmutableList.of(link1, link2, link3, link4), PathFragment.create("root2"), execRoot);
     assertThat(symlinks)
         .containsExactly(
-            PathFragment.create("from"),
+            PathFragment.create("root2/from"),
             PathFragment.create("to"),
-            PathFragment.create("/foo"),
-            PathFragment.create("/bar"));
-  }
-
-  @Test
-  public void readCorruptManifest() throws Exception {
-    Path execRoot = fs.getPath("/my/workspace");
-    execRoot.createDirectoryAndParents();
-    Path inputManifestPath = execRoot.getRelative("input_manifest");
-    FileSystemUtils.writeContentAsLatin1(inputManifestPath, "from to");
-    assertThrows(
-        IOException.class,
-        () -> SymlinkTreeHelper.readSymlinksFromFilesetManifest(inputManifestPath));
-  }
-
-  @Test
-  public void readNonExistentManifestFails() {
-    Path execRoot = fs.getPath("/my/workspace");
-    Path inputManifestPath = execRoot.getRelative("input_manifest");
-    assertThrows(
-        FileNotFoundException.class,
-        () -> SymlinkTreeHelper.readSymlinksFromFilesetManifest(inputManifestPath));
+            PathFragment.create("root2/foo"),
+            PathFragment.create("/bar"),
+            PathFragment.create("root2/rel"),
+            execRoot.getRelative("path"),
+            PathFragment.create("root2/rel2"),
+            PathFragment.create("/path"));
   }
 }