Add a target_type argument to ctx.actions.symlink.

This is necessary to create the right kind of filesystem object on Windows (junction for directories, symlink for files) when the target does not yet exist.

This argument is only allowed in conjunction with `target_path`. For `target_file`, I have a different plan (infer the type from the artifact) which will be implemented separately.

Fixes #26701.

Progress on #21747.

RELNOTES: `ctx.actions.symlink` now accepts a `target_type` argument.
PiperOrigin-RevId: 820670309
Change-Id: I2f29adfd074c404a0b15be369a97fcdfb84fbdad

Author: tjgq

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java

@@ -70,6 +70,7 @@
 import com.google.devtools.build.lib.supplier.InterruptibleSupplier;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.SymlinkTargetType;
 import com.google.protobuf.GeneratedMessage;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
@@ -250,6 +251,7 @@ public void symlink(
       FileApi output,
       Object /* Artifact or None */ targetFile,
       Object /* String or None */ targetPath,
+      Object /* String or None */ targetType,
       Boolean isExecutable,
       Object /* String or None */ progressMessageUnchecked,
       Object useExecRootForSourceObject,
@@ -277,6 +279,10 @@ public void symlink(
 
     Action action;
     if (targetFile != Starlark.NONE) {
+      if (targetType != Starlark.NONE) {
+        throw Starlark.errorf("\"target_type\" cannot be used with \"target_file\"");
+      }
+
       Artifact inputArtifact = (Artifact) targetFile;
       if (outputArtifact.isSymlink()) {
         throw Starlark.errorf(
@@ -322,9 +328,24 @@ public void symlink(
         throw Starlark.errorf("\"is_executable\" cannot be True when using \"target_path\"");
       }
 
+      SymlinkTargetType symlinkTargetType = SymlinkTargetType.UNSPECIFIED;
+      if (targetType instanceof String targetTypeStr) {
+        symlinkTargetType =
+            switch (targetTypeStr) {
+              case "file" -> SymlinkTargetType.FILE;
+              case "directory" -> SymlinkTargetType.DIRECTORY;
+              default ->
+                  throw Starlark.errorf("\"target_type\" must be one of \"file\" or \"directory\"");
+            };
+      }
+
       action =
           UnresolvedSymlinkAction.create(
-              ruleContext.getActionOwner(), outputArtifact, (String) targetPath, progressMessage);
+              ruleContext.getActionOwner(),
+              outputArtifact,
+              (String) targetPath,
+              symlinkTargetType,
+              progressMessage);
     }
     registerAction(action);
   }

src/main/java/com/google/devtools/build/lib/analysis/starlark/UnresolvedSymlinkAction.java

@@ -34,6 +34,7 @@
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.SymlinkTargetType;
 import java.io.IOException;
 import javax.annotation.Nullable;
 
@@ -47,19 +48,29 @@ public final class UnresolvedSymlinkAction extends AbstractAction {
   private static final String GUID = "0f302651-602c-404b-881c-58913193cfe7";
 
   private final String target;
+  private final SymlinkTargetType targetType;
   private final String progressMessage;
 
   private UnresolvedSymlinkAction(
-      ActionOwner owner, Artifact primaryOutput, String target, String progressMessage) {
+      ActionOwner owner,
+      Artifact primaryOutput,
+      String target,
+      SymlinkTargetType targetType,
+      String progressMessage) {
     super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), ImmutableSet.of(primaryOutput));
     this.target = target;
+    this.targetType = targetType;
     this.progressMessage = progressMessage;
   }
 
   public static UnresolvedSymlinkAction create(
-      ActionOwner owner, Artifact primaryOutput, String target, String progressMessage) {
+      ActionOwner owner,
+      Artifact primaryOutput,
+      String target,
+      SymlinkTargetType targetType,
+      String progressMessage) {
     Preconditions.checkArgument(primaryOutput.isSymlink());
-    return new UnresolvedSymlinkAction(owner, primaryOutput, target, progressMessage);
+    return new UnresolvedSymlinkAction(owner, primaryOutput, target, targetType, progressMessage);
   }
 
   @Override
@@ -68,7 +79,7 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext)
 
     Path outputPath = actionExecutionContext.getInputPath(getPrimaryOutput());
     try {
-      outputPath.createSymbolicLink(getTargetPathFragment());
+      outputPath.createSymbolicLink(getTargetPathFragment(), targetType);
     } catch (IOException e) {
       String message =
           String.format(
@@ -88,11 +99,12 @@ protected void computeKey(
       Fingerprint fp) {
     fp.addString(GUID);
     fp.addString(target);
+    fp.addString(targetType.name());
   }
 
   @Override
   public String describeKey() {
-    return String.format("GUID: %s\ntarget: %s\n", GUID, target);
+    return String.format("GUID: %s\ntarget: %s\ntype: %s\n", GUID, target, targetType.name());
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkActionFactoryApi.java

@@ -235,6 +235,21 @@ public interface StarlarkActionFactoryApi extends StarlarkValue {
             doc =
                 "The exact path that the output symlink will point to. No normalization or other "
                     + "processing is applied."),
+        @Param(
+            name = "target_type",
+            allowedTypes = {
+              @ParamType(type = String.class),
+              @ParamType(type = NoneType.class),
+            },
+            named = true,
+            positional = false,
+            defaultValue = "None",
+            doc =
+                "May only be used with <code>target_path</code>, not <code>target_file</code>. If"
+                    + " specified, it must be one of 'file' or 'directory', indicating the target"
+                    + " path's expected type.<p>On Windows, this determines which kind of"
+                    + " filesystem object to create (junction for a directory, symlink for a file)."
+                    + " It has no effect on other operating systems."),
         @Param(
             name = "is_executable",
             named = true,
@@ -270,6 +285,7 @@ void symlink(
       FileApi output,
       Object targetFile,
       Object targetPath,
+      Object targetType,
       Boolean isExecutable,
       Object progressMessage,
       Object useExecRootForSourceObject,

src/main/java/com/google/devtools/build/lib/vfs/SymlinkTargetType.java

@@ -14,6 +14,8 @@
 //
 package com.google.devtools.build.lib.vfs;
 
+import com.google.devtools.build.lib.skyframe.serialization.EnumCodec;
+
 /**
  * Indicates the file type at the other end of a symlink.
  *
@@ -27,5 +29,19 @@ public enum SymlinkTargetType {
   /** The target is a regular file. */
   FILE,
   /** The target is a directory. */
-  DIRECTORY,
+  DIRECTORY;
+
+  /**
+   * Codec for {@link SymlinkTargetType}.
+   *
+   * <p>{@link com.google.devtools.build.lib.skyframe.serialization.AutoRegistry} excludes the
+   * entire com.google.devtools.build.lib.vfs java package from having DynamicCodec support.
+   * Therefore, we need to provide our own codec.
+   */
+  @SuppressWarnings("unused") // found by CLASSPATH-scanning magic
+  private static final class Codec extends EnumCodec<SymlinkTargetType> {
+    Codec() {
+      super(SymlinkTargetType.class);
+    }
+  }
 }

src/test/java/com/google/devtools/build/lib/analysis/starlark/UnresolvedSymlinkActionTest.java

@@ -40,6 +40,7 @@
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
+import com.google.devtools.build.lib.vfs.SymlinkTargetType;
 import com.google.devtools.build.lib.vfs.SyscallCache;
 import org.junit.Before;
 import org.junit.Test;
@@ -71,6 +72,7 @@ public final void setUp() throws Exception {
             NULL_ACTION_OWNER,
             outputArtifact,
             "../some/relative/path",
+            SymlinkTargetType.UNSPECIFIED,
             "Creating unresolved symlink");
   }
 
@@ -88,12 +90,48 @@ public void testOutputArtifactIsOutput() {
   public void testTargetAffectsKey() {
     UnresolvedSymlinkAction action1 =
         UnresolvedSymlinkAction.create(
-            NULL_ACTION_OWNER, outputArtifact, "some/path", "Creating unresolved symlink");
+            NULL_ACTION_OWNER,
+            outputArtifact,
+            "some/path",
+            SymlinkTargetType.UNSPECIFIED,
+            "Creating unresolved symlink");
     UnresolvedSymlinkAction action2 =
         UnresolvedSymlinkAction.create(
-            NULL_ACTION_OWNER, outputArtifact, "some/other/path", "Creating unresolved symlink");
+            NULL_ACTION_OWNER,
+            outputArtifact,
+            "some/other/path",
+            SymlinkTargetType.UNSPECIFIED,
+            "Creating unresolved symlink");
+
+    assertThat(computeKey(action1)).isNotEqualTo(computeKey(action2));
+  }
+
+  @Test
+  public void testTargetTypeAffectsKey() {
+    UnresolvedSymlinkAction action1 =
+        UnresolvedSymlinkAction.create(
+            NULL_ACTION_OWNER,
+            outputArtifact,
+            "some/path",
+            SymlinkTargetType.UNSPECIFIED,
+            "Creating unresolved symlink");
+    UnresolvedSymlinkAction action2 =
+        UnresolvedSymlinkAction.create(
+            NULL_ACTION_OWNER,
+            outputArtifact,
+            "some/path",
+            SymlinkTargetType.FILE,
+            "Creating unresolved symlink");
+    UnresolvedSymlinkAction action3 =
+        UnresolvedSymlinkAction.create(
+            NULL_ACTION_OWNER,
+            outputArtifact,
+            "some/path",
+            SymlinkTargetType.DIRECTORY,
+            "Creating unresolved symlink");
 
     assertThat(computeKey(action1)).isNotEqualTo(computeKey(action2));
+    assertThat(computeKey(action2)).isNotEqualTo(computeKey(action3));
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/remote/BUILD

@@ -216,6 +216,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/test/java/com/google/devtools/build/lib/buildtool/util",
+        "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
         "//third_party:guava",
         "//third_party:junit4",
         "//third_party:truth",

src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java

@@ -34,12 +34,15 @@
 import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
 import com.google.devtools.build.lib.skyframe.ActionExecutionValue;
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
+import com.google.devtools.build.lib.testutil.TestUtils;
 import com.google.devtools.build.lib.util.CommandBuilder;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.io.RecordingOutErr;
 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.SymlinkTargetType;
+import com.google.testing.junit.testparameterinjector.TestParameter;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
@@ -1032,29 +1035,53 @@ public void downloadToplevel_symlinkToDirectory() throws Exception {
   }
 
   @Test
-  public void downloadToplevel_unresolvedSymlink() throws Exception {
-    // Dangling symlink would require developer mode to be enabled in the CI environment.
-    assumeFalse(OS.getCurrent() == OS.WINDOWS);
+  public void downloadToplevel_unresolvedSymlink(@TestParameter SymlinkTargetType targetType)
+      throws Exception {
+    Path targetPath = TestUtils.createUniqueTmpDir(null).getChild("target");
+
+    String targetPathArg = targetPath.getPathString();
+    String targetTypeArg =
+        switch (targetType) {
+          case FILE -> "file";
+          case DIRECTORY -> "directory";
+          case UNSPECIFIED -> "";
+        };
 
     setDownloadToplevel();
     writeSymlinkRule();
     write(
         "BUILD",
-        "load(':symlink.bzl', 'symlink')",
-        "symlink(",
-        "  name = 'foo-link',",
-        "  target_path = '/some/path',",
-        ")");
+        """
+        load(':symlink.bzl', 'symlink')
+        symlink(
+          name = 'foo-link',
+          target_path = '%s',
+          target_type = '%s',
+        )
+        """
+            .formatted(targetPathArg, targetTypeArg));
 
     buildTarget("//:foo-link");
 
-    assertSymlink("foo-link", PathFragment.create("/some/path"));
+    assertSymlink("foo-link", targetPath.asFragment());
 
     // Delete link, re-plant symlink
     getOutputPath("foo-link").delete();
     buildTarget("//:foo-link");
 
-    assertSymlink("foo-link", PathFragment.create("/some/path"));
+    assertSymlink("foo-link", targetPath.asFragment());
+
+    // Assert that the symlink works after planting the target.
+    if (targetType == SymlinkTargetType.FILE) {
+      FileSystemUtils.writeContent(targetPath, UTF_8, "hello world");
+      assertThat(FileSystemUtils.readContent(getOutputPath("foo-link"), UTF_8))
+          .isEqualTo("hello world");
+    } else if (targetType == SymlinkTargetType.DIRECTORY) {
+      targetPath.createDirectory();
+      FileSystemUtils.writeContent(targetPath.getChild("file.txt"), UTF_8, "hello world");
+      assertThat(FileSystemUtils.readContent(getOutputPath("foo-link/file.txt"), UTF_8))
+          .isEqualTo("hello world");
+    }
   }
 
   @Test
@@ -2043,7 +2070,11 @@ def _symlink_impl(ctx):
                 ctx.actions.symlink(output = link, target_file = ctx.file.target_artifact)
             elif ctx.attr.target_path and not ctx.file.target_artifact:
                 link = ctx.actions.declare_symlink(ctx.attr.name)
-                ctx.actions.symlink(output = link, target_path = ctx.attr.target_path)
+                ctx.actions.symlink(
+                    output = link,
+                    target_path = ctx.attr.target_path,
+                    target_type = ctx.attr.target_type or None,
+                )
             else:
                 fail("exactly one of target_artifact or target_path must be set")
 
@@ -2054,6 +2085,7 @@ def _symlink_impl(ctx):
             attrs = {
                 "target_artifact": attr.label(allow_single_file = True),
                 "target_path": attr.string(),
+                "target_type": attr.string(),
             },
         )
         """);