Make globs work in remote repositories.
This involved quite a few changes, mainly changing a bunch of places where we refer to packages by a PathFragment to PackageIdentifier.
The only wart is the code in PathPackageLocator: ideally, it would just call into PackageLookupFunction. Unfortunately, it is (through globbing and Parser.include) called from within a Skyframe function, and we don't want to have two eval() calls going on at the same time, so we cannot use that.
There is a potential correctness issue there: PathPackageLocator now assumes where external repositories are put and assumes that they are there when it gets control, but my understanding is that the associated RepositoryValue is always evaluated before, so it works out okay.
--
MOS_MIGRATED_REVID=97751539
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
index 9ea6210..ac5b6a2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
@@ -64,9 +64,9 @@
}
@Override
- public boolean isPackage(EventHandler eventHandler, String packageName)
+ public boolean isPackage(EventHandler eventHandler, PackageIdentifier packageId)
throws MissingDepException {
- SkyKey packageLookupKey = PackageLookupValue.key(new PathFragment(packageName));
+ SkyKey packageLookupKey = PackageLookupValue.key(packageId);
try {
PackageLookupValue packageLookupValue =
(PackageLookupValue) env.getValueOrThrow(packageLookupKey, NoSuchPackageException.class,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
index d1ee81c..942a201 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
@@ -18,6 +18,7 @@
import com.google.common.cache.CacheBuilder;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.packages.PackageIdentifier;
import com.google.devtools.build.lib.vfs.Dirent;
import com.google.devtools.build.lib.vfs.Dirent.Type;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -60,8 +61,9 @@
PathFragment globSubdir = glob.getSubdir();
if (!globSubdir.equals(PathFragment.EMPTY_FRAGMENT)) {
PackageLookupValue globSubdirPkgLookupValue = (PackageLookupValue) env.getValue(
- PackageLookupValue.key(glob.getPackageId().getPackageFragment()
- .getRelative(globSubdir)));
+ PackageLookupValue.key(new PackageIdentifier(
+ glob.getPackageId().getRepository(),
+ glob.getPackageId().getPackageFragment().getRelative(globSubdir))));
if (globSubdirPkgLookupValue == null) {
return null;
}
@@ -220,7 +222,8 @@
PathFragment directory = glob.getPackageId().getPackageFragment()
.getRelative(glob.getSubdir()).getRelative(fileName);
PackageLookupValue pkgLookupValue = (PackageLookupValue)
- env.getValue(PackageLookupValue.key(directory));
+ env.getValue(PackageLookupValue.key(new PackageIdentifier(
+ glob.getPackageId().getRepository(), directory)));
if (pkgLookupValue == null) {
return;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
index c4e8c49..c029a1d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
@@ -79,8 +79,8 @@
}
@Override
- public boolean isPackage(EventHandler eventHandler, String packageName) {
- SkyKey packageLookupKey = PackageLookupValue.key(new PathFragment(packageName));
+ public boolean isPackage(EventHandler eventHandler, PackageIdentifier packageName) {
+ SkyKey packageLookupKey = PackageLookupValue.key(packageName);
if (!graph.exists(packageLookupKey)) {
// If the package lookup key does not exist in the graph, then it must not correspond to any
// package, because the SkyQuery environment has already loaded the universe.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
index db3e7c1..2c8cc8e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
@@ -41,9 +41,9 @@
*/
class PackageLookupFunction implements SkyFunction {
- private final AtomicReference<ImmutableSet<String>> deletedPackages;
+ private final AtomicReference<ImmutableSet<PackageIdentifier>> deletedPackages;
- PackageLookupFunction(AtomicReference<ImmutableSet<String>> deletedPackages) {
+ PackageLookupFunction(AtomicReference<ImmutableSet<PackageIdentifier>> deletedPackages) {
this.deletedPackages = deletedPackages;
}
@@ -64,7 +64,7 @@
+ packageNameErrorMsg);
}
- if (deletedPackages.get().contains(packageKey.getPackageFragment().toString())) {
+ if (deletedPackages.get().contains(packageKey)) {
return PackageLookupValue.deletedPackage();
}
@@ -142,7 +142,7 @@
throws PackageLookupFunctionException {
PackageIdentifier id = (PackageIdentifier) skyKey.argument();
SkyKey repositoryKey = RepositoryValue.key(id.getRepository());
- RepositoryValue repositoryValue = null;
+ RepositoryValue repositoryValue;
try {
repositoryValue = (RepositoryValue) env.getValueOrThrow(
repositoryKey, NoSuchPackageException.class, IOException.class, EvalException.class);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
index 5230d09..6fbfe57 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
@@ -193,7 +193,10 @@
@Override
public boolean isPackage(String packageName) {
- return packageProvider.isPackage(env.getListener(), packageName);
+ // TODO(bazel-team): this should get the whole PackageIdentifier. Using only the package name
+ // makes it impossible to use wildcards to refer to targets in remote repositories.
+ return packageProvider.isPackage(env.getListener(),
+ PackageIdentifier.createInDefaultRepo(packageName));
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
index 942ed83..b913628 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
@@ -132,7 +132,10 @@
@Override
public boolean isPackage(String packageName) {
- return recursivePackageProvider.isPackage(eventHandler, packageName);
+ // TODO(bazel-team): this should get the whole PackageIdentifier. Using only the package name
+ // makes it impossible to use the //... wildcard to refer to targets in remote repositories.
+ return recursivePackageProvider.isPackage(
+ eventHandler, PackageIdentifier.createInDefaultRepo(packageName));
}
@Override
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 ef22ab9..a1ac8e2 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
@@ -211,11 +211,12 @@
}
@Override
- public void sync(PackageCacheOptions packageCacheOptions, Path workingDirectory,
+ public void sync(PackageCacheOptions packageCacheOptions, Path outputBase, Path workingDirectory,
String defaultsPackageContents, UUID commandId)
throws InterruptedException, AbruptExitException {
this.valueCacheEvictionLimit = packageCacheOptions.minLoadedPkgCountForCtNodeEviction;
- super.sync(packageCacheOptions, workingDirectory, defaultsPackageContents, commandId);
+ super.sync(
+ packageCacheOptions, outputBase, workingDirectory, defaultsPackageContents, commandId);
handleDiffs();
}
@@ -243,11 +244,10 @@
recordingDiffer.invalidate(Iterables.filter(memoizingEvaluator.getValues().keySet(), pred));
}
- private void invalidateDeletedPackages(Iterable<String> deletedPackages) {
+ private void invalidateDeletedPackages(Iterable<PackageIdentifier> deletedPackages) {
ArrayList<SkyKey> packagesToInvalidate = Lists.newArrayList();
- for (String deletedPackage : deletedPackages) {
- PathFragment pathFragment = new PathFragment(deletedPackage);
- packagesToInvalidate.add(PackageLookupValue.key(pathFragment));
+ for (PackageIdentifier deletedPackage : deletedPackages) {
+ packagesToInvalidate.add(PackageLookupValue.key(deletedPackage));
}
recordingDiffer.invalidate(packagesToInvalidate);
}
@@ -257,7 +257,7 @@
*/
@Override
@VisibleForTesting // productionVisibility = Visibility.PRIVATE
- public void setDeletedPackages(Iterable<String> pkgs) {
+ public void setDeletedPackages(Iterable<PackageIdentifier> pkgs) {
// Invalidate the old deletedPackages as they may exist now.
invalidateDeletedPackages(deletedPackages.get());
deletedPackages.set(ImmutableSet.copyOf(pkgs));
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 96c39ef..601d7ec 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
@@ -180,8 +180,8 @@
new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS);
protected final AtomicReference<PathPackageLocator> pkgLocator =
new AtomicReference<>();
- protected final AtomicReference<ImmutableSet<String>> deletedPackages =
- new AtomicReference<>(ImmutableSet.<String>of());
+ protected final AtomicReference<ImmutableSet<PackageIdentifier>> deletedPackages =
+ new AtomicReference<>(ImmutableSet.<PackageIdentifier>of());
private final AtomicReference<EventBus> eventBus = new AtomicReference<>();
private final ImmutableList<BuildInfoFactory> buildInfoFactories;
@@ -753,7 +753,7 @@
* Sets the packages that should be treated as deleted and ignored.
*/
@VisibleForTesting // productionVisibility = Visibility.PRIVATE
- public abstract void setDeletedPackages(Iterable<String> pkgs);
+ public abstract void setDeletedPackages(Iterable<PackageIdentifier> pkgs);
/**
* Prepares the evaluator for loading.
@@ -1299,7 +1299,7 @@
/**
* Returns whether the given package should be consider deleted and thus should be ignored.
*/
- public boolean isPackageDeleted(String packageName) {
+ public boolean isPackageDeleted(PackageIdentifier packageName) {
return deletedPackages.get().contains(packageName);
}
@@ -1349,12 +1349,13 @@
@ThreadCompatible
public abstract void updateLoadedPackageSet(Set<PackageIdentifier> loadedPackages);
- public void sync(PackageCacheOptions packageCacheOptions, Path workingDirectory,
+ public void sync(PackageCacheOptions packageCacheOptions, Path outputBase, Path workingDirectory,
String defaultsPackageContents, UUID commandId) throws InterruptedException,
AbruptExitException{
preparePackageLoading(
- createPackageLocator(packageCacheOptions, directories.getWorkspace(), workingDirectory),
+ createPackageLocator(
+ packageCacheOptions, outputBase, directories.getWorkspace(), workingDirectory),
packageCacheOptions.defaultVisibility, packageCacheOptions.showLoadingProgress,
packageCacheOptions.globbingThreads, defaultsPackageContents, commandId);
setDeletedPackages(ImmutableSet.copyOf(packageCacheOptions.deletedPackages));
@@ -1364,9 +1365,9 @@
}
protected PathPackageLocator createPackageLocator(PackageCacheOptions packageCacheOptions,
- Path workspace, Path workingDirectory) throws AbruptExitException{
+ Path outputBase, Path workspace, Path workingDirectory) throws AbruptExitException {
return PathPackageLocator.create(
- packageCacheOptions.packagePath, getReporter(), workspace, workingDirectory);
+ outputBase, packageCacheOptions.packagePath, getReporter(), workspace, workingDirectory);
}
private CyclesReporter createCyclesReporter() {
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 3e9b6ca..7ffb79ce 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,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import com.google.devtools.build.lib.cmdline.LabelValidator;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
@@ -136,7 +135,7 @@
}
@Override
- public boolean isPackage(EventHandler eventHandler, String packageName) {
+ public boolean isPackage(EventHandler eventHandler, PackageIdentifier packageName) {
return getBuildFileForPackage(packageName) != null;
}
@@ -147,11 +146,10 @@
@ThreadSafe
@Override
- public Path getBuildFileForPackage(String packageName) {
+ public Path getBuildFileForPackage(PackageIdentifier packageName) {
// Note that this method needs to be thread-safe, as it is currently used concurrently by
// legacy blaze code.
- if (packageLoader.isPackageDeleted(packageName)
- || LabelValidator.validatePackageName(packageName) != null) {
+ if (packageLoader.isPackageDeleted(packageName)) {
return null;
}
// TODO(bazel-team): Use a PackageLookupValue here [skyframe-loading]
