Automated rollback of commit 00a4fefe594069d47d1bde99b28c6b8dcca0a7c1.
*** 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 https://github.com/bazelbuild/bazel/commit/c3a838b172088e0eaa6da0745cea0e07bcf646a1 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
index 1b30aa2..876d56c 100644
--- 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 @@
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);
+ // 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;
}
- 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;
- }
- }
-
- 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);
- } else {
- throw BzlLoadFailedException.noBuildFile(label, /* reason= */ null);
- }
+ if (packageOk) {
+ compileKey = key.getCompileKey(packageLookup.getContainingPackageRoot());
} else {
- throw BzlLoadFailedException.subpackageCrossesLabelPackageBoundary(
- label, candidateKey.argument(), candidateValue);
+ if (!packageLookup.hasContainingPackage()) {
+ throw BzlLoadFailedException.noBuildFile(
+ label, packageLookup.getReasonForNoContainingPackage());
+ } else {
+ throw BzlLoadFailedException.labelCrossesPackageBoundary(label, packageLookup);
+ }
}
}
+ return compileKey;
}
private Root getBuiltinsRoot(String builtinsBzlPath) {
@@ -1643,21 +1583,17 @@
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
index acfc817..444c15b 100644
--- 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 @@
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 += "//";
+ 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());
}
- message += containingPkg + ":" + labelNameInContainingPackage + "'?";
+
+ 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
index 08d73e4..30d84e8 100644
--- 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 @@
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
index 710c8cf..e95c046 100644
--- 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 @@
}
/**
- * 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 @@
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
index 76521d1..ffdf08d 100644
--- 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 @@
@Test
public void testPreludeNeedNotBePresent() throws Exception {
- scratch.file("pkg/BUILD", "print('FOO')");
+ scratch.file(
+ "pkg/BUILD", //
+ "print('FOO')");
getConfiguredTarget("//pkg:BUILD");
assertContainsEvent("FOO");
