Automated rollback of commit 00a4fef.

haxorz · copybara-github · commit 4344a0358f44 · 2023-06-01T19:35:25.000-07:00
*** Reason for rollback *** b/285381501 *** Original change description *** Use `PackageLookupValue` to do package lookup and subpackage boundary cross check in `BzlLoadFunction`. This is a similar change as c3a838b in which we switch from using `ContainingPackageLookupValue` to `PackageLookupValue` for `PackageFunction`. PiperOrigin-RevId: 537190556 Change-Id: Id6368b98cd80575f728ee91e730a727ac164459b
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,7 +50,6 @@
 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;
@@ -64,7 +63,6 @@
 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;
@@ -646,103 +644,45 @@ private BzlCompileValue.Key validatePackageAndGetCompileKey(
       return key.getCompileKey(getBuiltinsRoot(builtinsBzlPath));
     }
 
-    // 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;
-      }
+    // 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;
     }
 
-    if (candidateKey != null && candidateKey.argument().equals(label.getPackageIdentifier())) {
-      if (candidateValue.packageExists()) {
-        return key.getCompileKey(candidateValue.getRoot());
-      } else {
-        throw BzlLoadFailedException.noBuildFile(label, candidateValue.getErrorMsg());
-      }
+    // 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 (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);
+      if (packageOk) {
+        compileKey = key.getCompileKey(packageLookup.getContainingPackageRoot());
+      } else {
+        if (!packageLookup.hasContainingPackage()) {
+          throw BzlLoadFailedException.noBuildFile(
+              label, packageLookup.getReasonForNoContainingPackage());
         } else {
-          throw BzlLoadFailedException.noBuildFile(label, /* reason= */ null);
+          throw BzlLoadFailedException.labelCrossesPackageBoundary(label, packageLookup);
         }
-      } else {
-        throw BzlLoadFailedException.subpackageCrossesLabelPackageBoundary(
-            label, candidateKey.argument(), candidateValue);
       }
     }
+    return compileKey;
   }
 
   private Root getBuiltinsRoot(String builtinsBzlPath) {
@@ -1643,21 +1583,17 @@ static BzlLoadFailedException noBuildFile(Label file, @Nullable String reason) {
           Code.PACKAGE_NOT_FOUND);
     }
 
-    static BzlLoadFailedException labelSubpackageCrossesBoundary(
+    static BzlLoadFailedException labelCrossesPackageBoundary(
         Label label, ContainingPackageLookupValue containingPackageLookupValue) {
       return new BzlLoadFailedException(
-          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),
+          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),
           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,27 +59,48 @@ public static Key key(PackageIdentifier id) {
     return Key.create(id);
   }
 
-  /**
-   * 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) {
+  static String getErrorMessageForLabelCrossingPackageBoundary(
+      Root pkgRoot,
+      Label label,
+      ContainingPackageLookupValue containingPkgLookupValue) {
     PackageIdentifier containingPkg = containingPkgLookupValue.getContainingPackageName();
-    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 + "'?";
+    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";
+    }
     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.getErrorMessageForSubpackageCrossesLabelPackageBoundary(
+        PackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(
             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} if it contains a subpackage
-   * crossing boundary.
+   * Creates the error message for the input {@linkplain Label label} has a subpackage crossing
+   * boundary.
    *
    * <p>Returns {@code null} if no subpackage is discovered or the subpackage is marked as DELETED.
    */
   @Nullable
-  static String getErrorMessageForSubpackageCrossesLabelPackageBoundary(
+  static String getErrorMessageForLabelCrossingPackageBoundary(
       Root pkgRoot,
       Label label,
       PackageIdentifier subpackageIdentifier,
@@ -422,19 +422,19 @@ static String getErrorMessageForSubpackageCrossesLabelPackageBoundary(
       if (pkgRoot.equals(subPackageRoot)) {
         PathFragment labelRootPathFragment = label.getPackageIdentifier().getSourceRoot();
         PathFragment subpackagePathFragment = subpackageIdentifier.getSourceRoot();
-        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 += "//";
+        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?
         }
-        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,7 +1679,9 @@ 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.getErrorMessageForSubpackageCrossesLabelPackageBoundary(`
	`884`	`+ PackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(`
`885`	`885`	`pkgRoot, target.getLabel(), subpackageIdentifier, packageLookupValue);`
`886`	`886`	`if (errMsg != null) {`
`887`	`887`	`Event error =`