Declare the output of a SymlinkAction#toArtifact as a directory, not …

…a file, when the input is a directory.

Note that SymlinkAction#toArtifact doesn't currently work when the output is a directory, as it will attempt to create a symlink over it. However, some callers cheat by passing in a non-directory output, which works fine as far as SymlinkAction is concerned, but causes subtle issues elsewhere. In the future, SymlinkAction#toArtifact should itself enforce this precondition, but `ctx.actions.symlink` must be fixed first (in a separate CL).

This is a necessary step to fix issue #15678.

PiperOrigin-RevId: 463049372
Change-Id: I2d072a17a18cf26bc64b5639078fc2954fc3801b

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

@@ -199,8 +199,17 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext)
     } else {
       srcPath = actionExecutionContext.getExecRoot().getRelative(inputPath);
     }
+
+    Path outputPath = getOutputPath(actionExecutionContext);
+
     try {
-      getOutputPath(actionExecutionContext).createSymbolicLink(srcPath);
+      // Delete the empty output directory created prior to the action execution when the output is
+      // a tree artifact. All other actions that produce tree artifacts expect it to exist prior to
+      // their execution. It's not worth complicating ActionOutputDirectoryHelper just to avoid this
+      // small amount of overhead.
+      outputPath.delete();
+
+      outputPath.createSymbolicLink(srcPath);
     } catch (IOException e) {
       String message =
           String.format(

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

@@ -16,6 +16,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
@@ -163,12 +164,16 @@ public static Artifact getLayoutInfoFile(ActionConstructionContext actionConstru
   }
 
   /** Returns an artifact for the specified output under a standardized data binding base dir. */
-  static Artifact getDataBindingArtifact(RuleContext ruleContext, String relativePath) {
-    PathFragment binRelativeBasePath =
+  static Artifact getDataBindingArtifact(
+      RuleContext ruleContext, String relativePath, boolean isDirectory) {
+    ArtifactRoot root = ruleContext.getBinOrGenfilesDirectory();
+    PathFragment rootRelativePath =
         getDataBindingExecPath(ruleContext)
-            .relativeTo(ruleContext.getBinOrGenfilesDirectory().getExecPath());
-    return ruleContext.getDerivedArtifact(
-        binRelativeBasePath.getRelative(relativePath), ruleContext.getBinOrGenfilesDirectory());
+            .relativeTo(ruleContext.getBinOrGenfilesDirectory().getExecPath())
+            .getRelative(relativePath);
+    return isDirectory
+        ? ruleContext.getTreeArtifact(rootRelativePath, root)
+        : ruleContext.getDerivedArtifact(rootRelativePath, root);
   }
 
   static ImmutableList<Artifact> getAnnotationFile(RuleContext ruleContext, boolean useAndroidX) {
@@ -187,7 +192,8 @@ static ImmutableList<Artifact> getAnnotationFile(RuleContext ruleContext, boolea
               useAndroidX
                   ? "databinding_annotation_template_androidx.txt"
                   : "databinding_annotation_template_support_lib.txt");
-      Artifact annotationFile = getDataBindingArtifact(ruleContext, "DataBindingInfo.java");
+      Artifact annotationFile =
+          getDataBindingArtifact(ruleContext, "DataBindingInfo.java", /* isDirectory= */ false);
       ruleContext.registerAction(
           FileWriteAction.create(ruleContext, annotationFile, contents, false));
       return ImmutableList.of(annotationFile);
@@ -236,13 +242,15 @@ static ImmutableList<Artifact> getMetadataOutputs(
       if (useUpdatedArgs) {
         outputs.add(
             getDataBindingArtifact(
-                ruleContext, String.format("%s/%s-%s", METADATA_OUTPUT_DIR, javaPackage, suffix)));
+                ruleContext,
+                String.format("%s/%s-%s", METADATA_OUTPUT_DIR, javaPackage, suffix),
+                /* isDirectory= */ false));
       } else {
         outputs.add(
             getDataBindingArtifact(
                 ruleContext,
-                String.format(
-                    "%s/%s-%s-%s", METADATA_OUTPUT_DIR, javaPackage, javaPackage, suffix)));
+                String.format("%s/%s-%s-%s", METADATA_OUTPUT_DIR, javaPackage, javaPackage, suffix),
+                /* isDirectory= */ false));
       }
     }
     return outputs.build();
@@ -264,12 +272,14 @@ static Artifact getMetadataOutput(
     if (useUpdatedArgs) {
       return getDataBindingArtifact(
           ruleContext,
-          String.format("%s/%s-%s", METADATA_OUTPUT_DIR, javaPackage, metadataOutputSuffix));
+          String.format("%s/%s-%s", METADATA_OUTPUT_DIR, javaPackage, metadataOutputSuffix),
+          /* isDirectory= */ false);
     } else {
       return getDataBindingArtifact(
           ruleContext,
           String.format(
-              "%s/%s-%s-%s", METADATA_OUTPUT_DIR, javaPackage, javaPackage, metadataOutputSuffix));
+              "%s/%s-%s-%s", METADATA_OUTPUT_DIR, javaPackage, javaPackage, metadataOutputSuffix),
+          /* isDirectory= */ false);
     }
   }
 
@@ -287,8 +297,8 @@ static Artifact symlinkDepsMetadataIntoOutputTree(RuleContext ruleContext, Artif
     Artifact symlink =
         getDataBindingArtifact(
             ruleContext,
-            String.format(
-                "%s/%s", DEP_METADATA_INPUT_DIR, depMetadata.getRootRelativePathString()));
+            String.format("%s/%s", DEP_METADATA_INPUT_DIR, depMetadata.getRootRelativePathString()),
+            /* isDirectory= */ depMetadata.isTreeArtifact());
     ruleContext.registerAction(
         SymlinkAction.toArtifact(
             ruleContext.getActionOwner(),

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

@@ -296,7 +296,9 @@ private ImmutableList<Artifact> createBaseClasses(RuleContext ruleContext) {
 
     Artifact layoutInfo = getLayoutInfoFile();
     Artifact classInfoFile = getClassInfoFile(ruleContext);
-    Artifact srcOutFile = DataBinding.getDataBindingArtifact(ruleContext, "baseClassSrc.srcjar");
+    Artifact srcOutFile =
+        DataBinding.getDataBindingArtifact(
+            ruleContext, "baseClassSrc.srcjar", /* isDirectory= */ false);
 
     FilesToRunProvider exec =
         ruleContext.getExecutablePrerequisite(DataBinding.DATABINDING_EXEC_PROCESSOR_ATTR);

src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java

@@ -326,7 +326,7 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa
 
     // Make sure the tree artifact root is a regular directory. Note that this is how the action is
     // initialized, so this should hold unless the action itself has deleted the root.
-    if (!treeDir.isDirectory(Symlinks.NOFOLLOW)) {
+    if (!treeDir.isDirectory(Symlinks.FOLLOW)) {
       if (chmod) {
         setPathReadOnlyAndExecutableIfFile(treeDir);
       }

src/test/shell/bazel/bazel_symlink_test.sh

@@ -476,9 +476,9 @@ load(":a.bzl", "a")
 a(name="a")
 EOF
 
-  # TODO(tjgq): This should build successfully.
-  bazel build //a:a >& $TEST_log && fail "build succeeded"
-  expect_log "failed to create symbolic link"
+  bazel build //a:a || fail "build failed"
+  assert_contains "Hello, World!" bazel-bin/a/a.link/inside.txt
+  expect_symlink bazel-bin/a/a.link
 }
 
 function test_symlink_file_to_directory_created_from_symlink_action() {