In SkyQueryEnvironment, don't silently give up when there's a cycle in the graph. We can compute the universe target patterns outside of skyframe, which is the only reason we need the value we were requesting. Giving up was preventing us from evaluating "..." patterns even if the "..." pattern didn't contain any cycles itself.

--
MOS_MIGRATED_REVID=111605976

4 files changed, 46 insertions, 29 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
index 78aab4a486..614f502087 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
@@ -55,7 +55,7 @@ import com.google.devtools.build.lib.skyframe.FileValue;
 import com.google.devtools.build.lib.skyframe.GraphBackedRecursivePackageProvider;
 import com.google.devtools.build.lib.skyframe.PackageLookupValue;
 import com.google.devtools.build.lib.skyframe.PackageValue;
-import com.google.devtools.build.lib.skyframe.PrepareDepsOfPatternsValue;
+import com.google.devtools.build.lib.skyframe.PrepareDepsOfPatternsFunction;
 import com.google.devtools.build.lib.skyframe.RecursivePackageProviderBackedTargetPatternResolver;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue;
@@ -147,21 +147,23 @@ public class SkyQueryEnvironment extends AbstractBlazeQueryEnvironment<Target> {
       LOG.info("Spent " + (duration / 1000 / 1000) + " ms on evaluation and walkable graph");
     }
 
