Rolling forward https://github.com/bazelbuild/bazel/commit/23f171e2208700dc2f2f65f5ccb32bacd7aec87d, but looking for PackageLookupValues, which are guaranteed to be present if a bzl file has been loaded, as opposed to PackageValues, which are not.
*** Original change description (substitute "package lookup" for "package" everywhere to update the description) ***
Avoid touching the filesystem to see what the BUILD file for a Starlark extension was: we already have the package, so we can just read it out.
Genquery remains buggy here.
haxorz@ observed in http://b/123795023#comment6 that we only need to read these paths to determine if the package was defined in a BUILD or BUILD.bazel file. Since the package already knows that, we just ask it.
***
PiperOrigin-RevId: 389960483
diff --git a/src/main/java/com/google/devtools/build/lib/events/StoredEventHandler.java b/src/main/java/com/google/devtools/build/lib/events/StoredEventHandler.java
index 008b893..6983afe 100644
--- a/src/main/java/com/google/devtools/build/lib/events/StoredEventHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/events/StoredEventHandler.java
@@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.events;
+import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
import java.util.ArrayList;
import java.util.List;
@@ -69,4 +70,13 @@
posts.clear();
hasErrors = false;
}
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this)
+ .add("events", events)
+ .add("posts", posts)
+ .add("hasErrors", hasErrors)
+ .toString();
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/CachingPackageLocator.java b/src/main/java/com/google/devtools/build/lib/packages/CachingPackageLocator.java
index d485394..6aa8a38 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/CachingPackageLocator.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/CachingPackageLocator.java
@@ -16,6 +16,7 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.vfs.Path;
+import javax.annotation.Nullable;
/**
* CachingPackageLocator implementations locate a package by its name.
@@ -28,19 +29,26 @@
* pieces of legacy code while still updating the Skyframe node graph.
*/
public interface CachingPackageLocator {
-
/**
* Returns path of BUILD file for specified package iff the specified package exists, null
* otherwise (e.g. invalid package name, no build file, or package has been deleted via
* --deleted_packages)..
*
- * <p> The package's root directory may be computed by calling getParentFile().
+ * <p>The package's root directory may be computed by calling getParentFile().
*
- * <p> Instances of this interface are required to cache the results.
+ * <p>Instances of this interface are required to cache the results.
*
- * <p> This method must be thread-safe.
+ * <p>This method must be thread-safe.
*/
@ThreadSafe
+ @Nullable
Path getBuildFileForPackage(PackageIdentifier packageName);
+ /**
+ * Returns base name of BUILD file for specified package (typically 'BUILD' or 'BUILD.bazel').
+ * {@code packageName} must be an already loaded package. Implementations that do not have full
+ * access to the set of loaded packages may return null here.
+ */
+ @Nullable
+ String getBaseNameForLoadedPackage(PackageIdentifier packageName);
}
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageManager.java b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageManager.java
index 1e6f3b5..0770fbe 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageManager.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageManager.java
@@ -48,9 +48,6 @@
int getPackagesLoaded();
}
- /** Retrieves a target pattern parser that works with this package manager. */
- TargetPatternPreloader newTargetPatternPreloader();
-
/** Retrieves a {@link QueryTransitivePackagePreloader} for preloading packages in query. */
QueryTransitivePackagePreloader transitiveLoader();
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/BlazeQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/query/BlazeQueryEnvironment.java
index 31aaae2..8fe671b 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/BlazeQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/BlazeQueryEnvironment.java
@@ -55,7 +55,6 @@
import com.google.devtools.build.lib.query2.engine.SkyframeRestartQueryException;
import com.google.devtools.build.lib.query2.engine.ThreadSafeOutputFormatterCallback;
import com.google.devtools.build.lib.query2.engine.Uniquifier;
-import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
import java.util.ArrayList;
@@ -438,14 +437,14 @@
// Also add the BUILD file of the extension.
if (buildFiles) {
- Path buildFileForLoad =
- cachingPackageLocator.getBuildFileForPackage(
+ // Can be null in genquery: see http://b/123795023#comment6.
+ String baseName =
+ cachingPackageLocator.getBaseNameForLoadedPackage(
loadTarget.getLabel().getLabel().getPackageIdentifier());
- if (buildFileForLoad != null) {
+ if (baseName != null) {
Label buildFileLabel =
Label.createUnvalidated(
- loadTarget.getLabel().getLabel().getPackageIdentifier(),
- buildFileForLoad.getBaseName());
+ loadTarget.getLabel().getLabel().getPackageIdentifier(), baseName);
addIfUniqueLabel(
getNode(new FakeLoadTarget(buildFileLabel, pkg)), seenLabels, dependentFiles);
}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java
index d531b6a..e5c218f 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java
@@ -45,7 +45,6 @@
import com.google.devtools.build.lib.query2.engine.QueryUtil.UniquifierImpl;
import com.google.devtools.build.lib.query2.engine.SkyframeRestartQueryException;
import com.google.devtools.build.lib.query2.engine.Uniquifier;
-import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
import java.util.Collection;
@@ -438,13 +437,13 @@
// Also add the BUILD file of the extension.
if (buildFiles) {
- Path buildFileForLoad =
- cachingPackageLocator.getBuildFileForPackage(
+ // Can be null in genquery: see http://b/123795023#comment6.
+ String baseName =
+ cachingPackageLocator.getBaseNameForLoadedPackage(
loadTarget.getLabel().getPackageIdentifier());
- if (buildFileForLoad != null) {
+ if (baseName != null) {
Label buildFileLabel =
- Label.createUnvalidated(
- loadTarget.getLabel().getPackageIdentifier(), buildFileForLoad.getBaseName());
+ Label.createUnvalidated(loadTarget.getLabel().getPackageIdentifier(), baseName);
dependentFiles.add(new FakeLoadTarget(buildFileLabel, pkg));
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
index 6db5c2b..fc0dff4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
@@ -619,6 +619,14 @@
}
return pkg.getBuildFile().getPath();
}
+
+ @Override
+ public String getBaseNameForLoadedPackage(PackageIdentifier packageName) {
+ // TODO(b/123795023): we should have the data here but we don't have all packages for Starlark
+ // loads present here.
+ Package pkg = pkgMap.get(packageName);
+ return pkg == null ? null : pkg.getBuildFileLabel().getName();
+ }
}
private static class BrokenQueryScopeException extends Exception {
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryEnvironmentBasedCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryEnvironmentBasedCommand.java
index b2e78c6..153dc89 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryEnvironmentBasedCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/QueryEnvironmentBasedCommand.java
@@ -46,6 +46,7 @@
import com.google.devtools.build.lib.skyframe.LoadingPhaseStartedEvent;
import com.google.devtools.build.lib.skyframe.PackageProgressReceiver;
import com.google.devtools.build.lib.skyframe.SkyframeExecutorWrappingWalkableGraph;
+import com.google.devtools.build.lib.skyframe.SkyframeTargetPatternEvaluator;
import com.google.devtools.build.lib.util.AbruptExitException;
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.util.Either;
@@ -263,7 +264,7 @@
env.getSkyframeExecutor(),
targetProviderForQueryEnvironment,
env.getPackageManager(),
- env.getPackageManager().newTargetPatternPreloader(),
+ new SkyframeTargetPatternEvaluator(env.getSkyframeExecutor()),
env.getRelativeWorkingDirectory(),
keepGoing,
/*strictScope=*/ true,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index 6a8e420..5f1ed00 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -920,7 +920,7 @@
}
@Override
- public void dumpPackages(PrintStream out) {
+ protected void dumpPackages(PrintStream out) {
Iterable<SkyKey> packageSkyKeys =
Iterables.filter(
memoizingEvaluator.getValues().keySet(),
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index ac53b50..bd8be60b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -440,8 +440,7 @@
new QueryTransitivePackagePreloader(this::getDriver),
syscalls,
pkgLocator,
- numPackagesLoaded,
- this);
+ numPackagesLoaded);
this.fileSystem = fileSystem;
this.directories = Preconditions.checkNotNull(directories);
this.actionKeyContext = Preconditions.checkNotNull(actionKeyContext);
@@ -729,7 +728,8 @@
memoizingEvaluator.dump(summarize, out);
}
- public abstract void dumpPackages(PrintStream out);
+ @ForOverride
+ protected abstract void dumpPackages(PrintStream out);
public void setOutputService(OutputService outputService) {
this.outputService = outputService;
@@ -2640,6 +2640,22 @@
!packageName.getRepository().isDefault(), "package must be absolute: %s", packageName);
return deletedPackages.get().contains(packageName);
}
+
+ PackageLookupValue getPackageLookupValue(PackageIdentifier pkgName) {
+ try {
+ return (PackageLookupValue)
+ memoizingEvaluator.getExistingValue(PackageLookupValue.key(pkgName));
+ } catch (InterruptedException e) {
+ throw new IllegalStateException(
+ String.format(
+ "Evaluator %s should not be interruptible (%s)", memoizingEvaluator, pkgName),
+ e);
+ }
+ }
+
+ void dumpPackages(PrintStream out) {
+ SkyframeExecutor.this.dumpPackages(out);
+ }
}
@VisibleForTesting
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
index c988797..dcf4ccc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
@@ -13,6 +13,9 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+
import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
@@ -26,7 +29,6 @@
import com.google.devtools.build.lib.pkgcache.PackageManager;
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
import com.google.devtools.build.lib.pkgcache.QueryTransitivePackagePreloader;
-import com.google.devtools.build.lib.pkgcache.TargetPatternPreloader;
import com.google.devtools.build.lib.skyframe.SkyframeExecutor.SkyframePackageLoader;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.UnixGlob;
@@ -40,21 +42,18 @@
private final AtomicReference<UnixGlob.FilesystemCalls> syscalls;
private final AtomicReference<PathPackageLocator> pkgLocator;
private final AtomicInteger numPackagesLoaded;
- private final SkyframeExecutor skyframeExecutor;
public SkyframePackageManager(
SkyframePackageLoader packageLoader,
QueryTransitivePackagePreloader transitiveLoader,
AtomicReference<UnixGlob.FilesystemCalls> syscalls,
AtomicReference<PathPackageLocator> pkgLocator,
- AtomicInteger numPackagesLoaded,
- SkyframeExecutor skyframeExecutor) {
+ AtomicInteger numPackagesLoaded) {
this.packageLoader = packageLoader;
this.transitiveLoader = transitiveLoader;
this.pkgLocator = pkgLocator;
this.syscalls = syscalls;
this.numPackagesLoaded = numPackagesLoaded;
- this.skyframeExecutor = skyframeExecutor;
}
@ThreadSafe
@@ -74,12 +73,7 @@
@Override
public PackageManagerStatistics getAndClearStatistics() {
int packagesLoaded = numPackagesLoaded.getAndSet(0);
- return new PackageManagerStatistics() {
- @Override
- public int getPackagesLoaded() {
- return packagesLoaded;
- }
- };
+ return () -> packagesLoaded;
}
@Override
@@ -89,7 +83,7 @@
@Override
public void dump(PrintStream printStream) {
- skyframeExecutor.dumpPackages(printStream);
+ packageLoader.dumpPackages(printStream);
}
@ThreadSafe
@@ -107,6 +101,18 @@
}
@Override
+ public String getBaseNameForLoadedPackage(PackageIdentifier packageName) {
+ PackageLookupValue pkgLookupValue =
+ checkNotNull(
+ packageLoader.getPackageLookupValue(packageName),
+ "Package should already have been visited: %s",
+ packageName);
+ checkState(
+ pkgLookupValue.packageExists(), "Package must exist: %s %s", packageName, pkgLookupValue);
+ return pkgLookupValue.getBuildFileName().getFilenameFragment().getBaseName();
+ }
+
+ @Override
public PathPackageLocator getPackagePath() {
return pkgLocator.get();
}
@@ -115,9 +121,4 @@
public QueryTransitivePackagePreloader transitiveLoader() {
return transitiveLoader;
}
-
- @Override
- public TargetPatternPreloader newTargetPatternPreloader() {
- return new SkyframeTargetPatternEvaluator(skyframeExecutor);
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
index da99603..fe79140 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeTargetPatternEvaluator.java
@@ -45,10 +45,10 @@
import javax.annotation.Nullable;
/** Skyframe-based target pattern parsing. */
-final class SkyframeTargetPatternEvaluator implements TargetPatternPreloader {
+public final class SkyframeTargetPatternEvaluator implements TargetPatternPreloader {
private final SkyframeExecutor skyframeExecutor;
- SkyframeTargetPatternEvaluator(SkyframeExecutor skyframeExecutor) {
+ public SkyframeTargetPatternEvaluator(SkyframeExecutor skyframeExecutor) {
this.skyframeExecutor = skyframeExecutor;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
index 2eb0449..11818be 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
@@ -460,6 +460,12 @@
public Path getBuildFileForPackage(PackageIdentifier packageName) {
return pkgLocatorRef.get().getPackageBuildFileNullable(packageName, syscallCacheRef);
}
+
+ @Override
+ public String getBaseNameForLoadedPackage(PackageIdentifier packageName) {
+ Path buildFileForPackage = getBuildFileForPackage(packageName);
+ return buildFileForPackage == null ? null : buildFileForPackage.getBaseName();
+ }
};
ImmutableMap.Builder<SkyFunctionName, SkyFunction> builder = ImmutableMap.builder();
builder
diff --git a/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java b/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java
index d90c55c..f1b693f 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java
@@ -110,6 +110,12 @@
return null;
}
}
+
+ @Override
+ public String getBaseNameForLoadedPackage(PackageIdentifier packageName) {
+ Path buildFileForPackage = getBuildFileForPackage(packageName);
+ return buildFileForPackage == null ? null : buildFileForPackage.getBaseName();
+ }
},
null,
TestUtils.getPool(),
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index 21141f1..1710588 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -1251,6 +1251,11 @@
public Path getBuildFileForPackage(PackageIdentifier packageName) {
return null;
}
+
+ @Override
+ public String getBaseNameForLoadedPackage(PackageIdentifier packageName) {
+ return null;
+ }
},
null,
TestUtils.getPool(),
diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java
index bdaa988..3ed758a 100644
--- a/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java
@@ -1798,6 +1798,15 @@
}
@Test
+ public void buildfilesBazel() throws Exception {
+ writeFile("bar/BUILD.bazel");
+ writeFile("bar/bar.bzl", "sym = 0");
+ writeFile("foo/BUILD.bazel", "load('//bar:bar.bzl', 'sym')");
+ assertThat(evalToListOfStrings("buildfiles(foo:*)"))
+ .containsExactly("//foo:BUILD.bazel", "//bar:bar.bzl", "//bar:BUILD.bazel");
+ }
+
+ @Test
public void testTargetsFromBuildfilesAndRealTargets() throws Exception {
writeFile(
"foo/BUILD", "load('//baz:baz.bzl', 'x')", "sh_library(name = 'foo', deps = ['//baz'])");
diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java
index 24c22b5..7951cb7 100644
--- a/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java
@@ -730,6 +730,9 @@
public void bzlPackageBadDueToBrokenSyntax() {}
@Override
+ public void buildfilesBazel() {}
+
+ @Override
public void testTargetsFromBuildfilesAndRealTargets() {}
// siblings() operator.
diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java
index 4bd53a2..c179864 100644
--- a/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java
+++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java
@@ -55,6 +55,7 @@
import com.google.devtools.build.lib.skyframe.PackageValue;
import com.google.devtools.build.lib.skyframe.PrecomputedValue;
import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
+import com.google.devtools.build.lib.skyframe.SkyframeTargetPatternEvaluator;
import com.google.devtools.build.lib.testutil.SkyframeExecutorTestHelper;
import com.google.devtools.build.lib.testutil.TestPackageFactoryBuilderFactory;
import com.google.devtools.build.lib.util.AbruptExitException;
@@ -303,7 +304,7 @@
throw new IllegalStateException(e);
}
pkgManager = skyframeExecutor.getPackageManager();
- targetParser = pkgManager.newTargetPatternPreloader();
+ targetParser = new SkyframeTargetPatternEvaluator(skyframeExecutor);
}
@Override
