Add a flag to disallow materializing an output file as a directory.

PiperOrigin-RevId: 539591651
Change-Id: I25580ffd0b51d5db1f755c53523211d5cd377eb2

Author: tjgq

src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java

@@ -175,6 +175,17 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
           "Check for action prefix file path conflicts, regardless of action-specific overrides.")
   public boolean strictConflictChecks;
 
+  @Option(
+      name = "incompatible_disallow_unsound_directory_outputs",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+      metadataTags = OptionMetadataTag.INCOMPATIBLE_CHANGE,
+      effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
+      help =
+          "If set, it is an error for an action to materialize an output file as a directory. Does"
+              + " not affect source directories.")
+  public boolean disallowUnsoundDirectoryOutputs;
+
   // This option is only used during execution. However, it is a required input to the analysis
   // phase, as otherwise flipping this flag would not invalidate already-executed actions.
   @Option(
@@ -997,6 +1008,7 @@ public FragmentOptions getExec() {
     exec.useAutoExecGroups = useAutoExecGroups;
     exec.experimentalWritableOutputs = experimentalWritableOutputs;
     exec.strictConflictChecks = strictConflictChecks;
+    exec.disallowUnsoundDirectoryOutputs = disallowUnsoundDirectoryOutputs;
 
     // === Runfiles ===
     exec.buildRunfilesManifests = buildRunfilesManifests;

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

@@ -83,6 +83,7 @@
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.profiler.Profiler;
@@ -1504,7 +1505,7 @@ private static void flushActionFileSystem(
    * Validates that all action outputs were created or intentionally omitted. This can result in
    * chmod calls on the output files; see {@link ActionMetadataHandler}.
    *
-   * @return false if some outputs are missing, true - otherwise.
+   * @return false if some outputs are missing or invalid, true - otherwise.
    */
   private boolean checkOutputs(
       Action action,
@@ -1522,7 +1523,9 @@ private boolean checkOutputs(
           try {
             FileArtifactValue metadata = outputMetadataStore.getOutputMetadata(output);
 
-            checkForUnsoundDirectoryOutput(action, output, metadata);
+            if (!checkForUnsoundDirectoryOutput(action, output, metadata)) {
+              return false;
+            }
 
             addOutputToMetrics(
                 output,
@@ -1634,19 +1637,26 @@ private void checkForUnsoundDirectoryInputs(Action action, InputMetadataProvider
     }
   }
 
-  private void checkForUnsoundDirectoryOutput(
+  private boolean checkForUnsoundDirectoryOutput(
       Action action, Artifact output, FileArtifactValue metadata) {
+    boolean success = true;
     if (!output.isDirectory() && !output.isSymlink() && metadata.getType().isDirectory()) {
+      boolean asError = options.getOptions(CoreOptions.class).disallowUnsoundDirectoryOutputs;
       String ownerString = action.getOwner().getLabel().toString();
       reporter.handle(
-          Event.warn(
+          Event.of(
+                  asError ? EventKind.ERROR : EventKind.WARNING,
                   action.getOwner().getLocation(),
                   String.format(
                       "output '%s' of %s is a directory; "
                           + "dependency checking of directories is unsound",
                       output.prettyPrint(), ownerString))
               .withTag(ownerString));
+      if (asError) {
+        success = false;
+      }
     }
+    return success;
   }
 
   /**

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

@@ -199,6 +199,7 @@ java_test(
         "no_windows",
     ],
     deps = [
+        "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/test/java/com/google/devtools/build/lib/buildtool/util",
         "//third_party:junit4",
     ],

src/test/java/com/google/devtools/build/lib/buildtool/DirectoryArtifactWarningTest.java

@@ -13,6 +13,9 @@
 // limitations under the License.
 package com.google.devtools.build.lib.buildtool;
 
+import static org.junit.Assert.assertThrows;
+
+import com.google.devtools.build.lib.actions.BuildFailedException;
 import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -24,23 +27,39 @@
 @RunWith(JUnit4.class)
 public class DirectoryArtifactWarningTest extends BuildIntegrationTestCase {
 
-  @Test
-  public void testOutputArtifactDirectoryWarning_forGenrule() throws Exception {
+  private void setupGenruleWithOutputArtifactDirectory() throws Exception {
     write(
         "x/BUILD",
         "genrule(name = 'x',",
         "        outs = ['dir'],",
         "        cmd = 'mkdir $(location dir)',",
         "        srcs = [])");
+  }
+
+  @Test
+  public void testOutputArtifactDirectoryWarning_forGenrule() throws Exception {
+    setupGenruleWithOutputArtifactDirectory();
 
     buildTarget("//x");
 
-    events.assertContainsWarning("output 'x/dir' of //x:x is a directory; "
-                        + "dependency checking of directories is unsound");
+    events.assertContainsWarning(
+        "output 'x/dir' of //x:x is a directory; "
+            + "dependency checking of directories is unsound");
   }
 
   @Test
-  public void testOutputArtifactDirectoryWarning_forStarlarkRule() throws Exception {
+  public void testOutputArtifactDirectoryError_forGenrule() throws Exception {
+    setupGenruleWithOutputArtifactDirectory();
+
+    addOptions("--incompatible_disallow_unsound_directory_outputs");
+    assertThrows(BuildFailedException.class, () -> buildTarget("//x"));
+
+    events.assertContainsError(
+        "output 'x/dir' of //x:x is a directory; "
+            + "dependency checking of directories is unsound");
+  }
+
+  private void setupStarlarkRuleWithOutputArtifactDirectory() throws Exception {
     write(
         "x/defs.bzl",
         "def _impl(ctx):",
@@ -56,6 +75,11 @@ public void testOutputArtifactDirectoryWarning_forStarlarkRule() throws Exceptio
         "  },",
         ")");
     write("x/BUILD", "load('defs.bzl', 'my_rule')", "my_rule(name = 'x', out = 'dir')");
+  }
+
+  @Test
+  public void testOutputArtifactDirectoryWarning_forStarlarkRule() throws Exception {
+    setupStarlarkRuleWithOutputArtifactDirectory();
 
     buildTarget("//x");
 
@@ -64,6 +88,18 @@ public void testOutputArtifactDirectoryWarning_forStarlarkRule() throws Exceptio
             + "dependency checking of directories is unsound");
   }
 
+  @Test
+  public void testOutputArtifactDirectoryError_forStarlarkRule() throws Exception {
+    setupStarlarkRuleWithOutputArtifactDirectory();
+
+    addOptions("--incompatible_disallow_unsound_directory_outputs");
+    assertThrows(BuildFailedException.class, () -> buildTarget("//x"));
+
+    events.assertContainsError(
+        "output 'x/dir' of //x:x is a directory; "
+            + "dependency checking of directories is unsound");
+  }
+
   @Test
   public void testInputArtifactDirectoryWarning_forGenrule() throws Exception {
     write(
@@ -76,8 +112,9 @@ public void testInputArtifactDirectoryWarning_forGenrule() throws Exception {
 
     buildTarget("//x");
 
-    events.assertContainsWarning("input 'x/dir' to //x:x is a directory; "
-          + "dependency checking of directories is unsound");
+    events.assertContainsWarning(
+        "input 'x/dir' to //x:x is a directory; "
+            + "dependency checking of directories is unsound");
   }
 
   @Test