Make Bazel not crash if a runfiles tree is in a runfiles tree.

There was a test for this, but it didn't check for the exit code, only the error message and the message printed on the crash coincidentally contained the expected message.

My best understanding is that this was broken in unknown commit. The root cause of why that change was imperfect is probably the odd behavior that if one posts an error to the event handler in ActionExecutionContext, the action doesn't automatically fail.

RELNOTES: None.
PiperOrigin-RevId: 697564109
Change-Id: I0847bd7f090af44ffd37880b4957755bca511ad1

Author: lberki

src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java

@@ -148,8 +148,7 @@ public PathFragment getExecPath() {
     @Override
     public Map<PathFragment, Artifact> getMapping() {
       if (cachedMapping == null) {
-        return runfiles.getRunfilesInputs(
-            /* eventHandler= */ null, /* location= */ null, repoMappingManifest);
+        return runfiles.getRunfilesInputs(repoMappingManifest);
       }
 
       Map<PathFragment, Artifact> result = cachedMapping.get();
@@ -163,9 +162,7 @@ public Map<PathFragment, Artifact> getMapping() {
           return result;
         }
 
-        result =
-            runfiles.getRunfilesInputs(
-                /* eventHandler= */ null, /* location= */ null, repoMappingManifest);
+        result = runfiles.getRunfilesInputs(repoMappingManifest);
         cachedMapping = new WeakReference<>(result);
         return result;
       }

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

@@ -27,6 +27,9 @@
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactExpander;
+import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.UserExecException;
+import com.google.devtools.build.lib.analysis.Runfiles.ConflictType;
 import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.DeterministicWriter;
 import com.google.devtools.build.lib.analysis.starlark.UnresolvedSymlinkAction;
@@ -35,6 +38,9 @@
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.events.StoredEventHandler;
+import com.google.devtools.build.lib.server.FailureDetails;
+import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.BufferedWriter;
@@ -47,6 +53,7 @@
 import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
+import java.util.function.BiConsumer;
 import javax.annotation.Nullable;
 
 /**
@@ -199,9 +206,7 @@ public synchronized NestedSet<Artifact> getInputs() {
   @VisibleForTesting
   public void writeOutputFile(OutputStream out, @Nullable EventHandler eventHandler)
       throws IOException {
-    writeFile(
-        out,
-        runfiles.getRunfilesInputs(eventHandler, getOwner().getLocation(), repoMappingManifest));
+    writeFile(out, runfiles.getRunfilesInputs(repoMappingManifest));
   }
 
   /**
@@ -222,10 +227,33 @@ public String getStarlarkContent() throws IOException {
   }
 
   @Override
-  public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) {
-    final Map<PathFragment, Artifact> runfilesInputs =
-        runfiles.getRunfilesInputs(
-            ctx.getEventHandler(), getOwner().getLocation(), repoMappingManifest);
+  public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx)
+      throws ExecException {
+    StoredEventHandler eventHandler = new StoredEventHandler();
+    BiConsumer<ConflictType, String> eventReceiver =
+        runfiles.eventRunfilesConflictReceiver(eventHandler, getOwner().getLocation());
+    boolean seenNestedRunfilesTree[] = new boolean[] {false};
+    BiConsumer<ConflictType, String> receiver =
+        (conflictType, message) -> {
+          eventReceiver.accept(conflictType, message);
+          if (conflictType == ConflictType.NESTED_RUNFILES_TREE) {
+            seenNestedRunfilesTree[0] = true;
+          }
+        };
+
+    Map<PathFragment, Artifact> runfilesInputs =
+        runfiles.getRunfilesInputs(receiver, repoMappingManifest);
+    eventHandler.replayOn(ctx.getEventHandler());
+    if (seenNestedRunfilesTree[0]) {
+      FailureDetail failureDetail =
+          FailureDetail.newBuilder()
+              .setMessage("Cannot create input manifest for runfiles tree")
+              .setAnalysis(
+                  FailureDetails.Analysis.newBuilder()
+                      .setCode(FailureDetails.Analysis.Code.INVALID_RUNFILES_TREE))
+              .build();
+      throw new UserExecException(failureDetail);
+    }
     return out -> writeFile(out, runfilesInputs);
   }
 

src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java

@@ -143,12 +143,7 @@ private static ImmutableMap<PathFragment, PathFragment> getFilesetMap(
   private static Map<PathFragment, Artifact> getRunfilesMap(SymlinkTreeAction action) {
     // This call outputs warnings about overlapping symlinks. However, since this has already been
     // called by the SourceManifestAction, we silence the warnings here.
-    return action
-        .getRunfiles()
-        .getRunfilesInputs(
-            /* eventHandler= */ null,
-            action.getOwner().getLocation(),
-            action.getRepoMappingManifest());
+    return action.getRunfiles().getRunfilesInputs(action.getRepoMappingManifest());
   }
 
   private static void createOutput(

src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java

@@ -1256,6 +1256,7 @@ message Analysis {
     INCOMPATIBLE_TARGET_REQUESTED = 19 [(metadata) = { exit_code: 1 }];
     ANALYSIS_FAILURE_PROPAGATION_FAILED = 20 [(metadata) = { exit_code: 1 }];
     ANALYSIS_CACHE_DISCARDED = 21 [(metadata) = { exit_code: 1 }];
+    INVALID_RUNFILES_TREE = 22 [(metadata) = { exit_code: 1 }];
   }
 
   Code code = 1;

src/main/protobuf/failure_details.proto

@@ -28,7 +28,6 @@
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Map.Entry;
 import net.starlark.java.eval.EvalException;
-import net.starlark.java.syntax.Location;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -364,7 +363,7 @@ public void hasMappingForSymlinks() throws Exception {
     ImmutableList<String> runfilesPaths =
         runfilesSupport
             .getRunfiles()
-            .getRunfilesInputs(reporter, Location.BUILTIN, runfilesSupport.getRepoMappingManifest())
+            .getRunfilesInputs(runfilesSupport.getRepoMappingManifest())
             .keySet()
             .stream()
             .map(PathFragment::getPathString)