Warn on unsound directory inputs/outputs to any action, not just genr…

…ule actions.

The motivation behind this change is to eventually promote the warning to an error in the output case; this avoids surprising behavior such as described in #18579. I'm less confident that the input case (source directories) can be similarly promoted to an error, but I'm still extending the warning to all actions for symmetry.

PiperOrigin-RevId: 538470433
Change-Id: Ic6c8a4cb353dda5f6397d286fe2e6faee28f896c

src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java

@@ -24,25 +24,19 @@
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Iterators;
 import com.google.common.collect.Maps;
-import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
-import com.google.devtools.build.lib.actions.cache.OutputMetadataStore;
 import com.google.devtools.build.lib.actions.extra.ExtraActionInfo;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
-import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.collect.nestedset.Depset;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.AspectDescriptor;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
-import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
 import com.google.devtools.build.lib.starlarkbuildapi.ActionApi;
 import com.google.devtools.build.lib.starlarkbuildapi.CommandLineArgsApi;
 import com.google.devtools.build.lib.vfs.BulkDeleter;
@@ -73,7 +67,6 @@
 @Immutable
 @ThreadSafe
 public abstract class AbstractAction extends ActionKeyCacher implements Action, ActionApi {
-  private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
   /**
    * An arbitrary default resource set. We assume that a typical subprocess is single-threaded
@@ -538,77 +531,12 @@ public static void deleteOutput(Path path, @Nullable Root root) throws IOExcepti
       }
     }
   }
-
-  /**
-   * If the action might read directories as inputs in a way that is unsound wrt dependency
-   * checking, this method must be called.
-   */
-  protected void checkInputsForDirectories(
-      EventHandler eventHandler, InputMetadataProvider inputMetadataProvider) throws ExecException {
-    // Report "directory dependency checking" warning only for non-generated directories (generated
-    // ones will be reported earlier).
-    for (Artifact input : getMandatoryInputs().toList()) {
-      // Assume that if the file did not exist, we would not have gotten here.
-      try {
-        if (input.isSourceArtifact()
-            && inputMetadataProvider.getInputMetadata(input).getType().isDirectory()) {
-          // TODO(ulfjack): What about dependency checking of special files?
-          eventHandler.handle(
-              Event.warn(
-                  owner.getLocation(),
-                  String.format(
-                      "input '%s' to %s is a directory; "
-                          + "dependency checking of directories is unsound",
-                      input.prettyPrint(), owner.getLabel())));
-        }
-      } catch (IOException e) {
-        throw new EnvironmentalExecException(e, Code.INPUT_DIRECTORY_CHECK_IO_EXCEPTION);
-      }
-    }
-  }
-
+
   @Override
   public MiddlemanType getActionType() {
     return MiddlemanType.NORMAL;
   }
 
-  /** If the action might create directories as outputs this method must be called. */
-  protected void checkOutputsForDirectories(ActionExecutionContext actionExecutionContext)
-      throws InterruptedException {
-    FileArtifactValue metadata;
-    for (Artifact output : getOutputs()) {
-      OutputMetadataStore outputMetadataStore = actionExecutionContext.getOutputMetadataStore();
-      if (outputMetadataStore.artifactOmitted(output)) {
-        continue;
-      }
-      try {
-        metadata = outputMetadataStore.getOutputMetadata(output);
-      } catch (IOException e) {
-        logger.atWarning().withCause(e).log("Error getting metadata for %s", output);
-        metadata = null;
-      }
-      if (metadata != null) {
-        if (!metadata.getType().isDirectory()) {
-          continue;
-        }
-      } else if (!actionExecutionContext.getInputPath(output).isDirectory()) {
-        continue;
-      }
-      String ownerString = Label.print(owner.getLabel());
-      actionExecutionContext
-          .getEventHandler()
-          .handle(
-              Event.warn(
-                      owner.getLocation(),
-                      "output '"
-                          + output.prettyPrint()
-                          + "' of "
-                          + ownerString
-                          + " is a directory; dependency checking of directories is unsound")
-                  .withTag(ownerString));
-    }
-  }
-
   @Override
   public void prepare(
       Path execRoot,

src/main/java/com/google/devtools/build/lib/rules/genrule/BUILD

@@ -31,7 +31,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/packages",
-        "//src/main/java/com/google/devtools/build/lib/skyframe:track_source_directories_flag",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/util:filetype",
         "//src/main/java/com/google/devtools/build/lib/util:os",

src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleAction.java

@@ -18,18 +18,13 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.AbstractAction;
 import com.google.devtools.build.lib.actions.ActionEnvironment;
-import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.CommandLineLimits;
 import com.google.devtools.build.lib.actions.CommandLines;
-import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.RunfilesSupplier;
-import com.google.devtools.build.lib.actions.SpawnResult;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
-import com.google.devtools.build.lib.skyframe.TrackSourceDirectoriesFlag;
-import java.util.List;
 
 /**
  * A spawn action for genrules. Genrules are handled specially in that inputs and outputs are
@@ -68,20 +63,4 @@ public GenRuleAction(
   protected CommandLineLimits getCommandLineLimits() {
     return CommandLineLimits.UNLIMITED;
   }
-
-  @Override
-  protected void beforeExecute(ActionExecutionContext actionExecutionContext) throws ExecException {
-    if (!TrackSourceDirectoriesFlag.trackSourceDirectories()) {
-      checkInputsForDirectories(
-          actionExecutionContext.getEventHandler(),
-          actionExecutionContext.getInputMetadataProvider());
-    }
-  }
-
-  @Override
-  protected void afterExecute(
-      ActionExecutionContext actionExecutionContext, List<SpawnResult> spawnResults)
-      throws InterruptedException {
-    checkOutputsForDirectories(actionExecutionContext);
-  }
 }

src/main/java/com/google/devtools/build/lib/skyframe/BUILD

@@ -195,6 +195,7 @@ java_library(
         ":top_level_action_lookup_key_wrapper",
         ":top_level_aspects_value",
         ":top_level_status_events",
+        ":track_source_directories_flag",
         ":transitive_base_traversal_function",
         ":transitive_target_key",
         ":transitive_target_value",

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

@@ -1118,6 +1118,8 @@ private ActionStepOrResult executeAction(ExtendedEventHandler eventHandler, Acti
         throws LostInputsActionExecutionException, InterruptedException {
       ActionResult result;
       try (SilentCloseable c = profiler.profile(ProfilerTask.INFO, "Action.execute")) {
+        checkForUnsoundDirectoryInputs(action, actionExecutionContext.getInputMetadataProvider());
+
         result = action.execute(actionExecutionContext);
 
         // An action's result (or intermediate state) may have been affected by lost inputs. If an
@@ -1520,6 +1522,8 @@ private boolean checkOutputs(
           try {
             FileArtifactValue metadata = outputMetadataStore.getOutputMetadata(output);
 
+            checkForUnsoundDirectoryOutput(action, output, metadata);
+
             addOutputToMetrics(
                 output,
                 metadata,
@@ -1591,6 +1595,60 @@ private void addOutputToMetrics(
     }
   }
 
+  private void checkForUnsoundDirectoryInputs(Action action, InputMetadataProvider metadataProvider)
+      throws ActionExecutionException {
+    if (TrackSourceDirectoriesFlag.trackSourceDirectories()) {
+      return;
+    }
+
+    if (action.getMnemonic().equals("FilesetTraversal")) {
+      // Omit warning for filesets (b/1437948).
+      return;
+    }
+
+    // Report "directory dependency checking" warning only for non-generated directories (generated
+    // ones will have been reported earlier, in the checkForUnsoundDirectoryOutput call for the
+    // respective producing action).
+    for (Artifact input : action.getMandatoryInputs().toList()) {
+      // Assume that if the file did not exist, we would not have gotten here.
+      try {
+        if (input.isSourceArtifact()
+            && metadataProvider.getInputMetadata(input).getType().isDirectory()) {
+          // TODO(ulfjack): What about dependency checking of special files?
+          String ownerString = action.getOwner().getLabel().toString();
+          reporter.handle(
+              Event.warn(
+                      action.getOwner().getLocation(),
+                      String.format(
+                          "input '%s' to %s is a directory; "
+                              + "dependency checking of directories is unsound",
+                          input.prettyPrint(), ownerString))
+                  .withTag(ownerString));
+        }
+      } catch (IOException e) {
+        throw ActionExecutionException.fromExecException(
+            new EnvironmentalExecException(
+                e, FailureDetails.Execution.Code.INPUT_DIRECTORY_CHECK_IO_EXCEPTION),
+            action);
+      }
+    }
+  }
+
+  private void checkForUnsoundDirectoryOutput(
+      Action action, Artifact output, FileArtifactValue metadata) {
+    if (!output.isDirectory() && !output.isSymlink() && metadata.getType().isDirectory()) {
+      String ownerString = action.getOwner().getLabel().toString();
+      reporter.handle(
+          Event.warn(
+                  action.getOwner().getLocation(),
+                  String.format(
+                      "output '%s' of %s is a directory; "
+                          + "dependency checking of directories is unsound",
+                      output.prettyPrint(), ownerString))
+              .withTag(ownerString));
+    }
+  }
+
   /**
    * Convenience function for creating an ActionExecutionException reporting that the action failed
    * due to the exception cause, if there is an additional explanatory message that clarifies the

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

@@ -25,7 +25,7 @@
 public class DirectoryArtifactWarningTest extends BuildIntegrationTestCase {
 
   @Test
-  public void testOutputArtifactDirectoryWarning() throws Exception {
+  public void testOutputArtifactDirectoryWarning_forGenrule() throws Exception {
     write(
         "x/BUILD",
         "genrule(name = 'x',",
@@ -40,7 +40,32 @@ public void testOutputArtifactDirectoryWarning() throws Exception {
   }
 
   @Test
-  public void testInputArtifactDirectoryWarning() throws Exception {
+  public void testOutputArtifactDirectoryWarning_forStarlarkRule() throws Exception {
+    write(
+        "x/defs.bzl",
+        "def _impl(ctx):",
+        "  ctx.actions.run_shell(",
+        "    outputs = [ctx.outputs.out],",
+        "    command = 'mkdir %s' % ctx.outputs.out.path,",
+        "  )",
+        "",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'out': attr.output(),",
+        "  },",
+        ")");
+    write("x/BUILD", "load('defs.bzl', 'my_rule')", "my_rule(name = 'x', out = 'dir')");
+
+    buildTarget("//x");
+
+    events.assertContainsWarning(
+        "output 'x/dir' of //x:x is a directory; "
+            + "dependency checking of directories is unsound");
+  }
+
+  @Test
+  public void testInputArtifactDirectoryWarning_forGenrule() throws Exception {
     write(
         "x/BUILD",
         "genrule(name = 'x',",
@@ -55,4 +80,32 @@ public void testInputArtifactDirectoryWarning() throws Exception {
           + "dependency checking of directories is unsound");
   }
 
+  @Test
+  public void testInputArtifactDirectoryWarning_forStarlarkRule() throws Exception {
+    write(
+        "x/defs.bzl",
+        "def _impl(ctx):",
+        "  ctx.actions.run_shell(",
+        "    inputs = [ctx.file.src],",
+        "    outputs = [ctx.outputs.out],",
+        "    command = 'touch %s' % ctx.outputs.out.path,",
+        "  )",
+        "",
+        "my_rule = rule(",
+        "  implementation = _impl,",
+        "  attrs = {",
+        "    'src': attr.label(allow_single_file = True),",
+        "    'out': attr.output(),",
+        "  },",
+        ")");
+    write(
+        "x/BUILD", "load('defs.bzl', 'my_rule')", "my_rule(name = 'x', src = 'dir', out = 'out')");
+    write("x/dir/empty");
+
+    buildTarget("//x");
+
+    events.assertContainsWarning(
+        "input 'x/dir' to //x:x is a directory; "
+            + "dependency checking of directories is unsound");
+  }
 }

src/test/shell/bazel/bazel_hermetic_sandboxing_test.sh

@@ -366,16 +366,14 @@ function test_subdirectories_in_declared_directory() {
   assert_contains "dir/subdir1/subdir2" "bazel-bin/examples/hermetic/subdirectories_in_declared_directory.result"
 }
 
-# Test that the sandbox is not crashing and not producing warnings for various types of artifacts.
+# Test that the sandbox is able to handle various types of artifacts.
 # Regression test for Issue #15340
 function test_other_artifacts() {
-  bazel shutdown # Clear memory about duplicated warnings
   bazel build examples/hermetic:other_artifacts &> $TEST_log
-  expect_not_log "WARNING"
-  assert_contains "regular_file_artifact" "bazel-bin/examples/hermetic/other_artifacts.result"
-  assert_contains "unresolved_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"
+  assert_contains ".regular_file_artifact" "bazel-bin/examples/hermetic/other_artifacts.result"
+  assert_contains ".unresolved_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.