From 4344a0358f44c0290f85f8d90dede5824593ce77 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 1 Jun 2023 19:34:25 -0700 Subject: [PATCH] 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 --- .../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, 101 insertions(+), 142 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 1b30aa272503e5..876d56c6f5adb4 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 @@ 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 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 index acfc8178f1e5d6..444c15bb988abe 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 @@ 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 index 08d73e41e093da..30d84e8692a289 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.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 710c8cfebfdaa6..e95c0460b9506f 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} if it contains a subpackage - * crossing boundary. + * Creates the error message for the input {@linkplain Label label} has a subpackage crossing + * boundary. * *

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