From 00a4fefe594069d47d1bde99b28c6b8dcca0a7c1 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 24 May 2023 14:49:53 -0700 Subject: [PATCH] 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: 534989868 Change-Id: Ic06ffbea8b13ad5eff4001049791c9a512e0211d --- .../build/lib/skyframe/BzlLoadFunction.java | 146 +++++++++++++----- .../ContainingPackageLookupValue.java | 61 +++----- .../build/lib/skyframe/PackageFunction.java | 2 +- .../lib/skyframe/PackageLookupValue.java | 30 ++-- .../lib/skyframe/PackageFunctionTest.java | 4 +- 5 files changed, 142 insertions(+), 101 deletions(-) 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 876d56c6f5adb4..1b30aa272503e5 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,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 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 index 444c15bb988abe..acfc8178f1e5d6 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,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 index 30d84e8692a289..08d73e41e093da 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 @@ 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 index e95c0460b9506f..710c8cfebfdaa6 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 @@ 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. * *

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 index ffdf08d63c24ae..76521d113da1a9 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,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");