Move tests from SkyframeLoadingAndAnalysisTest to LoadingPhaseRunnerTest.

This exposes an issue with the new Skyframe-based implementation, which is
fixed here as well.

--
MOS_MIGRATED_REVID=113556169

2 files changed, 62 insertions, 1 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index bfc7fb3453..f60b333036 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1760,9 +1760,19 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
               ImmutableList.of(key), keepGoing, /*numThreads=*/10, eventHandler);
       if (evalResult.hasError()) {
         ErrorInfo errorInfo = evalResult.getError(key);
-        if (errorInfo != null && errorInfo.getException() != null) {
+        if (!Iterables.isEmpty(errorInfo.getCycleInfo())) {
+          String errorMessage = "cycles detected during target parsing";
+          getCyclesReporter().reportCycles(errorInfo.getCycleInfo(), key, eventHandler);
+          throw new TargetParsingException(errorMessage);
+        }
+        if (errorInfo.getException() != null) {
           Exception e = errorInfo.getException();
           Throwables.propagateIfInstanceOf(e, TargetParsingException.class);
+          if (!keepGoing) {
+            // This is the same code as in SkyframeTargetPatternEvaluator; we allow any exception
+            // and turn it into a TargetParsingException here.
+            throw new TargetParsingException(e.getMessage());
+          }
           throw new IllegalStateException("Unexpected Exception type from TargetPatternPhaseValue "
               + "for '" + targetPatterns + "'' with root causes: "
               + Iterables.toString(errorInfo.getRootCauses()), e);
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
index 74d27fc629..f933199b2f 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java
@@ -18,6 +18,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import com.google.common.base.Functions;
 import com.google.common.base.Joiner;
@@ -34,6 +35,7 @@ import com.google.devtools.build.lib.analysis.BuildView;
 import com.google.devtools.build.lib.analysis.util.AnalysisMock;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.cmdline.TargetParsingException;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.StoredEventHandler;
@@ -448,6 +450,55 @@ public class LoadingPhaseRunnerTest {
         .containsExactly("//foo:a.y", "//foo:b.y");
   }
 
+  /** Regression test: handle symlink cycles gracefully. */
+  @Test
+  public void testCycleReporting_SymlinkCycleDuringTargetParsing() throws Exception {
+    tester.addFile("hello/BUILD", "cc_library(name = 'a', srcs = glob(['*.cc']))");
+    Path buildFilePath = tester.getWorkspace().getRelative("hello/BUILD");
+    Path dirPath = buildFilePath.getParentDirectory();
+    Path fooFilePath = dirPath.getRelative("foo.cc");
+    Path barFilePath = dirPath.getRelative("bar.cc");
+    Path bazFilePath = dirPath.getRelative("baz.cc");
+    fooFilePath.createSymbolicLink(barFilePath);
+    barFilePath.createSymbolicLink(bazFilePath);
+    bazFilePath.createSymbolicLink(fooFilePath);
+    assertCircularSymlinksDuringTargetParsing("//hello:a");
+  }
+
+  @Test
+  public void testRecursivePatternWithCircularSymlink() throws Exception {
+    tester.getWorkspace().getChild("broken").createDirectory();
+
+    // Create a circular symlink.
+    tester.getWorkspace().getRelative(new PathFragment("broken/BUILD"))
+        .createSymbolicLink(new PathFragment("BUILD"));
+
+    assertCircularSymlinksDuringTargetParsing("//broken/...");
+  }
+
+  @Test
+  public void testRecursivePatternWithTwoCircularSymlinks() throws Exception {
+    tester.getWorkspace().getChild("broken").createDirectory();
+
+    // Create a circular symlink.
+    tester.getWorkspace().getRelative(new PathFragment("broken/BUILD"))
+        .createSymbolicLink(new PathFragment("x"));
+    tester.getWorkspace().getRelative(new PathFragment("broken/x"))
+        .createSymbolicLink(new PathFragment("BUILD"));
+
+    assertCircularSymlinksDuringTargetParsing("//broken/...");
+  }
+
+  private void assertCircularSymlinksDuringTargetParsing(String targetPattern) throws Exception {
+    try {
+      tester.load(targetPattern);
+      fail();
+    } catch (TargetParsingException e) {
+      // Expected.
+      tester.assertContainsError("circular symlinks detected");
+    }
+  }
+
   private LoadingResult assertNoErrors(LoadingResult loadingResult) {
     assertFalse(loadingResult.hasTargetPatternError());
     assertFalse(loadingResult.hasLoadingError());