Use PackageLookupValue to do package lookup and subpackage boundary cross check in BzlLoadFunction.

yuyue730 · copybara-github · commit 00a4fefe5940 · 2023-05-24T14:51:03.000-07:00
This is a similar change as bazelbuild@c3a838b in which we switch from using `ContainingPackageLookupValue` to `PackageLookupValue` for `PackageFunction`. PiperOrigin-RevId: 534989868 Change-Id: Ic06ffbea8b13ad5eff4001049791c9a512e0211d
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
@@ -50,6 +50,7 @@
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.server.FailureDetails.StarlarkLoading;
 import com.google.devtools.build.lib.server.FailureDetails.StarlarkLoading.Code;
+import com.google.devtools.build.lib.skyframe.PackageLookupValue.NoRepositoryPackageLookupValue;
 import com.google.devtools.build.lib.skyframe.StarlarkBuiltinsFunction.BuiltinsFailedException;
 import com.google.devtools.build.lib.util.DetailedExitCode;
 import com.google.devtools.build.lib.util.Fingerprint;
@@ -63,6 +64,7 @@
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.SkyframeLookupResult;
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
@@ -644,45 +646,103 @@ private BzlCompileValue.Key validatePackageAndGetCompileKey(
       return key.getCompileKey(getBuiltinsRoot(builtinsBzlPath));
     }
 
