Allow artifact -> artifact symlink actions from Starlark

Updates #7514

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

@@ -16,6 +16,7 @@
 import static java.util.stream.Collectors.toList;
 
 import com.google.common.base.Joiner;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Action;
@@ -220,29 +221,89 @@ public void doNothing(String mnemonic, Object inputs) throws EvalException {
   static final GeneratedMessage.GeneratedExtension<ExtraActionInfo, SpawnInfo> SPAWN_INFO =
       SpawnInfo.spawnInfo;
 
+  private String progressMessageForSymlink(
+      Object /* String or None */ progressMessage, Artifact output) {
+    if (progressMessage != Starlark.NONE) {
+      // Should have been verified by Starlark before this function is called.
+      return (String)progressMessage;
+    }
+
+    return "Creating symlink " + output.getRootRelativePathString();
+  }
+
   @Override
-  public void symlink(FileApi output, String path) throws EvalException {
+  public void symlink(
+      FileApi output,
+      Object /* Artifact or None */ targetFile,
+      Object /* String or None */ targetPath,
+      Boolean isExecutable,
+      Object /* String or None */ progressMessage)
+      throws EvalException {
     context.checkMutable("actions.symlink");
 
+    if ((targetFile != Starlark.NONE && targetPath != Starlark.NONE) ||
+        (targetFile == Starlark.NONE && targetPath == Starlark.NONE)) {
+      throw Starlark.errorf("\"target_file\" and \"target_path\" cannot be set at the same time");
+    }
+
+    // Should have been verified by Starlark before this function is called.
+    Artifact outputArtifact = (Artifact)output;
+
+    if (targetFile != Starlark.NONE) {
+      Preconditions.checkState(targetPath == Starlark.NONE);
+
+      if (outputArtifact.isSymlink()) {
+        // TODO(yannic): Do we allow symlinks from files created by declare_symlink() to artifacts?
+        throw Starlark.errorf(
+            "output of symlink action with \"target_file\" must be created by declare_file()");
+      }
+
+      // Should have been verified by Starlark before this function is called.
+      Artifact target = (Artifact)targetFile;
+      if (isExecutable) {
+        SymlinkAction action =
+            SymlinkAction.toExecutable(
+                ruleContext.getActionOwner(),
+                target,
+                outputArtifact,
+                progressMessageForSymlink(progressMessage, outputArtifact));
+        registerAction(action);
+        return;
+      }
+      SymlinkAction action =
+          SymlinkAction.toArtifact(
+              ruleContext.getActionOwner(),
+              target,
+              outputArtifact,
+              progressMessageForSymlink(progressMessage, outputArtifact));
+      registerAction(action);
+      return;
+    }
+    Preconditions.checkState(targetPath != Starlark.NONE);
+
     if (!ruleContext.getConfiguration().allowUnresolvedSymlinks()) {
-      throw new EvalException(
-          null,
-          "actions.symlink() is not allowed; "
+      throw Starlark.errorf(
+          "actions.symlink() to unresolved symlink is not allowed; "
               + "use the --experimental_allow_unresolved_symlinks command line option");
     }
 
-    PathFragment targetPath = PathFragment.create(path);
-    Artifact outputArtifact = (Artifact) output;
     if (!outputArtifact.isSymlink()) {
       throw Starlark.errorf("output of symlink action must be created by declare_symlink()");
     }
 
-    Action action =
+    if (isExecutable) {
+      // TODO(yannic): Do we allow dangling symlinks to be executable?
+      throw Starlark.errorf("files created by declare_symlink() cannot be executable");
+    }
+
+    // Should have been verified by Starlark before this function is called.
+    String target = (String)targetPath;
+    SymlinkAction action =
         SymlinkAction.createUnresolved(
             ruleContext.getActionOwner(),
             outputArtifact,
-            targetPath,
-            "creating symlink " + ((Artifact) output).getRootRelativePathString());
+            PathFragment.create(target),
+            progressMessageForSymlink(progressMessage, outputArtifact));
     registerAction(action);
   }
 

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkActionFactoryApi.java

@@ -159,17 +159,70 @@ public interface SkylarkActionFactoryApi extends StarlarkValue {
   @SkylarkCallable(
       name = "symlink",
       doc =
-          "<p><b>Experimental</b>. This parameter is experimental and may change at any "
-              + "time. Please do not depend on it. It may be enabled on an experimental basis by "
-              + "setting <code>--experimental_allow_unresolved_symlinks</code></p><p>"
-              + "Creates a symlink in the file system. If the output file is a regular file, the "
-              + "symlink must point to a file. If the output is an unresolved symlink, a dangling "
-              + "symlink is allowed.",
+          "Creates a symlink in the file system."
+              + "<p>If the output file is a regular file (i.e. created by "
+              + "<a href=\"#declare_file\"><code>declare_file()</code></a>), the symlink must "
+              + "point to a file. If the output is an unresolved symlink (i.e. created by "
+              + "<a href=\"#declare_symlink\"><code>declare_symlink()</code></a>), a dangling "
+              + "symlink is allowed.</p>",
       parameters = {
-        @Param(name = "output", type = FileApi.class, doc = "The output path.", named = true),
-        @Param(name = "target", type = String.class, doc = "The target.", named = true),
+        @Param(
+            name = "output",
+            type = FileApi.class,
+            named = true,
+            positional = false,
+            doc = "The output of this action."),
+        @Param(
+            name = "target_file",
+            type = FileApi.class,
+            named = true,
+            positional = false,
+            noneable = true,
+            defaultValue = "None",
+            doc =
+                "The target of the symlink."
+                    + "<p>If this parameter is set, <code>target_path</code> "
+                    + "must be <code>None</code>.</p>"),
+        @Param(
+            name = "target_path",
+            type = String.class,
+            named = true,
+            positional = false,
+            noneable = true,
+            defaultValue = "None",
+            doc =
+                "The target path of the symlink."
+                    + "<p>If this parameter is set, <code>target_file</code> "
+                    + "must be <code>None</code>.</p>"
+                    + "<p><b>Experimental</b> This parameter is experimental and may change at "
+                    + "any time. Please do not depend on it. It may be enabled on an experimental "
+                    + "basis by setting <code>--experimental_allow_unresolved_symlinks</code></p>"),
+        @Param(
+            name = "is_executable",
+            type = Boolean.class,
+            named = true,
+            positional = false,
+            defaultValue = "False",
+            doc =
+                "Whether the output file should be executable. "
+                    + "<p>If this is <code>True<code>, <code>target_path</code> must also be "
+                    + "executable.</p>"),
+        @Param(
+            name = "progress_message",
+            type = String.class,
+            named = true,
+            positional = false,
+            noneable = true,
+            defaultValue = "None",
+            doc = "Progress message to show to the user during the build.")
       })
-  void symlink(FileApi output, String targetPath) throws EvalException;
+  void symlink(
+      FileApi output,
+      Object targetFile,
+      Object targetPath,
+      Boolean isExecutable,
+      Object progressMessage)
+      throws EvalException;
 
   @SkylarkCallable(
       name = "write",

src/test/shell/bazel/BUILD

@@ -70,7 +70,6 @@ sh_test(
         ":test-deps",
         "@bazel_tools//tools/bash/runfiles",
     ],
-    tags = ["no_windows"],
 )
 
 sh_test(