diff options
28 files changed, 182 insertions, 88 deletions
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 f6badad9dd..3d677990f3 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 @@ -40,6 +40,6 @@ public interface CachingPackageLocator { * <p> This method must be thread-safe. */ @ThreadSafe - Path getBuildFileForPackage(String packageName); + Path getBuildFileForPackage(PackageIdentifier packageName); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java b/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java index de9bfdfebc..43669a86bf 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java +++ b/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java @@ -23,7 +23,6 @@ import com.google.common.util.concurrent.SettableFuture; import com.google.devtools.build.lib.concurrent.ThreadSafety; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Symlinks; import com.google.devtools.build.lib.vfs.UnixGlob; @@ -107,7 +106,6 @@ public class GlobCache { this.syscalls = syscalls == null ? new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS) : syscalls; Preconditions.checkNotNull(locator); - final PathFragment pkgNameFrag = packageId.getPackageFragment(); childDirectoryPredicate = new Predicate<Path>() { @Override public boolean apply(Path directory) { @@ -115,7 +113,9 @@ public class GlobCache { return true; } - PathFragment pkgName = pkgNameFrag.getRelative(directory.relativeTo(packageDirectory)); + PackageIdentifier subPackageId = new PackageIdentifier( + packageId.getRepository(), + packageId.getPackageFragment().getRelative(directory.relativeTo(packageDirectory))); UnixGlob.FilesystemCalls syscalls = GlobCache.this.syscalls.get(); if (syscalls != UnixGlob.DEFAULT_SYSCALLS) { // This is needed because in case the BUILD file exists, we do not call readdir() on its @@ -134,7 +134,7 @@ public class GlobCache { syscalls.statNullable(directory.getChild("BUILD"), Symlinks.FOLLOW); } - return locator.getBuildFileForPackage(pkgName.getPathString()) == null; + return locator.getBuildFileForPackage(subPackageId) == null; } }; } diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageIdentifier.java b/src/main/java/com/google/devtools/build/lib/packages/PackageIdentifier.java index f2f5b8e0e0..803fb48855 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageIdentifier.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageIdentifier.java @@ -16,6 +16,7 @@ package com.google.devtools.build.lib.packages; import com.google.common.base.Preconditions; import com.google.common.collect.ComparisonChain; +import com.google.devtools.build.lib.cmdline.LabelValidator; import com.google.devtools.build.lib.syntax.Label.SyntaxException; import com.google.devtools.build.lib.util.StringCanonicalizer; import com.google.devtools.build.lib.util.StringUtilities; @@ -217,6 +218,34 @@ public final class PackageIdentifier implements Comparable<PackageIdentifier>, S this.pkgName = Canonicalizer.fragments().intern(pkgName.normalize()); } + public static PackageIdentifier parse(String input) throws SyntaxException { + String repo; + String packageName; + int packageStartPos = input.indexOf("//"); + if (packageStartPos > 0) { + repo = input.substring(0, packageStartPos); + packageName = input.substring(packageStartPos + 2); + } else if (packageStartPos == 0) { + repo = PackageIdentifier.DEFAULT_REPOSITORY; + packageName = input.substring(2); + } else { + repo = PackageIdentifier.DEFAULT_REPOSITORY; + packageName = input; + } + + String error = RepositoryName.validate(repo); + if (error != null) { + throw new SyntaxException(error); + } + + error = LabelValidator.validatePackageName(packageName); + if (error != null) { + throw new SyntaxException(error); + } + + return new PackageIdentifier(repo, new PathFragment(packageName)); + } + private Object writeReplace() throws ObjectStreamException { return new SerializationProxy(this); } diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageCacheOptions.java b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageCacheOptions.java index e57fe0df61..daba8f1e88 100644 --- a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageCacheOptions.java +++ b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageCacheOptions.java @@ -16,6 +16,7 @@ package com.google.devtools.build.lib.pkgcache; import com.google.devtools.build.lib.Constants; import com.google.devtools.build.lib.packages.ConstantRuleVisibility; +import com.google.devtools.build.lib.packages.PackageIdentifier; import com.google.devtools.build.lib.packages.RuleVisibility; import com.google.devtools.build.lib.syntax.CommaSeparatedPackageNameListConverter; import com.google.devtools.common.options.Converter; @@ -110,7 +111,7 @@ public class PackageCacheOptions extends OptionsBase { + "encounters a label '//x:y/z' if that is still provided by another " + "package_path entry. Specifying --deleted_packages x/y avoids this " + "problem.") - public List<String> deletedPackages; + public List<PackageIdentifier> deletedPackages; @Option(name = "default_visibility", defaultValue = "private", diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageProvider.java b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageProvider.java index 5363cb5a8a..9bce541a51 100644 --- a/src/main/java/com/google/devtools/build/lib/pkgcache/PackageProvider.java +++ b/src/main/java/com/google/devtools/build/lib/pkgcache/PackageProvider.java @@ -59,5 +59,5 @@ public interface PackageProvider extends TargetProvider { * @param eventHandler the eventHandler on which to report warnings and errors * @param packageName the name of the package. */ - boolean isPackage(EventHandler eventHandler, String packageName); + boolean isPackage(EventHandler eventHandler, PackageIdentifier packageName); } diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java b/src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java index c8204c6a26..0c8b174802 100644 --- a/src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java +++ b/src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java @@ -13,6 +13,9 @@ // limitations under the License. package com.google.devtools.build.lib.pkgcache; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Objects; +import com.google.common.base.Verify; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.events.Event; @@ -45,33 +48,29 @@ public class PathPackageLocator implements Serializable { public static final Set<String> DEFAULT_TOP_LEVEL_EXCLUDES = ImmutableSet.of("experimental"); - /** - * An interface which accepts {@link PathFragment}s. - */ - public interface AcceptsPathFragment { - - /** - * Accept a {@link PathFragment}. - * - * @param fragment The path fragment. - */ - void accept(PathFragment fragment); - } - private final ImmutableList<Path> pathEntries; + // Transient because this is an injected value in Skyframe, and as such, its serialized + // representation is used as a key. We want a change to output base not to invalidate things. + private final transient Path outputBase; + + private PathPackageLocator(Path outputBase, List<Path> pathEntries) { + this.outputBase = outputBase; + this.pathEntries = ImmutableList.copyOf(pathEntries); + } /** * Constructs a PathPackageLocator based on the specified list of package root directories. */ + @VisibleForTesting public PathPackageLocator(List<Path> pathEntries) { - this.pathEntries = ImmutableList.copyOf(pathEntries); + this(null, pathEntries); } /** * Constructs a PathPackageLocator based on the specified array of package root directories. */ public PathPackageLocator(Path... pathEntries) { - this(Arrays.asList(pathEntries)); + this(null, Arrays.asList(pathEntries)); } /** @@ -87,11 +86,10 @@ public class PathPackageLocator implements Serializable { * <p>If the same package exists beneath multiple package path entries, the * first path that matches always wins. */ - public Path getPackageBuildFile(String packageName) throws NoSuchPackageException { + public Path getPackageBuildFile(PackageIdentifier packageName) throws NoSuchPackageException { Path buildFile = getPackageBuildFileNullable(packageName, UnixGlob.DEFAULT_SYSCALLS_REF); if (buildFile == null) { - throw new BuildFileNotFoundException(PackageIdentifier.createInDefaultRepo(packageName), - "BUILD file not found on package path"); + throw new BuildFileNotFoundException(packageName, "BUILD file not found on package path"); } return buildFile; } @@ -102,9 +100,28 @@ public class PathPackageLocator implements Serializable { * @param packageName the name of the package. * @param cache a filesystem-level cache of stat() calls. */ - public Path getPackageBuildFileNullable(String packageName, + public Path getPackageBuildFileNullable(PackageIdentifier packageName, AtomicReference<? extends UnixGlob.FilesystemCalls> cache) { - return getFilePath(new PathFragment(packageName).getRelative("BUILD"), cache); + if (packageName.getRepository().isDefault()) { + return getFilePath(packageName.getPackageFragment().getRelative("BUILD"), cache); + } else if (!packageName.getRepository().isDefault()) { + Verify.verify(outputBase != null, String.format( + "External package '%s' needs to be loaded but this PathPackageLocator instance does not " + + "support external packages", packageName)); + // This works only to some degree, because it relies on the presence of the repository under + // $OUTPUT_BASE/external, which is created by the appropriate RepositoryValue. This is true + // for the invocation in GlobCache, but not for the locator.getBuildFileForPackage() + // invocation in Parser#include(). + Path buildFile = outputBase.getRelative(packageName.getPathFragment()).getRelative("BUILD"); + FileStatus stat = cache.get().statNullable(buildFile, Symlinks.FOLLOW); + if (stat != null && stat.isFile()) { + return buildFile; + } else { + return null; + } + } else { + return null; + } } @@ -124,6 +141,7 @@ public class PathPackageLocator implements Serializable { * A factory of PathPackageLocators from a list of path elements. Elements * may contain "%workspace%", indicating the workspace. * + * @param outputBase the output base. Can be null if remote repositories are not in use. * @param pathElements Each element must be an absolute path, relative path, * or some string "%workspace%" + relative, where relative is itself a * relative path. The special symbol "%workspace%" means to interpret @@ -135,7 +153,8 @@ public class PathPackageLocator implements Serializable { * @param clientWorkingDirectory The client's working directory. * @return a list of {@link Path}s. */ - public static PathPackageLocator create(List<String> pathElements, + public static PathPackageLocator create(Path outputBase, + List<String> pathElements, EventHandler eventHandler, Path workspace, Path clientWorkingDirectory) { @@ -164,7 +183,7 @@ public class PathPackageLocator implements Serializable { resolvedPaths.add(rootPath); } } - return new PathPackageLocator(resolvedPaths); + return new PathPackageLocator(outputBase, resolvedPaths); } /** @@ -194,7 +213,7 @@ public class PathPackageLocator implements Serializable { @Override public int hashCode() { - return pathEntries.hashCode(); + return Objects.hashCode(pathEntries, outputBase); } @Override @@ -205,6 +224,7 @@ public class PathPackageLocator implements Serializable { if (!(other instanceof PathPackageLocator)) { return false; } - return this.getPathEntries().equals(((PathPackageLocator) other).getPathEntries()); + return this.getPathEntries().equals(((PathPackageLocator) other).getPathEntries()) + && Objects.equal(this.outputBase, ((PathPackageLocator) other).outputBase); } } 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 2aae12f91a..bbc432e7a5 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 @@ -461,7 +461,7 @@ public class GenQuery implements RuleConfiguredTargetFactory { } @Override - public boolean isPackage(EventHandler eventHandler, String packageName) { + public boolean isPackage(EventHandler eventHandler, PackageIdentifier packageName) { throw new UnsupportedOperationException(); } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java index fca2ae918f..fcd11e5e46 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java @@ -1014,7 +1014,7 @@ public final class BlazeRuntime { if (!skyframeExecutor.hasIncrementalState()) { clearSkyframeRelevantCaches(); } - skyframeExecutor.sync(packageCacheOptions, getWorkingDirectory(), + skyframeExecutor.sync(packageCacheOptions, getOutputBase(), getWorkingDirectory(), defaultsPackageContents, getCommandId()); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/ProjectFileSupport.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/ProjectFileSupport.java index bec0deb5d1..133179f07d 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/ProjectFileSupport.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/ProjectFileSupport.java @@ -63,8 +63,11 @@ public final class ProjectFileSupport { // relative to the cwd instead. PathFragment projectFilePath = new PathFragment(targets.get(0).substring(1)); List<Path> packagePath = PathPackageLocator.create( - optionsParser.getOptions(PackageCacheOptions.class).packagePath, runtime.getReporter(), - runtime.getWorkspace(), runtime.getWorkingDirectory()).getPathEntries(); + runtime.getOutputBase(), + optionsParser.getOptions(PackageCacheOptions.class).packagePath, + runtime.getReporter(), + runtime.getWorkspace(), + runtime.getWorkingDirectory()).getPathEntries(); ProjectFile projectFile = projectFileProvider.getProjectFile(packagePath, projectFilePath); runtime.getReporter().handle(Event.info("Using " + projectFile.getName())); 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 9ea62103fe..ac5b6a2f1f 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 @@ public final class EnvironmentBackedRecursivePackageProvider implements Recursiv } @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 d1ee81cb07..942a201770 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.Cache; 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 @@ final class GlobFunction implements SkyFunction { 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 @@ final class GlobFunction implements SkyFunction { 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 c4e8c490af..c029a1ddc1 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 @@ public final class GraphBackedRecursivePackageProvider implements RecursivePacka } @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 db3e7c18bb..2c8cc8e83c 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 @@ import javax.annotation.Nullable; */ 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 @@ class PackageLookupFunction implements SkyFunction { + packageNameErrorMsg); } - if (deletedPackages.get().contains(packageKey.getPackageFragment().toString())) { + if (deletedPackages.get().contains(packageKey)) { return PackageLookupValue.deletedPackage(); } @@ -142,7 +142,7 @@ class PackageLookupFunction implements SkyFunction { 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 5230d09779..6fbfe574b1 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 @@ public class PrepareDepsOfPatternFunction implements SkyFunction { @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 942ed83fcb..b91362834e 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 @@ public class RecursivePackageProviderBackedTargetPatternResolver @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 ef22ab993b..a1ac8e2116 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 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { } @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 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { 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 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { */ @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 96c39ef9f5..601d7ec6bd 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { * 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { /** * 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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { @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 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { } 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 3e9b6cad1f..7ffb79ce89 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 @@ class SkyframePackageManager implements PackageManager { } @Override - public boolean isPackage(EventHandler eventHandler, String packageName) { + public boolean isPackage(EventHandler eventHandler, PackageIdentifier packageName) { return getBuildFileForPackage(packageName) != null; } @@ -147,11 +146,10 @@ class SkyframePackageManager implements PackageManager { @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] diff --git a/src/main/java/com/google/devtools/build/lib/syntax/CommaSeparatedPackageNameListConverter.java b/src/main/java/com/google/devtools/build/lib/syntax/CommaSeparatedPackageNameListConverter.java index 070e928eb9..ce7680ce3e 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/CommaSeparatedPackageNameListConverter.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/CommaSeparatedPackageNameListConverter.java @@ -16,7 +16,7 @@ package com.google.devtools.build.lib.syntax; import com.google.common.base.Splitter; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.cmdline.LabelValidator; +import com.google.devtools.build.lib.packages.PackageIdentifier; import com.google.devtools.common.options.Converter; import com.google.devtools.common.options.OptionsParsingException; @@ -26,22 +26,22 @@ import java.util.List; * A converter from strings containing comma-separated names of packages to lists of strings. */ public class CommaSeparatedPackageNameListConverter - implements Converter<List<String>> { + implements Converter<List<PackageIdentifier>> { private static final Splitter SPACE_SPLITTER = Splitter.on(','); @Override - public List<String> convert(String input) throws OptionsParsingException { + public List<PackageIdentifier> convert(String input) throws OptionsParsingException { if (Strings.isNullOrEmpty(input)) { return ImmutableList.of(); } - ImmutableList.Builder<String> list = ImmutableList.builder(); + ImmutableList.Builder<PackageIdentifier> list = ImmutableList.builder(); for (String s : SPACE_SPLITTER.split(input)) { - String errorMessage = LabelValidator.validatePackageName(s); - if (errorMessage != null) { - throw new OptionsParsingException(errorMessage); + try { + list.add(PackageIdentifier.parse(s)); + } catch (Label.SyntaxException e) { + throw new OptionsParsingException(e.getMessage()); } - list.add(s); } return list.build(); } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvaluationContext.java b/src/main/java/com/google/devtools/build/lib/syntax/EvaluationContext.java index 4966fb8717..f91222c740 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/EvaluationContext.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/EvaluationContext.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.events.EventKind; import com.google.devtools.build.lib.packages.CachingPackageLocator; +import com.google.devtools.build.lib.packages.PackageIdentifier; import com.google.devtools.build.lib.syntax.Environment.NoSuchVariableException; import com.google.devtools.build.lib.vfs.Path; @@ -90,7 +91,7 @@ public final class EvaluationContext { /** Mock package locator */ private static final class EmptyPackageLocator implements CachingPackageLocator { @Override - public Path getBuildFileForPackage(String packageName) { + public Path getBuildFileForPackage(PackageIdentifier packageName) { return null; } } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java index 2a9d103beb..f2f7c2c546 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java @@ -612,10 +612,13 @@ class Parser { try { Label label = Label.parseAbsolute(labelName); - String packageName = label.getPackageFragment().getPathString(); - Path packagePath = locator.getBuildFileForPackage(packageName); + // Note that this doesn't really work if the label belongs to a different repository, because + // there is no guarantee that its RepositoryValue has been evaluated. In an ideal world, we + // could put a Skyframe dependency the appropriate PackageLookupValue, but we can't do that + // because package loading is not completely Skyframized. + Path packagePath = locator.getBuildFileForPackage(label.getPackageIdentifier()); if (packagePath == null) { - reportError(location, "Package '" + packageName + "' not found"); + reportError(location, "Package '" + label.getPackageIdentifier() + "' not found"); list.add(mocksubincludeExpression(labelName, "", location)); return; } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java index e5bfd83e42..94975bb5fe 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java @@ -242,7 +242,7 @@ public abstract class AnalysisTestCase extends FoundationTestCase { PackageCacheOptions packageCacheOptions = optionsParser.getOptions(PackageCacheOptions.class); PathPackageLocator pathPackageLocator = PathPackageLocator.create( - packageCacheOptions.packagePath, reporter, rootDirectory, rootDirectory); + null, packageCacheOptions.packagePath, reporter, rootDirectory, rootDirectory); skyframeExecutor.preparePackageLoading(pathPackageLocator, packageCacheOptions.defaultVisibility, true, 7, ruleClassProvider.getDefaultsPackageContent(), UUID.randomUUID()); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index e6fa83308c..283f231ae1 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -274,7 +274,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { private void setUpSkyframe() { PathPackageLocator pkgLocator = PathPackageLocator.create( - packageCacheOptions.packagePath, reporter, rootDirectory, rootDirectory); + null, packageCacheOptions.packagePath, reporter, rootDirectory, rootDirectory); skyframeExecutor.preparePackageLoading(pkgLocator, packageCacheOptions.defaultVisibility, true, 7, ruleClassProvider.getDefaultsPackageContent(optionsParser), 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 bd8d53689c..392c750184 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 @@ -94,7 +94,8 @@ public class GlobCacheTest { cache = new GlobCache(packageDirectory, PackageIdentifier.createInDefaultRepo("isolated"), new CachingPackageLocator() { @Override - public Path getBuildFileForPackage(String packageName) { + public Path getBuildFileForPackage(PackageIdentifier packageId) { + String packageName = packageId.getPackageFragment().getPathString(); if (packageName.equals("isolated")) { return scratch.resolve("isolated/BUILD"); } else if (packageName.equals("isolated/sub")) { diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java index bc641ddc73..e769e389ca 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java +++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java @@ -160,7 +160,7 @@ public class PackageFactoryApparatus { public static CachingPackageLocator createEmptyLocator() { return new CachingPackageLocator() { @Override - public Path getBuildFileForPackage(String packageName) { + public Path getBuildFileForPackage(PackageIdentifier packageName) { return null; } }; diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java index e789d5171d..133e76dc87 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java @@ -94,7 +94,7 @@ public abstract class PackageLoadingTestCase extends FoundationTestCase { private void setUpSkyframe(PackageCacheOptions packageCacheOptions) { PathPackageLocator pkgLocator = PathPackageLocator.create( - packageCacheOptions.packagePath, reporter, rootDirectory, rootDirectory); + null, packageCacheOptions.packagePath, reporter, rootDirectory, rootDirectory); skyframeExecutor.preparePackageLoading(pkgLocator, packageCacheOptions.defaultVisibility, true, 7, ruleClassProvider.getDefaultsPackageContent(), diff --git a/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java b/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java index 84562358e4..a1b108e02b 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.events.EventKind; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.events.util.EventCollectionApparatus; import com.google.devtools.build.lib.packages.CachingPackageLocator; +import com.google.devtools.build.lib.packages.PackageIdentifier; import com.google.devtools.build.lib.testutil.JunitTestUtils; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.vfs.Path; @@ -45,8 +46,8 @@ public class BuildFileASTTest { private class ScratchPathPackageLocator implements CachingPackageLocator { @Override - public Path getBuildFileForPackage(String packageName) { - return scratch.resolve(packageName).getRelative("BUILD"); + public Path getBuildFileForPackage(PackageIdentifier packageName) { + return scratch.resolve(packageName.getPackageFragment()).getRelative("BUILD"); } } @@ -264,7 +265,7 @@ public class BuildFileASTTest { private class EmptyPackageLocator implements CachingPackageLocator { @Override - public Path getBuildFileForPackage(String packageName) { + public Path getBuildFileForPackage(PackageIdentifier packageName) { return null; } } diff --git a/src/test/shell/bazel/local_repository_test.sh b/src/test/shell/bazel/local_repository_test.sh index 682245b430..8a83a927b3 100755 --- a/src/test/shell/bazel/local_repository_test.sh +++ b/src/test/shell/bazel/local_repository_test.sh @@ -483,6 +483,7 @@ EOF function test_local_deps() { local r=$TEST_TMPDIR/r + rm -fr $r mkdir -p $r cat > WORKSPACE <<EOF local_repository( @@ -516,4 +517,30 @@ EOF bazel build @r//a || fail "build failed" } +function test_globs() { + local r=$TEST_TMPDIR/r + rm -fr $r + mkdir -p $r + cat > WORKSPACE <<EOF +local_repository( + name = "r", + path = "$r", +) + +EOF + + cat > $r/BUILD <<EOF +filegroup( + name = "fg", + srcs = glob(["**"]), +) +EOF + + touch $r/a + mkdir -p $r/b + touch $r/b/{BUILD,b} + + bazel build @r//:fg || fail "build failed" +} + run_suite "local repository tests" |