-    // Do package lookup.
-    PathFragment dir = Label.getContainingDirectory(label);
-    PackageIdentifier dirId = PackageIdentifier.create(label.getRepository(), dir);
-    ContainingPackageLookupValue packageLookup;
-    try {
-      packageLookup =
-          (ContainingPackageLookupValue)
-              env.getValueOrThrow(
-                  ContainingPackageLookupValue.key(dirId),
-                  BuildFileNotFoundException.class,
-                  InconsistentFilesystemException.class);
-    } catch (BuildFileNotFoundException | InconsistentFilesystemException e) {
-      throw BzlLoadFailedException.errorFindingContainingPackage(label.toPathFragment(), e);
-    }
-    if (packageLookup == null) {
-      return null;
+    // The block below derives all (sub)directories that could possibly contain (sub)packages and
+    // add them to a list of PackageLookup keys. These (sub)directories include the label path, and
+    // all subdirectories from label path to the bzl file. For example,
+    // 1. If the label is //a/b/c:d.bzl, allPackageLookupKeys only contains //a/b/c. There is no
+    // subdirectory under label path.
+    // 2. If the label name contains '/', for example, //a/b/c:d/e/f.bzl, allPackageLookupKeys
+    // contain //a/b/c, //a/b/c/d and //a/b/c/d/e.
+    List<PackageLookupValue.Key> allPackageLookupKeys = new ArrayList<>();
+    allPackageLookupKeys.add(PackageLookupValue.key(label.getPackageIdentifier()));
+    RepositoryName labelRepository = label.getRepository();
+    PathFragment subpkgPath = label.getPackageFragment();
+    PathFragment labelAsRelativePath = PathFragment.create(label.getName()).getParentDirectory();
+    for (String segment : labelAsRelativePath.segments()) {
+      subpkgPath = subpkgPath.getRelative(segment);
+      PackageLookupValue.Key currentPackageLookupKey =
+          PackageLookupValue.key(PackageIdentifier.create(labelRepository, subpkgPath));
+      allPackageLookupKeys.add(currentPackageLookupKey);
+    }
+
+    SkyframeLookupResult packageLookupResults = env.getValuesAndExceptions(allPackageLookupKeys);
+
+    // We intentionally choose not to check `env.valuesMissing()` here. It is possible that all
+    // PackageLookupValues are already not null but `env.valuesMissing()` is still true from a prior
+    // request. Returning `null` in this case causes unnecessary Skyframe restarts.
+
+    PackageLookupValue.Key candidateKey = null;
+    PackageLookupValue candidateValue = null;
+    for (PackageLookupValue.Key packageLookupKey : allPackageLookupKeys) {
+      // Iterate in order of the directory structure so that the candidate{Key,Value} will end up as
+      // the deepest package, in other words the "containing package".
+      PackageLookupValue packageLookupValue;
+      try {
+        packageLookupValue =
+            (PackageLookupValue)
+                packageLookupResults.getOrThrow(
+                    packageLookupKey,
+                    BuildFileNotFoundException.class,
+                    InconsistentFilesystemException.class);
+      } catch (BuildFileNotFoundException | InconsistentFilesystemException e) {
+        throw BzlLoadFailedException.errorFindingContainingPackage(label.toPathFragment(), e);
+      }
+
+      if (packageLookupValue == null) {
+        return null;
+      }
+
+      if (packageLookupValue instanceof NoRepositoryPackageLookupValue) {
+        throw BzlLoadFailedException.noBuildFile(label, packageLookupValue.getErrorMsg());
+      }
+
+      if (packageLookupValue.packageExists()) {
+        candidateKey = packageLookupKey;
+        candidateValue = packageLookupValue;
+      }
     }
 
-    // Resolve to compile key or error.
-    BzlCompileValue.Key compileKey;
-    boolean packageOk =
-        packageLookup.hasContainingPackage()
-            && packageLookup.getContainingPackageName().equals(label.getPackageIdentifier());
-    if (key.isBuildPrelude() && !packageOk) {
-      // Ignore the prelude, its package doesn't exist.
-      compileKey = BzlCompileValue.EMPTY_PRELUDE_KEY;
-    } else {
-      if (packageOk) {
-        compileKey = key.getCompileKey(packageLookup.getContainingPackageRoot());
+    if (candidateKey != null && candidateKey.argument().equals(label.getPackageIdentifier())) {
+      if (candidateValue.packageExists()) {
+        return key.getCompileKey(candidateValue.getRoot());
       } else {
-        if (!packageLookup.hasContainingPackage()) {
-          throw BzlLoadFailedException.noBuildFile(
-              label, packageLookup.getReasonForNoContainingPackage());
+        throw BzlLoadFailedException.noBuildFile(label, candidateValue.getErrorMsg());
+      }
+    } else {
+      if (key.isBuildPrelude()) {
+        return BzlCompileValue.EMPTY_PRELUDE_KEY;
+      }
+      if (candidateKey == null) {
+        // If we cannot find any subpackage below label's package directory, it is still possible
+        // that the label's package is a subpackage itself. This case should be rare, so we choose
+        // to still handle it using ContainingPackageLookup node.
+        ContainingPackageLookupValue containingPackageLookup;
+        try {
+          containingPackageLookup =
+              (ContainingPackageLookupValue)
+                  env.getValueOrThrow(
+                      ContainingPackageLookupValue.key(label.getPackageIdentifier()),
+                      BuildFileNotFoundException.class,
+                      InconsistentFilesystemException.class);
+        } catch (BuildFileNotFoundException | InconsistentFilesystemException e) {
+          throw BzlLoadFailedException.errorFindingContainingPackage(label.toPathFragment(), e);
+        }
+
+        if (containingPackageLookup == null) {
+          return null;
+        }
+
+        if (containingPackageLookup.hasContainingPackage()) {
+          throw BzlLoadFailedException.labelSubpackageCrossesBoundary(
+              label, containingPackageLookup);
         } else {
-          throw BzlLoadFailedException.labelCrossesPackageBoundary(label, packageLookup);
+          throw BzlLoadFailedException.noBuildFile(label, /* reason= */ null);
         }
+      } else {
+        throw BzlLoadFailedException.subpackageCrossesLabelPackageBoundary(
+            label, candidateKey.argument(), candidateValue);
       }
     }
-    return compileKey;
   }
 
   private Root getBuiltinsRoot(String builtinsBzlPath) {
@@ -1583,17 +1643,21 @@ static BzlLoadFailedException noBuildFile(Label file, @Nullable String reason) {
           Code.PACKAGE_NOT_FOUND);
     }
 
-    static BzlLoadFailedException labelCrossesPackageBoundary(
+    static BzlLoadFailedException labelSubpackageCrossesBoundary(
         Label label, ContainingPackageLookupValue containingPackageLookupValue) {
       return new BzlLoadFailedException(
-          ContainingPackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(
-              // We don't actually know the proper Root to pass in here (since we don't e.g. know
-              // the root of the bzl/BUILD file that is trying to load 'label'). Therefore we just
-              // pass in the Root of the containing package in order to still get a useful error
-              // message for the user.
-              containingPackageLookupValue.getContainingPackageRoot(),
-              label,
-              containingPackageLookupValue),
+          ContainingPackageLookupValue.getErrorMessageForLabelSubpackageCrossesBoundary(
+              containingPackageLookupValue, label),
+          Code.LABEL_CROSSES_PACKAGE_BOUNDARY);
+    }
+
+    static BzlLoadFailedException subpackageCrossesLabelPackageBoundary(
+        Label label,
+        PackageIdentifier subpackageIdentifier,
+        PackageLookupValue packageLookupValue) {
+      return new BzlLoadFailedException(
+          PackageLookupValue.getErrorMessageForSubpackageCrossesLabelPackageBoundary(
+              packageLookupValue.getRoot(), label, subpackageIdentifier, packageLookupValue),
           Code.LABEL_CROSSES_PACKAGE_BOUNDARY);
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java
@@ -59,48 +59,27 @@ public static Key key(PackageIdentifier id) {
     return Key.create(id);
   }
 
-  static String getErrorMessageForLabelCrossingPackageBoundary(
-      Root pkgRoot,
-      Label label,
-      ContainingPackageLookupValue containingPkgLookupValue) {
+  /**
+   * Creates the error message for the input {@linkplain Label label} if label itself is a
+   * subpackage crosses boundary when an outer package exists.
+   */
+  static String getErrorMessageForLabelSubpackageCrossesBoundary(
+      ContainingPackageLookupValue containingPkgLookupValue, Label label) {
     PackageIdentifier containingPkg = containingPkgLookupValue.getContainingPackageName();
-    boolean crossesPackageBoundaryBelow =
-        containingPkg.getSourceRoot().startsWith(label.getPackageIdentifier().getSourceRoot());
-    PathFragment labelNameFragment = PathFragment.create(label.getName());
-    String message;
-    if (crossesPackageBoundaryBelow) {
-      message =
-          String.format("Label '%s' is invalid because '%s' is a subpackage", label, containingPkg);
-    } else {
-      message =
-          String.format(
-              "Label '%s' is invalid because '%s' is not a package", label, label.getPackageName());
-    }
-
-    Root containingRoot = containingPkgLookupValue.getContainingPackageRoot();
-    if (pkgRoot.equals(containingRoot)) {
-      PathFragment containingPkgFragment = containingPkg.getPackageFragment();
-      PathFragment labelNameInContainingPackage =
-          crossesPackageBoundaryBelow
-              ? labelNameFragment.subFragment(
-                  containingPkgFragment.segmentCount()
-                      - label.getPackageFragment().segmentCount(),
-                  labelNameFragment.segmentCount())
-              : label.toPathFragment().relativeTo(containingPkgFragment);
-      message += "; perhaps you meant to put the colon here: '";
-      if (containingPkg.getRepository().isMain()) {
-        message += "//";
-      }
-      message += containingPkg + ":" + labelNameInContainingPackage + "'?";
-    } else {
-      message +=
-          "; have you deleted "
-              + containingPkg
-              + "/BUILD? "
-              + "If so, use the --deleted_packages="
-              + containingPkg
-              + " option";
-    }
+    Preconditions.checkState(
+        label.getPackageIdentifier().getSourceRoot().startsWith(containingPkg.getSourceRoot()),
+        "Label's path should start with outer package's path.");
+
+    String message =
+        String.format(
+            "Label '%s' is invalid because '%s' is not a package", label, label.getPackageName());
+    PathFragment labelNameInContainingPackage =
+        label.toPathFragment().relativeTo(containingPkg.getPackageFragment());
+    message += "; perhaps you meant to put the colon here: '";
+    if (containingPkg.getRepository().isMain()) {
+      message += "//";
+    }
+    message += containingPkg + ":" + labelNameInContainingPackage + "'?";
     return message;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -881,7 +881,7 @@ private static boolean maybeAddEventAboutLabelCrossingSubpackage(
       return true;
     }
     String errMsg =
-        PackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(
+        PackageLookupValue.getErrorMessageForSubpackageCrossesLabelPackageBoundary(
             pkgRoot, target.getLabel(), subpackageIdentifier, packageLookupValue);
     if (errMsg != null) {
       Event error =
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java
@@ -401,13 +401,13 @@ public String getErrorMsg() {
   }
 
   /**
-   * Creates the error message for the input {@linkplain Label label} has a subpackage crossing
-   * boundary.
+   * Creates the error message for the input {@linkplain Label label} if it contains a subpackage
+   * crossing boundary.
    *
    * <p>Returns {@code null} if no subpackage is discovered or the subpackage is marked as DELETED.
    */
   @Nullable
-  static String getErrorMessageForLabelCrossingPackageBoundary(
+  static String getErrorMessageForSubpackageCrossesLabelPackageBoundary(
       Root pkgRoot,
       Label label,
       PackageIdentifier subpackageIdentifier,
@@ -422,19 +422,19 @@ static String getErrorMessageForLabelCrossingPackageBoundary(
       if (pkgRoot.equals(subPackageRoot)) {
         PathFragment labelRootPathFragment = label.getPackageIdentifier().getSourceRoot();
         PathFragment subpackagePathFragment = subpackageIdentifier.getSourceRoot();
-        if (subpackagePathFragment.startsWith(labelRootPathFragment)) {
-          PathFragment labelNameInSubpackage =
-              PathFragment.create(label.getName())
-                  .subFragment(
-                      subpackagePathFragment.segmentCount() - labelRootPathFragment.segmentCount());
-          message += "; perhaps you meant to put the" + " colon here: '";
-          if (subpackageIdentifier.getRepository().isMain()) {
-            message += "//";
-          }
-          message += subpackageIdentifier + ":" + labelNameInSubpackage + "'?";
-        } else {
-          // TODO: Is this a valid case? How do we handle this case?
+        Preconditions.checkState(
+            subpackagePathFragment.startsWith(labelRootPathFragment),
+            "Subpackage should start with label's package path when they share the same package"
+                + " root");
+        PathFragment labelNameInSubpackage =
+            PathFragment.create(label.getName())
+                .subFragment(
+                    subpackagePathFragment.segmentCount() - labelRootPathFragment.segmentCount());
+        message += "; perhaps you meant to put the" + " colon here: '";
+        if (subpackageIdentifier.getRepository().isMain()) {
+          message += "//";
         }
+        message += subpackageIdentifier + ":" + labelNameInSubpackage + "'?";
       } else {
         message +=
             "; have you deleted "
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
@@ -1679,9 +1679,7 @@ public void testPreludeCanAccessBzlDialectFeatures() throws Exception {
 
     @Test
     public void testPreludeNeedNotBePresent() throws Exception {
-      scratch.file(
-          "pkg/BUILD", //
-          "print('FOO')");
+      scratch.file("pkg/BUILD", "print('FOO')");
 
       getConfiguredTarget("//pkg:BUILD");
       assertContainsEvent("FOO");

Original file line number	Diff line number	Diff line change
`@@ -881,7 +881,7 @@ private static boolean maybeAddEventAboutLabelCrossingSubpackage(`
`881`	`881`	`return true;`
`882`	`882`	`}`
`883`	`883`	`String errMsg =`
`884`		`- PackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(`
	`884`	`+ PackageLookupValue.getErrorMessageForSubpackageCrossesLabelPackageBoundary(`
`885`	`885`	`pkgRoot, target.getLabel(), subpackageIdentifier, packageLookupValue);`
`886`	`886`	`if (errMsg != null) {`
`887`	`887`	`Event error =`