+    SkyKey universeKey = graphFactory.getUniverseKey(universeScope, parserPrefix);
+    universeTargetPatternKeys =
+        PrepareDepsOfPatternsFunction.getTargetPatternKeys(
+            PrepareDepsOfPatternsFunction.getSkyKeys(universeKey, eventHandler));
+
     // The prepareAndGet call above evaluates a single PrepareDepsOfPatterns SkyKey.
     // We expect to see either a single successfully evaluated value or a cycle in the result.
     Collection<SkyValue> values = result.values();
     if (!values.isEmpty()) {
       Preconditions.checkState(values.size() == 1, "Universe query \"%s\" returned multiple"
               + " values unexpectedly (%s values in result)", universeScope, values.size());
-      PrepareDepsOfPatternsValue prepareDepsOfPatternsValue =
-          (PrepareDepsOfPatternsValue) Iterables.getOnlyElement(values);
-      universeTargetPatternKeys = prepareDepsOfPatternsValue.getTargetPatternKeys();
+      Preconditions.checkNotNull(result.get(universeKey), result);
     } else {
       // No values in the result, so there must be an error. We expect the error to be a cycle.
       boolean foundCycle = !Iterables.isEmpty(result.getError().getCycleInfo());
       Preconditions.checkState(foundCycle, "Universe query \"%s\" failed with non-cycle error: %s",
           universeScope, result.getError());
-      universeTargetPatternKeys = ImmutableList.of();
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunction.java
index db306d2bb9..60ec9caec1 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunction.java
@@ -39,23 +39,14 @@ import javax.annotation.Nullable;
  */
 public class PrepareDepsOfPatternsFunction implements SkyFunction {
 
-  /**
-   * Given a {@link SkyKey} that contains a sequence of target patterns, when this function returns
-   * {@link PrepareDepsOfPatternsValue}, then all targets matching that sequence, and those targets'
-   * transitive dependencies, have been loaded.
-   */
-  @Nullable
-  @Override
-  public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
-    EventHandler eventHandler = env.getListener();
-    boolean handlerIsParseFailureListener = eventHandler instanceof ParseFailureListener;
+  public static ImmutableList<SkyKey> getSkyKeys(SkyKey skyKey, EventHandler eventHandler) {
     TargetPatternSequence targetPatternSequence = (TargetPatternSequence) skyKey.argument();
-
     Iterable<PrepareDepsOfPatternSkyKeyOrException> keysMaybe =
         PrepareDepsOfPatternValue.keys(targetPatternSequence.getPatterns(),
             targetPatternSequence.getOffset());
 
     ImmutableList.Builder<SkyKey> skyKeyBuilder = ImmutableList.builder();
+    boolean handlerIsParseFailureListener = eventHandler instanceof ParseFailureListener;
     for (PrepareDepsOfPatternSkyKeyOrException skyKeyOrException : keysMaybe) {
       try {
         skyKeyBuilder.add(skyKeyOrException.getSkyKey());
@@ -64,7 +55,34 @@ public class PrepareDepsOfPatternsFunction implements SkyFunction {
             skyKeyOrException.getOriginalPattern(), e);
       }
     }
-    ImmutableList<SkyKey> skyKeys = skyKeyBuilder.build();
+
+    return skyKeyBuilder.build();
+  }
+
+  private static final Function<SkyKey, TargetPatternKey> SKY_TO_TARGET_PATTERN =
+      new Function<SkyKey, TargetPatternKey>() {
+        @Nullable
+        @Override
+        public TargetPatternKey apply(SkyKey skyKey) {
+          return (TargetPatternKey) skyKey.argument();
+        }
+      };
+
+  public static ImmutableList<TargetPatternKey> getTargetPatternKeys(
+      ImmutableList<SkyKey> skyKeys) {
+    return ImmutableList.copyOf(Iterables.transform(skyKeys, SKY_TO_TARGET_PATTERN));
+  }
+
+  /**
+   * Given a {@link SkyKey} that contains a sequence of target patterns, when this function returns
+   * {@link PrepareDepsOfPatternsValue}, then all targets matching that sequence, and those targets'
+   * transitive dependencies, have been loaded.
+   */
+  @Nullable
+  @Override
+  public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
+    EventHandler eventHandler = env.getListener();
+    ImmutableList<SkyKey> skyKeys = getSkyKeys(skyKey, eventHandler);
 
     Map<SkyKey, ValueOrException<TargetParsingException>> tokensByKey =
         env.getValuesOrThrow(skyKeys, TargetParsingException.class);
@@ -72,6 +90,7 @@ public class PrepareDepsOfPatternsFunction implements SkyFunction {
       return null;
     }
 
+    boolean handlerIsParseFailureListener = eventHandler instanceof ParseFailureListener;
     for (SkyKey key : skyKeys) {
       try {
         // The only exception type throwable by PrepareDepsOfPatternFunction is
@@ -84,15 +103,7 @@ public class PrepareDepsOfPatternsFunction implements SkyFunction {
       }
     }
 
-    ImmutableList<TargetPatternKey> targetPatternKeys =
-        ImmutableList.copyOf(Iterables.transform(skyKeys,
-            new Function<SkyKey, TargetPatternKey>() {
-              @Override
-              public TargetPatternKey apply(SkyKey skyKey) {
-                return (TargetPatternKey) skyKey.argument();
-              }
-            }));
-    return new PrepareDepsOfPatternsValue(targetPatternKeys);
+    return new PrepareDepsOfPatternsValue(getTargetPatternKeys(skyKeys));
   }
 
   private static void handleTargetParsingException(EventHandler eventHandler,
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 883a605a11..d72094201c 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
@@ -1444,7 +1444,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   @Override
   public EvaluationResult<SkyValue> prepareAndGet(Collection<String> patterns, String offset,
       int numThreads, EventHandler eventHandler) throws InterruptedException {
-    SkyKey skyKey = getPrepareDepsKey(patterns, offset);
+    SkyKey skyKey = getUniverseKey(patterns, offset);
     EvaluationResult<SkyValue> evaluationResult =
         buildDriver.evaluate(ImmutableList.of(skyKey), true, numThreads, eventHandler);
     Preconditions.checkNotNull(evaluationResult.getWalkableGraph(), patterns);
@@ -1456,10 +1456,11 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
    * underlying evaluation implementation.
    */
   public String prepareAndGetMetadata(Collection<String> patterns, String offset) {
-    return buildDriver.meta(ImmutableList.of(getPrepareDepsKey(patterns, offset)));
+    return buildDriver.meta(ImmutableList.of(getUniverseKey(patterns, offset)));
   }
 
-  private SkyKey getPrepareDepsKey(Collection<String> patterns, String offset) {
+  @Override
+  public SkyKey getUniverseKey(Collection<String> patterns, String offset) {
     return PrepareDepsOfPatternsValue.key(ImmutableList.copyOf(patterns), offset);
   }
 
diff --git a/src/main/java/com/google/devtools/build/skyframe/WalkableGraph.java b/src/main/java/com/google/devtools/build/skyframe/WalkableGraph.java
index 89fe2fd54d..0c073a7565 100644
--- a/src/main/java/com/google/devtools/build/skyframe/WalkableGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/WalkableGraph.java
@@ -79,5 +79,8 @@ public interface WalkableGraph {
   interface WalkableGraphFactory {
     EvaluationResult<SkyValue> prepareAndGet(Collection<String> roots, String offset,
         int numThreads, EventHandler eventHandler) throws InterruptedException;
+
+    /** Returns the {@link SkyKey} that defines this universe. */
+    SkyKey getUniverseKey(Collection<String> roots, String offset);
   }
 }