Tolerate NoSuchPackageException when processing subdirectories.

In RecursiveDirectoryTraversalFunction, we must tolerate
NoSuchPackageException being thrown by subdirectories' nodes, since
that can happen in a nokeep_going build.

--
Change-Id: Id9a48256aa209775f27130186c58e03c788d20a9
Reviewed-on: https://bazel-review.googlesource.com/#/c/3392/5
MOS_MIGRATED_REVID=120081575

3 files changed, 30 insertions, 20 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
index 72d07b8542..09372ef1ff 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
@@ -13,9 +13,9 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.base.Function;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
@@ -112,18 +112,6 @@ abstract class RecursiveDirectoryTraversalFunction
     void visitPackageValue(Package pkg, Environment env);
   }
 
-  private static final Function<ValueOrException<NoSuchPackageException>, SkyValue> GET_SKYVALUE =
-      new Function<ValueOrException<NoSuchPackageException>, SkyValue>() {
-        @Override
-        public SkyValue apply(ValueOrException<NoSuchPackageException> val) {
-          try {
-            return Preconditions.checkNotNull(val.get(), val);
-          } catch (NoSuchPackageException e) {
-            throw new IllegalStateException(e);
-          }
-        }
-      };
-
   /**
    * Looks in the directory specified by {@code recursivePkgKey} for a package, does some work
    * as specified by {@link Visitor} if such a package exists, then recursively does work in each
@@ -300,10 +288,17 @@ abstract class RecursiveDirectoryTraversalFunction
           return null;
         }
       }
-      subdirectorySkyValues =
-          Maps.transformValues(
-              Maps.filterKeys(dependentSkyValues, Predicates.not(Predicates.equalTo(packageKey))),
-              GET_SKYVALUE);
+      ImmutableMap.Builder<SkyKey, SkyValue> subdirectoryBuilder = ImmutableMap.builder();
+      for (Map.Entry<SkyKey, ValueOrException<NoSuchPackageException>> entry :
+          Maps.filterKeys(dependentSkyValues, Predicates.not(Predicates.equalTo(packageKey)))
+              .entrySet()) {
+        try {
+          subdirectoryBuilder.put(entry.getKey(), entry.getValue().get());
+        } catch (NoSuchPackageException e) {
+          // ignored.
+        }
+      }
+      subdirectorySkyValues = subdirectoryBuilder.build();
     } else {
       subdirectorySkyValues = env.getValues(childDeps);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java
index a701802c06..607e9b2c4e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgFunction.java
@@ -19,6 +19,7 @@ import com.google.devtools.build.lib.cmdline.RepositoryName;
 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.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.skyframe.RecursivePkgValue.RecursivePkgKey;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -45,6 +46,7 @@ public class RecursivePkgFunction implements SkyFunction {
     this.directories = directories;
   }
 
+  /** N.B.: May silently throw {@link NoSuchPackageException} in nokeep_going mode! */
   @Override
   public SkyValue compute(SkyKey skyKey, Environment env) {
     return new MyTraversalFunction().visitDirectory((RecursivePkgKey) skyKey.argument(), env);
@@ -79,9 +81,7 @@ public class RecursivePkgFunction implements SkyFunction {
         Map<SkyKey, SkyValue> subdirectorySkyValues) {
       // Aggregate the transitive subpackages.
       for (SkyValue childValue : subdirectorySkyValues.values()) {
-        if (childValue != null) {
-          visitor.addTransitivePackages(((RecursivePkgValue) childValue).getPackages());
-        }
+        visitor.addTransitivePackages(((RecursivePkgValue) childValue).getPackages());
       }
       return visitor.createRecursivePkgValue();
     }
diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java
index dc32181c88..68b9f64a35 100644
--- a/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java
@@ -676,6 +676,21 @@ public class TargetPatternEvaluatorTest extends AbstractTargetPatternEvaluatorTe
     }
   }
 
+  @Test
+  public void testSubdirectoryCircularSymlinkNoKeepGoingPrimedParent() throws Exception {
+    setupSubDirectoryCircularSymlink();
+    // We make sure that the parent package is present, so that in the subsequent nokeep_going
+    // build, the pattern parsing will have as many of its deps available as possible, which
+    // exercises more code coverage during error bubbling.
+    assertThat(parseList("//parent:all")).containsExactly(Label.parseAbsolute("//parent:parent"));
+    try {
+      parseList("//parent/...");
+      fail();
+    } catch (TargetParsingException e) {
+      // Expected.
+    }
+  }
+
   /** Regression test for bug: "Bogus 'helpful' error message" */
   @Test
   public void testHelpfulMessageForFileOutsideOfAnyPackage() throws Exception {