Prohibit creating a FilesetOutputSymlink with an empty name or targ…

…et path. This was only happening in tests. Also clean up some dead code in `LocalFilesScanner` and `SpawnInputExpander` for handling such a case, which used to represent an empty file many years ago. `FilesetManifestTest` is updated to use `TestParameterInjector`. PiperOrigin-RevId: 681025803 Change-Id: I3c40b94d91781f0e2cacebc460f27d867f9e5aa3
bazelbuild · Oct 1, 2024 · 499809d · 499809d
1 parent a62a98f
commit 499809d
Show file tree

Hide file tree

Showing 5 changed files with 23 additions and 61 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetManifest.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetManifest.java
@@ -16,7 +16,6 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 
-import com.google.common.base.Strings;
 import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -30,7 +29,6 @@
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
-import javax.annotation.Nullable;
 
 /**
  * Representation of a Fileset manifest.
@@ -108,42 +106,40 @@ public static FilesetManifest constructFilesetManifest(
     Map<String, FileArtifactValue> artifactValues = new HashMap<>();
     for (FilesetOutputSymlink outputSymlink : outputSymlinks) {
       PathFragment fullLocation = targetPrefix.getRelative(outputSymlink.getName());
-      String artifact = Strings.emptyToNull(outputSymlink.getTargetPath().getPathString());
+      String targetPath = outputSymlink.getTargetPath().getPathString();
       if (isRelativeSymlink(outputSymlink)) {
-        addRelativeSymlinkEntry(artifact, fullLocation, relSymlinkBehavior, relativeLinks);
+        addRelativeSymlinkEntry(targetPath, fullLocation, relSymlinkBehavior, relativeLinks);
       } else {
         // Symlinks are already deduplicated by name in SkyframeFilesetManifestAction.
         checkState(
-            entries.put(fullLocation, artifact) == null,
+            entries.put(fullLocation, targetPath) == null,
             "Duplicate fileset entry at %s",
             fullLocation);
       }
       if (outputSymlink.getMetadata() instanceof FileArtifactValue) {
-        artifactValues.put(artifact, (FileArtifactValue) outputSymlink.getMetadata());
+        artifactValues.put(targetPath, (FileArtifactValue) outputSymlink.getMetadata());
       }
     }
     resolveRelativeSymlinks(entries, relativeLinks, targetPrefix.isAbsolute(), relSymlinkBehavior);
     return new FilesetManifest(entries, artifactValues);
   }
 
   private static boolean isRelativeSymlink(FilesetOutputSymlink symlink) {
-    return !symlink.getTargetPath().isEmpty()
-        && !symlink.getTargetPath().isAbsolute()
-        && !symlink.isRelativeToExecRoot();
+    return !symlink.getTargetPath().isAbsolute() && !symlink.isRelativeToExecRoot();
   }
 
   /** Potentially adds the relative symlink to the map, depending on {@code relSymlinkBehavior}. */
   private static void addRelativeSymlinkEntry(
-      @Nullable String artifact,
+      String targetPath,
       PathFragment fullLocation,
       RelativeSymlinkBehavior relSymlinkBehavior,
       Map<PathFragment, String> relativeLinks)
       throws ForbiddenRelativeSymlinkException {
     switch (relSymlinkBehavior) {
-      case ERROR -> throw new ForbiddenRelativeSymlinkException(artifact);
+      case ERROR -> throw new ForbiddenRelativeSymlinkException(targetPath);
       case RESOLVE, RESOLVE_FULLY ->
           checkState(
-              relativeLinks.put(fullLocation, artifact) == null,
+              relativeLinks.put(fullLocation, targetPath) == null,
               "Duplicate fileset entry at %s",
               fullLocation);
       case IGNORE -> {
@@ -172,7 +168,7 @@ private static void fullyResolveRelativeSymlinks(
         PathFragment location = e.getKey();
         Path locationPath = root.getRelative(location);
         locationPath.getParentDirectory().createDirectoryAndParents();
-        FileSystemUtils.writeContentAsLatin1(locationPath, Strings.nullToEmpty(e.getValue()));
+        FileSystemUtils.writeContentAsLatin1(locationPath, e.getValue());
       }
       for (Map.Entry<PathFragment, String> e : relativeLinks.entrySet()) {
         PathFragment location = e.getKey();
@@ -198,9 +194,7 @@ private static void addSymlinks(Path root, Map<PathFragment, String> entries, bo
           addSymlinks(path, entries, absolute);
         } else {
           String contents = new String(FileSystemUtils.readContentAsLatin1(path));
-          entries.put(
-              absolute ? path.asFragment() : path.asFragment().toRelative(),
-              Strings.emptyToNull(contents));
+          entries.put(absolute ? path.asFragment() : path.asFragment().toRelative(), contents);
         }
       } catch (IOException e) {
         logger.atWarning().log("Symlink %s is dangling or cyclic: %s", path, e.getMessage());
@@ -279,7 +273,6 @@ private FilesetManifest(
    * <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() {

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
@@ -152,6 +152,8 @@ public static FilesetOutputSymlink createAlreadyRelativized(
       HasDigest metadata,
       boolean isRelativeToExecRoot,
       @Nullable PathFragment enclosingTreeArtifactExecPath) {
+    checkArgument(!name.isEmpty(), "Empty symlink name pointing to %s", target);
+    checkArgument(!target.isEmpty(), "Empty symlink target for %s", name);
     return new AutoValue_FilesetOutputSymlink(
         name, target, metadata, isRelativeToExecRoot, enclosingTreeArtifactExecPath);
   }

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
@@ -193,10 +193,7 @@ private void addFilesetManifest(
 
     for (Map.Entry<PathFragment, String> mapping : filesetManifest.getEntries().entrySet()) {
       String value = mapping.getValue();
-      ActionInput artifact =
-          value == null
-              ? VirtualActionInput.EMPTY_MARKER
-              : ActionInputHelper.fromPath(execRoot.getRelative(value).asFragment());
+      ActionInput artifact = ActionInputHelper.fromPath(execRoot.getRelative(value).asFragment());
       // TODO(bazel-team): Add path mapping support for filesets.
       addMapping(inputMap, mapping.getKey(), artifact, baseDirectory);
     }

diff --git a/...t/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java b/...t/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLineTest.java
@@ -392,7 +392,9 @@ private SpecialArtifact createFileset(String relativePath) {
 
   private FilesetOutputSymlink createFilesetSymlink(String relativePath) {
     return FilesetOutputSymlink.createForTesting(
-        PathFragment.create(relativePath), PathFragment.EMPTY_FRAGMENT, execRoot.asFragment());
+        PathFragment.create(relativePath),
+        execRoot.asFragment().getRelative("some/target"),
+        execRoot.asFragment());
   }
 
   private SpecialArtifact createTreeArtifact(String relativePath) {

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
@@ -16,11 +16,9 @@
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehavior.ERROR;
 import static com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehavior.IGNORE;
-import static com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehavior.RESOLVE;
 import static com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehavior.RESOLVE_FULLY;
 import static org.junit.Assert.assertThrows;
 
-import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.FilesetManifest;
 import com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehavior;
@@ -29,11 +27,12 @@
 import com.google.devtools.build.lib.exec.FilesetManifestTest.OneOffManifestTests;
 import com.google.devtools.build.lib.exec.FilesetManifestTest.ResolvingManifestTests;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import java.util.List;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
-import org.junit.runners.Parameterized;
 import org.junit.runners.Suite;
 
 /** Tests for {@link FilesetManifest}. */
@@ -53,22 +52,10 @@ private static FilesetOutputSymlink filesetSymlink(String from, String to) {
   }
 
   /** Manifest tests that apply to all relative symlink behavior. */
-  @RunWith(Parameterized.class)
+  @RunWith(TestParameterInjector.class)
   public static final class ManifestCommonTests {
-    private final RelativeSymlinkBehavior behavior;
-
-    @Parameterized.Parameters
-    public static ImmutableCollection<Object[]> behaviors() {
-      return ImmutableList.of(
-          new Object[] {ERROR},
-          new Object[] {RESOLVE},
-          new Object[] {IGNORE},
-          new Object[] {RESOLVE_FULLY});
-    }
 
-    public ManifestCommonTests(RelativeSymlinkBehavior behavior) {
-      this.behavior = behavior;
-    }
+    @TestParameter private RelativeSymlinkBehavior behavior;
 
     @Test
     public void testEmptyManifest() throws Exception {
@@ -120,18 +107,6 @@ public void testManifestWithDirectory() throws Exception {
           .containsExactly(PathFragment.create("out/foo/bar"), "/some");
     }
 
-    /** Regression test: code was previously crashing in this case. */
-    @Test
-    public void testManifestWithEmptyPath() throws Exception {
-      List<FilesetOutputSymlink> symlinks = ImmutableList.of(filesetSymlink("bar", ""));
-
-      FilesetManifest manifest =
-          FilesetManifest.constructFilesetManifest(
-              symlinks, PathFragment.create("out/foo"), behavior);
-
-      assertThat(manifest.getEntries()).containsExactly(PathFragment.create("out/foo/bar"), null);
-    }
-
     @Test
     public void testManifestWithExecRootRelativePath() throws Exception {
       List<FilesetOutputSymlink> symlinks =
@@ -196,18 +171,11 @@ public void testManifestWithResolvedRelativeDirectorySymlink() throws Exception
   }
 
   /** Manifest tests that apply resolving relative symlink behavior. */
-  @RunWith(Parameterized.class)
+  @RunWith(TestParameterInjector.class)
   public static final class ResolvingManifestTests {
-    private final RelativeSymlinkBehavior behavior;
 
-    @Parameterized.Parameters
-    public static ImmutableCollection<Object[]> behaviors() {
-      return ImmutableList.of(new Object[] {RESOLVE}, new Object[] {RESOLVE_FULLY});
-    }
-
-    public ResolvingManifestTests(RelativeSymlinkBehavior behavior) {
-      this.behavior = behavior;
-    }
+    @TestParameter({"RESOLVE", "RESOLVE_FULLY"})
+    private RelativeSymlinkBehavior behavior;
 
     @Test
     public void testManifestWithResolvedRelativeSymlink() throws Exception {