Hardcode --incompatible_disallow_unsound_directory_outputs.

PiperOrigin-RevId: 597191505
Change-Id: I03a1879c35afd82f5b25d2760b7ac66d051a82b8

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

@@ -220,17 +220,6 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
               + "as errors.")
   public boolean strictFilesets;
 
-  @Option(
-      name = "incompatible_disallow_unsound_directory_outputs",
-      defaultValue = "true",
-      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(

src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java

@@ -43,6 +43,15 @@ public final class BazelRulesModule extends BlazeModule {
    */
   @SuppressWarnings("deprecation") // These fields have no JavaDoc by design
   public static class BuildGraveyardOptions extends OptionsBase {
+    @Option(
+        name = "incompatible_disallow_unsound_directory_outputs",
+        defaultValue = "true",
+        documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+        metadataTags = OptionMetadataTag.INCOMPATIBLE_CHANGE,
+        effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
+        help = "Deprecated. No-op.")
+    public boolean disallowUnsoundDirectoryOutputs;
+
     @Option(
         name = "experimental_use_scheduling_middlemen",
         defaultValue = "false",

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

@@ -1655,24 +1655,19 @@ private void checkForUnsoundDirectoryInputs(Action action, InputMetadataProvider
 
   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.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;
-      }
+    if (output.isDirectory() || output.isSymlink() || !metadata.getType().isDirectory()) {
+      return true;
     }
-    return success;
+    String ownerString = action.getOwner().getLabel().toString();
+    reporter.handle(
+        Event.of(
+                EventKind.ERROR,
+                action.getOwner().getLocation(),
+                String.format(
+                    "output '%s' of %s is a directory but was not declared as such",
+                    output.prettyPrint(), ownerString))
+            .withTag(ownerString));
+    return false;
   }
 
   /**

src/main/starlark/builtins_bzl/common/builtin_exec_platforms.bzl

@@ -238,7 +238,6 @@ bazel_fragments["CoreOptions"] = fragment(
         "//command_line_option:incompatible_check_testonly_for_output_files",
         "//command_line_option:incompatible_auto_exec_groups",
         "//command_line_option:experimental_writable_outputs",
-        "//command_line_option:incompatible_disallow_unsound_directory_outputs",
         "//command_line_option:build_runfile_manifests",
         "//command_line_option:build_runfile_links",
         "//command_line_option:legacy_external_runfiles",

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

@@ -36,28 +36,14 @@ private void setupGenruleWithOutputArtifactDirectory() throws Exception {
         "        srcs = [])");
   }
 
-  @Test
-  public void testOutputArtifactDirectoryWarning_forGenrule() throws Exception {
-    setupGenruleWithOutputArtifactDirectory();
-
-    addOptions("--noincompatible_disallow_unsound_directory_outputs");
-    buildTarget("//x");
-
-    events.assertContainsWarning(
-        "output 'x/dir' of //x:x is a directory; "
-            + "dependency checking of directories is unsound");
-  }
-
   @Test
   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");
+        "output 'x/dir' of //x:x is a directory but was not declared as such");
   }
 
   private void setupStarlarkRuleWithOutputArtifactDirectory() throws Exception {
@@ -78,28 +64,14 @@ private void setupStarlarkRuleWithOutputArtifactDirectory() throws Exception {
     write("x/BUILD", "load('defs.bzl', 'my_rule')", "my_rule(name = 'x', out = 'dir')");
   }
 
-  @Test
-  public void testOutputArtifactDirectoryWarning_forStarlarkRule() throws Exception {
-    setupStarlarkRuleWithOutputArtifactDirectory();
-
-    addOptions("--noincompatible_disallow_unsound_directory_outputs");
-    buildTarget("//x");
-
-    events.assertContainsWarning(
-        "output 'x/dir' of //x:x is a directory; "
-            + "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");
+        "output 'x/dir' of //x:x is a directory but was not declared as such");
   }
 
   @Test

src/test/shell/bazel/bazel_hermetic_sandboxing_test.sh

@@ -261,27 +261,21 @@ def _other_artifacts_impl(ctx):
 
     # Produce artifacts of other types
 
-    regular_file_artifact = ctx.actions.declare_file(ctx.attr.name + ".regular_file_artifact")
-    directory_artifact = ctx.actions.declare_file(ctx.attr.name + ".directory_artifact")
-    tree_artifact = ctx.actions.declare_directory(ctx.attr.name + ".tree_artifact")
-    unresolved_symlink_artifact = ctx.actions.declare_symlink(ctx.attr.name + ".unresolved_symlink_artifact")
+    file_artifact = ctx.actions.declare_file(ctx.attr.name + ".file_artifact")
+    directory_artifact = ctx.actions.declare_directory(ctx.attr.name + ".directory_artifact")
+    symlink_artifact = ctx.actions.declare_symlink(ctx.attr.name + ".symlink_artifact")
 
     ctx.actions.run_shell(
-        command = "touch %s && mkdir %s" % (regular_file_artifact.path, directory_artifact.path),
-        outputs = [regular_file_artifact, tree_artifact, directory_artifact],
-    )
-
-    ctx.actions.symlink(
-        output = unresolved_symlink_artifact,
-        target_path="dangling"
+        command = "touch %s && mkdir -p %s && ln -s dangling %s" % (
+            file_artifact.path, directory_artifact.path, symlink_artifact.path),
+        outputs = [file_artifact, directory_artifact, symlink_artifact],
     )
 
     # Test other artifact types as input to hermetic sandbox.
 
-    all_artifacts = [regular_file_artifact,
+    all_artifacts = [file_artifact,
                      directory_artifact,
-                     tree_artifact,
-                     unresolved_symlink_artifact]
+                     symlink_artifact]
     input_paths_string = " ".join([a.path for a in all_artifacts])
     result_file = ctx.actions.declare_file(ctx.attr.name + ".result")
     ctx.actions.run_shell(
@@ -369,12 +363,10 @@ function test_subdirectories_in_declared_directory() {
 # Test that the sandbox is able to handle various types of artifacts.
 # Regression test for Issue #15340
 function test_other_artifacts() {
-  bazel build --noincompatible_disallow_unsound_directory_outputs \
-    examples/hermetic:other_artifacts &> $TEST_log
-  assert_contains ".regular_file_artifact" "bazel-bin/examples/hermetic/other_artifacts.result"
-  assert_contains ".unresolved_symlink_artifact" "bazel-bin/examples/hermetic/other_artifacts.result"
+  bazel build examples/hermetic:other_artifacts &> $TEST_log
+  assert_contains ".file_artifact" "bazel-bin/examples/hermetic/other_artifacts.result"
+  assert_contains ".symlink_artifact" "bazel-bin/examples/hermetic/other_artifacts.result"
   assert_contains ".directory_artifact" "bazel-bin/examples/hermetic/other_artifacts.result"
-  assert_contains ".tree_artifact" "bazel-bin/examples/hermetic/other_artifacts.result"
 }
 
 # The test shouldn't fail if the environment doesn't support running it.

src/test/shell/bazel/remote/remote_execution_test.sh

@@ -897,96 +897,6 @@ EOF
   expect_log "Failed to query remote execution capabilities"
 }
 
-function set_symlinks_in_directory_testfixtures() {
-    cat > BUILD <<'EOF'
-genrule(
-    name = 'make-links',
-    outs = ['dir', 'r', 'a', 'rd', 'ad'],
-    cmd = ('mkdir $(location dir) && ' +
-        'cd $(location dir) && ' +
-        'echo hello > foo && ' + # Regular file.
-        'ln -s foo r && ' +  # Relative symlink, will be passed as symlink.
-        'ln -s $$PWD/foo a && ' +  # Absolute symlink, will be copied.
-        'mkdir bar && ' + # Regular directory.
-        'echo bla > bar/baz && ' +
-        'ln -s bar rd && ' +  # Relative symlink, will be passed as symlink.
-        'ln -s $$PWD/bar ad && ' + # Absolute symlink, will be copied.
-        'cd .. && ' +
-        'ln -s dir/foo r && ' +  # Relative symlink, will be passed as symlink.
-        'ln -s $$PWD/dir/foo a && ' +  # Absolute symlink, will be copied.
-        'ln -s dir rd && ' +  # Relative symlink, will be passed as symlink.
-        'ln -s $$PWD/dir ad' # Absolute symlink, will be copied.
-    ),
-)
-EOF
-    cat > "${TEST_TMPDIR}/expected_links" <<'EOF'
-./ad/r
-./ad/rd
-./dir/r
-./dir/rd
-./r
-./rd
-EOF
-}
-
-function test_symlinks_in_directory() {
-    set_symlinks_in_directory_testfixtures
-    # Need --remote_download_all because the genrule generates directory output
-    # for one of the declared outputs which is not supported when BwoB.
-    bazel build \
-          --incompatible_remote_symlinks \
-          --noincompatible_disallow_unsound_directory_outputs \
-          --remote_executor=grpc://localhost:${worker_port} \
-          --remote_download_all \
-          --spawn_strategy=remote \
-          //:make-links &> $TEST_log \
-          || fail "Failed to build //:make-links with remote execution"
-    expect_log "1 remote"
-    find -L bazel-genfiles -type f -exec cat {} \; | sort | uniq -c &> $TEST_log
-    expect_log "9 bla"
-    expect_log "11 hello"
-    CUR=$PWD && cd bazel-genfiles && \
-      find . -type l | sort > "${TEST_TMPDIR}/links" && cd $CUR
-    diff "${TEST_TMPDIR}/links" "${TEST_TMPDIR}/expected_links" \
-      || fail "Remote execution created different symbolic links"
-}
-
-function test_symlinks_in_directory_cache_only() {
-    # This test is the same as test_symlinks_in_directory, except it works
-    # locally and uses the remote cache to query results.
-    #
-    # Need --remote_download_all because the genrule generates directory output
-    # for one of the declared outputs which is not supported when BwoB.
-    set_symlinks_in_directory_testfixtures
-    bazel build \
-          --incompatible_remote_symlinks \
-          --noincompatible_disallow_unsound_directory_outputs \
-          --remote_cache=grpc://localhost:${worker_port} \
-          --remote_download_all \
-          --spawn_strategy=local \
-          //:make-links &> $TEST_log \
-          || fail "Failed to build //:make-links with remote cache service"
-    expect_log "1 local"
-    bazel clean # Get rid of local results, rely on remote cache.
-    bazel build \
-          --incompatible_remote_symlinks \
-          --noincompatible_disallow_unsound_directory_outputs \
-          --remote_cache=grpc://localhost:${worker_port} \
-          --remote_download_all \
-          --spawn_strategy=local \
-          //:make-links &> $TEST_log \
-          || fail "Failed to build //:make-links with remote cache service"
-    expect_log "1 remote cache hit"
-    # Check that the results downloaded from remote cache are the same as local.
-    find -L bazel-genfiles -type f -exec cat {} \; | sort | uniq -c &> $TEST_log
-    expect_log "9 bla"
-    expect_log "11 hello"
-    CUR=$PWD && cd bazel-genfiles && \
-      find . -type l | sort > "${TEST_TMPDIR}/links" && cd $CUR
-    diff "${TEST_TMPDIR}/links" "${TEST_TMPDIR}/expected_links" \
-      || fail "Cached result created different symbolic links"
-}
-
 function test_treeartifact_in_runfiles() {
      mkdir -p a
     cat > a/BUILD <<'EOF'