From dc41a20bb045d221a43223a5db6b8b44cd8f1676 Mon Sep 17 00:00:00 2001 From: John Cater Date: Mon, 14 Feb 2022 10:10:04 -0500 Subject: [PATCH] [5.1.0] cherrypick subpackages support (#14780) * Split ConfiguredAttributeMapper into a new build target. PiperOrigin-RevId: 411055066 * Part 1 of the Implementation for new 'subpackages()` built-in helper function. Design proposal: https://docs.google.com/document/d/13UOT0GoQofxDW40ILzH2sWpUOmuYy6QZ7CUmhej9vgk/edit# This CL modifies the globber infrastructure to support an additional mode of listing sub-directories. * Add new Globber Operation enum allowing, Globber implementations to discriminate between glob, glob w/directories and the future sub-packages use-case. * Modify UnixGlob to replace Predicate and bools with UnixGlobPathDiscriminator interface for: a) Determining whether to traverse a sub-directory (previously was lambda) b) function for determing what entries to include in the List produced by UnixGlob.globAsync. These allow relatively simple re-use of the same logic for both subpackages and glob 4) Add a few tests for UnixGlob to ensure both cases continue to work as expected. PiperOrigin-RevId: 421125424 * Part 2 Implementation for new 'subpackages()` built-in helper function. Design proposal: https://docs.google.com/document/d/13UOT0GoQofxDW40ILzH2sWpUOmuYy6QZ7CUmhej9vgk/edit# Overview: Add StarlarkNativeModule 'subpackages' function with parameters that mirror glob() PiperOrigin-RevId: 422652954 * Fix some typographical errors in the 'subpackages' docs. PiperOrigin-RevId: 425942284 Co-authored-by: kkress Co-authored-by: Googler --- .../build/docgen/templates/be/functions.vm | 77 ++-- .../google/devtools/build/lib/analysis/BUILD | 1 + .../devtools/build/lib/includescanning/BUILD | 1 + .../lib/includescanning/IncludeParser.java | 3 +- .../google/devtools/build/lib/packages/BUILD | 32 +- .../build/lib/packages/GlobCache.java | 202 ++++++----- .../devtools/build/lib/packages/Globber.java | 15 +- .../build/lib/packages/GlobberUtils.java | 56 +++ .../lib/packages/NonSkyframeGlobber.java | 24 +- .../build/lib/packages/PackageFactory.java | 28 +- .../lib/packages/StarlarkNativeModule.java | 92 +++-- .../build/lib/packages/WorkspaceFactory.java | 5 +- .../google/devtools/build/lib/query2/BUILD | 1 + .../devtools/build/lib/runtime/commands/BUILD | 1 + .../google/devtools/build/lib/skyframe/BUILD | 5 + .../build/lib/skyframe/GlobDescriptor.java | 36 +- .../build/lib/skyframe/GlobFunction.java | 129 ++++--- .../build/lib/skyframe/GlobValue.java | 9 +- .../build/lib/skyframe/PackageFunction.java | 51 ++- .../StarlarkNativeModuleApi.java | 37 ++ .../devtools/build/lib/vfs/UnixGlob.java | 129 +++---- .../lib/vfs/UnixGlobPathDiscriminator.java | 41 +++ .../FakeStarlarkNativeModuleApi.java | 7 + .../google/devtools/build/lib/analysis/BUILD | 1 + .../devtools/build/lib/analysis/util/BUILD | 1 + .../google/devtools/build/lib/packages/BUILD | 3 + .../build/lib/packages/GlobCacheTest.java | 113 ++++-- .../build/lib/packages/NativeGlobTest.java | 161 +++++++++ .../lib/packages/NativeSubpackagesTest.java | 341 ++++++++++++++++++ .../lib/packages/PackageFactoryTest.java | 8 +- .../devtools/build/lib/rules/apple/BUILD | 1 + .../devtools/build/lib/rules/config/BUILD | 1 + .../google/devtools/build/lib/skyframe/BUILD | 1 + .../lib/skyframe/GlobDescriptorTest.java | 33 +- .../build/lib/skyframe/GlobFunctionTest.java | 205 ++++++++++- .../com/google/devtools/build/lib/vfs/BUILD | 1 + .../devtools/build/lib/vfs/GlobTest.java | 206 +++++++---- .../build/lib/vfs/NativePathTest.java | 14 +- .../build/lib/vfs/RecursiveGlobTest.java | 11 +- .../google/devtools/build/lib/vfs/util/BUILD | 15 +- .../util/TestUnixGlobPathDiscriminator.java | 47 +++ 41 files changed, 1674 insertions(+), 471 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/packages/GlobberUtils.java create mode 100644 src/main/java/com/google/devtools/build/lib/vfs/UnixGlobPathDiscriminator.java create mode 100644 src/test/java/com/google/devtools/build/lib/packages/NativeGlobTest.java create mode 100644 src/test/java/com/google/devtools/build/lib/packages/NativeSubpackagesTest.java create mode 100644 src/test/java/com/google/devtools/build/lib/vfs/util/TestUnixGlobPathDiscriminator.java diff --git a/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm b/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm index 4824b14d235beb..a7da2ed42d1916 100644 --- a/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm +++ b/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm @@ -19,6 +19,7 @@ title: Functions
  • exports_files
  • glob
  • select
    • +
  • subpackages
    • #end @@ -636,48 +637,54 @@ sh_binary(
  • select works with most, but not all, attributes. Incompatible attributes are marked nonconfigurable in their documentation. -
    • - - -By default, Bazel produces the following error when no conditions match: -
    -Configurable attribute "foo" doesn't match this configuration (would a default
-condition help?).
-Conditions checked:
- //pkg:conditionA.
- //pkg:conditionB.
-
    + -You can signal more precise errors with no_match_error. +

    subpackages

    -

    Examples

    +
    subpackages(include, exclude=[], allow_empty=True)
    -
    -config_setting(
-    name = "windows",
-    values = {
-        "crosstool_top": "//crosstools/windows",
-    },
-)
+
    
+  subpackages() is a helper function, similar to glob()
+  that lists subpackages instead of files and directories.  It uses the same
+  path patterns as glob() and can match any subpackage that is a
+  direct decendant of the currently loading BUILD file.  See glob for a detailed explanation and examples of include and
+  exclude patterns.
+
    
 
-cc_binary(
-    name = "multiplatform_app",
-    ...
-    linkopts = select({
-        ":windows": [
-            "-Wl,windows_support1.lib",
-            "-Wl,windows_support2.lib",
-        ],
-        "//conditions:default": [],
-    ...
-)
-
    +

    + The resulting list of subpackages returned is in sorted order and contains + paths relative to the current loading package that match the given patterns in + include and not those in exclude. -

    In the above example, multiplatform_app links with additional - options when invoked with bazel build //pkg:multiplatform_app - --crosstool_top=//crosstools/windows . +

    Example

    + The following example lists all the direct subpackages for the package foo/BUILD + +

    +# The following BUILD files exist:
+# foo/BUILD
+# foo/bar/baz/BUILD
+# foo/sub/BUILD
+# foo/sub/deeper/BUILD
+#
+# In foo/BUILD a call to
+subs = subpackages(include = ["**"])
+
+# results in subs == ["sub", "bar/baz"]
+#
+# 'sub/deeper' is not included because it is a subpackage of 'foo/sub' not of
+# 'foo'
+
    + +

    + In general it is preferred that instead of calling this function directly + that users use the 'subpackages' module of + skylib. #if (!$singlePage) #parse("com/google/devtools/build/docgen/templates/be/footer.vm") diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index eb84111920299d..ce55b174f16cd0 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -400,6 +400,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec:test_xml_output_parser_exception", "//src/main/java/com/google/devtools/build/lib/graph", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper", "//src/main/java/com/google/devtools/build/lib/packages:exec_group", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/profiler", diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/BUILD b/src/main/java/com/google/devtools/build/lib/includescanning/BUILD index 7da1f1179557ca..d156e189f0a2ca 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/BUILD +++ b/src/main/java/com/google/devtools/build/lib/includescanning/BUILD @@ -33,6 +33,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec:module_action_context_registry", "//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_resolver", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:globber", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/rules/cpp", "//src/main/java/com/google/devtools/build/lib/skyframe:containing_package_lookup_value", diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeParser.java b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeParser.java index 0e405b0a259934..b0d0b1f07936f1 100644 --- a/src/main/java/com/google/devtools/build/lib/includescanning/IncludeParser.java +++ b/src/main/java/com/google/devtools/build/lib/includescanning/IncludeParser.java @@ -38,6 +38,7 @@ import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.includescanning.IncludeParser.Inclusion.Kind; +import com.google.devtools.build.lib.packages.Globber; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; @@ -347,7 +348,7 @@ ImmutableSet getPathLevelHintedInclusions( containingPackageLookupValue.getContainingPackageName(), containingPackageLookupValue.getContainingPackageRoot(), pattern, - /*excludeDirs=*/ true, + Globber.Operation.FILES, relativePath.relativeTo(packageFragment))); } catch (InvalidGlobPatternException e) { env.getListener() diff --git a/src/main/java/com/google/devtools/build/lib/packages/BUILD b/src/main/java/com/google/devtools/build/lib/packages/BUILD index f4f1d8164671d8..b7fac19dac5551 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/packages/BUILD @@ -10,21 +10,51 @@ filegroup( visibility = ["//src:__subpackages__"], ) +java_library( + name = "configured_attribute_mapper", + srcs = ["ConfiguredAttributeMapper.java"], + deps = [ + ":packages", + "//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider", + "//src/main/java/com/google/devtools/build/lib/cmdline", + "//third_party:guava", + "//third_party:jsr305", + ], +) + +java_library( + name = "globber", + srcs = ["Globber.java"], +) + +java_library( + name = "globber_utils", + srcs = ["GlobberUtils.java"], + deps = [ + ":globber", + "//third_party:error_prone_annotations", + ], +) + java_library( name = "packages", srcs = glob( ["*.java"], exclude = [ "BuilderFactoryForTesting.java", # see builder_factory_for_testing + "Globber.java", + "GlobberUtils.java", "ExecGroup.java", + "ConfiguredAttributeMapper.java", ], ), deps = [ ":exec_group", + ":globber", + ":globber_utils", "//src/main/java/com/google/devtools/build/docgen/annot", "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements", "//src/main/java/com/google/devtools/build/lib/actions:thread_state_receiver", - "//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment", "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_class_set", "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition", 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 33c882d98ffdbb..11caecc411b115 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 @@ -15,7 +15,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import com.google.common.base.Predicate; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; @@ -29,6 +28,7 @@ import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.UnixGlob; +import com.google.devtools.build.lib.vfs.UnixGlobPathDiscriminator; import java.io.IOException; import java.util.ArrayList; import java.util.Collection; @@ -44,36 +44,26 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; -/** - * Caches the results of glob expansion for a package. - */ +/** Caches the results of glob expansion for a package. */ @ThreadSafety.ThreadCompatible public class GlobCache { /** - * A mapping from glob expressions (e.g. "*.java") to the list of files it - * matched (in the order returned by VFS) at the time the package was - * constructed. Required for sound dependency analysis. + * A mapping from glob expressions (e.g. "*.java") to the list of files it matched (in the order + * returned by VFS) at the time the package was constructed. Required for sound dependency + * analysis. * - * We don't use a Multimap because it provides no way to distinguish "key not - * present" from (key -> {}). + *

    We don't use a Multimap because it provides no way to distinguish "key not present" from + * (key -> {}). */ - private final Map, Future>> globCache = new HashMap<>(); + private final Map, Future>> globCache = + new HashMap<>(); - /** - * The directory in which our package's BUILD file resides. - */ + /** The directory in which our package's BUILD file resides. */ private final Path packageDirectory; - /** - * The name of the package we belong to. - */ + /** The name of the package we belong to. */ private final PackageIdentifier packageId; - /** - * The package locator-based directory traversal predicate. - */ - private final Predicate childDirectoryPredicate; - /** System call caching layer. */ private final AtomicReference syscalls; @@ -84,6 +74,10 @@ public class GlobCache { private final AtomicBoolean globalStarted = new AtomicBoolean(false); + private final CachingPackageLocator packageLocator; + + private final ImmutableSet ignoredGlobPrefixes; + /** * Create a glob expansion cache. * @@ -120,47 +114,55 @@ public GlobCache( this.maxDirectoriesToEagerlyVisit = maxDirectoriesToEagerlyVisit; Preconditions.checkNotNull(locator); - childDirectoryPredicate = - directory -> { - if (directory.equals(packageDirectory)) { - return true; - } + this.packageLocator = locator; + this.ignoredGlobPrefixes = ignoredGlobPrefixes; + } - PathFragment subPackagePath = - packageId.getPackageFragment().getRelative(directory.relativeTo(packageDirectory)); + private boolean globCacheShouldTraverseDirectory(Path directory) { + if (directory.equals(packageDirectory)) { + return true; + } - for (PathFragment ignoredPrefix : ignoredGlobPrefixes) { - if (subPackagePath.startsWith(ignoredPrefix)) { - return false; - } - } + PathFragment subPackagePath = + packageId.getPackageFragment().getRelative(directory.relativeTo(packageDirectory)); + + for (PathFragment ignoredPrefix : ignoredGlobPrefixes) { + if (subPackagePath.startsWith(ignoredPrefix)) { + return false; + } + } - PackageIdentifier subPackageId = - PackageIdentifier.create(packageId.getRepository(), subPackagePath); - return locator.getBuildFileForPackage(subPackageId) == null; - }; + return !isSubPackage(PackageIdentifier.create(packageId.getRepository(), subPackagePath)); + } + + private boolean isSubPackage(Path directory) { + return isSubPackage( + PackageIdentifier.create( + packageId.getRepository(), + packageId.getPackageFragment().getRelative(directory.relativeTo(packageDirectory)))); + } + + private boolean isSubPackage(PackageIdentifier subPackageId) { + return packageLocator.getBuildFileForPackage(subPackageId) != null; } /** - * Returns the future result of evaluating glob "pattern" against this - * package's directory, using the package's cache of previously-started - * globs if possible. + * Returns the future result of evaluating glob "pattern" against this package's directory, using + * the package's cache of previously-started globs if possible. * - * @return the list of paths matching the pattern, relative to the package's - * directory. - * @throws BadGlobException if the glob was syntactically invalid, or - * contained uplevel references. + * @return the list of paths matching the pattern, relative to the package's directory. + * @throws BadGlobException if the glob was syntactically invalid, or contained uplevel + * references. */ - Future> getGlobUnsortedAsync(String pattern, boolean excludeDirs) + Future> getGlobUnsortedAsync(String pattern, Globber.Operation globberOperation) throws BadGlobException { - Future> cached = globCache.get(Pair.of(pattern, excludeDirs)); + Future> cached = globCache.get(Pair.of(pattern, globberOperation)); if (cached == null) { - if (maxDirectoriesToEagerlyVisit > -1 - && !globalStarted.getAndSet(true)) { + if (maxDirectoriesToEagerlyVisit > -1 && !globalStarted.getAndSet(true)) { packageDirectory.prefetchPackageAsync(maxDirectoriesToEagerlyVisit); } - cached = safeGlobUnsorted(pattern, excludeDirs); - setGlobPaths(pattern, excludeDirs, cached); + cached = safeGlobUnsorted(pattern, globberOperation); + setGlobPaths(pattern, globberOperation, cached); } return cached; } @@ -168,20 +170,20 @@ Future> getGlobUnsortedAsync(String pattern, boolean excludeDirs) @VisibleForTesting List getGlobUnsorted(String pattern) throws IOException, BadGlobException, InterruptedException { - return getGlobUnsorted(pattern, false); + return getGlobUnsorted(pattern, Globber.Operation.FILES_AND_DIRS); } @VisibleForTesting - protected List getGlobUnsorted(String pattern, boolean excludeDirs) + protected List getGlobUnsorted(String pattern, Globber.Operation globberOperation) throws IOException, BadGlobException, InterruptedException { - Future> futureResult = getGlobUnsortedAsync(pattern, excludeDirs); + Future> futureResult = getGlobUnsortedAsync(pattern, globberOperation); List globPaths = fromFuture(futureResult); // Replace the UnixGlob.GlobFuture with a completed future object, to allow // garbage collection of the GlobFuture and GlobVisitor objects. if (!(futureResult instanceof SettableFuture)) { SettableFuture> completedFuture = SettableFuture.create(); completedFuture.set(globPaths); - globCache.put(Pair.of(pattern, excludeDirs), completedFuture); + globCache.put(Pair.of(pattern, globberOperation), completedFuture); } List result = Lists.newArrayListWithCapacity(globPaths.size()); @@ -198,16 +200,15 @@ protected List getGlobUnsorted(String pattern, boolean excludeDirs) } /** Adds glob entries to the cache. */ - private void setGlobPaths(String pattern, boolean excludeDirectories, Future> result) { - globCache.put(Pair.of(pattern, excludeDirectories), result); + private void setGlobPaths( + String pattern, Globber.Operation globberOperation, Future> result) { + globCache.put(Pair.of(pattern, globberOperation), result); } - /** - * Actually execute a glob against the filesystem. Otherwise similar to - * getGlob(). - */ + /** Actually execute a glob against the filesystem. Otherwise similar to getGlob(). */ @VisibleForTesting - Future> safeGlobUnsorted(String pattern, boolean excludeDirs) throws BadGlobException { + Future> safeGlobUnsorted(String pattern, Globber.Operation globberOperation) + throws BadGlobException { // Forbidden patterns: if (pattern.indexOf('?') != -1) { throw new BadGlobException("glob pattern '" + pattern + "' contains forbidden '?' wildcard"); @@ -220,8 +221,7 @@ Future> safeGlobUnsorted(String pattern, boolean excludeDirs) throws try { return UnixGlob.forPath(packageDirectory) .addPattern(pattern) - .setExcludeDirectories(excludeDirs) - .setDirectoryFilter(childDirectoryPredicate) + .setPathDiscriminator(new GlobUnixPathDiscriminator(globberOperation)) .setExecutor(globExecutor) .setFilesystemCalls(syscalls) .globAsync(); @@ -230,18 +230,14 @@ Future> safeGlobUnsorted(String pattern, boolean excludeDirs) throws } } - /** - * Sanitize the future exceptions - the only expected checked exception - * is IOException. - */ + /** Sanitize the future exceptions - the only expected checked exception is IOException. */ private static List fromFuture(Future> future) throws IOException, InterruptedException { try { return future.get(); } catch (ExecutionException e) { Throwable cause = e.getCause(); - Throwables.propagateIfPossible(cause, - IOException.class, InterruptedException.class); + Throwables.propagateIfPossible(cause, IOException.class, InterruptedException.class); throw new RuntimeException(e); } } @@ -253,26 +249,24 @@ private static List fromFuture(Future> future) *

    Called by PackageFactory via Package. */ public List globUnsorted( - List includes, List excludes, boolean excludeDirs, boolean allowEmpty) + List includes, + List excludes, + Globber.Operation globberOperation, + boolean allowEmpty) throws IOException, BadGlobException, InterruptedException { // Start globbing all patterns in parallel. The getGlob() calls below will // block on an individual pattern's results, but the other globs can // continue in the background. for (String pattern : includes) { @SuppressWarnings("unused") - Future possiblyIgnoredError = getGlobUnsortedAsync(pattern, excludeDirs); + Future possiblyIgnoredError = getGlobUnsortedAsync(pattern, globberOperation); } HashSet results = new HashSet<>(); for (String pattern : includes) { - List items = getGlobUnsorted(pattern, excludeDirs); + List items = getGlobUnsorted(pattern, globberOperation); if (!allowEmpty && items.isEmpty()) { - throw new BadGlobException( - "glob pattern '" - + pattern - + "' didn't match anything, but allow_empty is set to False " - + "(the default value of allow_empty can be set with " - + "--incompatible_disallow_empty_glob)."); + GlobberUtils.throwBadGlobExceptionEmptyResult(pattern, globberOperation); } results.addAll(items); } @@ -282,21 +276,16 @@ public List globUnsorted( throw new BadGlobException(ex.getMessage()); } if (!allowEmpty && results.isEmpty()) { - throw new BadGlobException( - "all files in the glob have been excluded, but allow_empty is set to False " - + "(the default value of allow_empty can be set with " - + "--incompatible_disallow_empty_glob)."); + GlobberUtils.throwBadGlobExceptionAllExcluded(globberOperation); } return new ArrayList<>(results); } - public Set> getKeySet() { + public Set> getKeySet() { return globCache.keySet(); } - /** - * Block on the completion of all potentially-abandoned background tasks. - */ + /** Block on the completion of all potentially-abandoned background tasks. */ public void finishBackgroundTasks() { finishBackgroundTasks(globCache.values()); } @@ -334,4 +323,45 @@ private static void cancelBackgroundTasks(Collection>> tasks) public String toString() { return "GlobCache for " + packageId + " in " + packageDirectory; } + + /** + * Used by 'glob()' and 'subpackages()' with UnixGlob to determine if a directory should be + * traversed when recursing through a filesystem directory structure or include a Path in the + * result. This essentially filters out a set of ignored prefixes and then checks to see if a + * given sub-dir actually represents a sub-package or not when traversing. + * + *

    The logic of including inspects the Globber.Operation to determine if it will include all + * files, include directories or subpackages in the output. + */ + private class GlobUnixPathDiscriminator implements UnixGlobPathDiscriminator { + private final Globber.Operation globberOperation; + + GlobUnixPathDiscriminator(Globber.Operation globberOperation) { + this.globberOperation = globberOperation; + } + + @Override + public boolean shouldTraverseDirectory(Path directory) { + return globCacheShouldTraverseDirectory(directory); + } + + @Override + public boolean shouldIncludePathInResult(Path path, boolean isDirectory) { + switch (globberOperation) { + case FILES_AND_DIRS: + return !isDirectory || !isSubPackage(path); + case SUBPACKAGES: + // no files, or root pkg + if (!isDirectory || path.equals(packageDirectory)) { + return false; + } + return isSubPackage(path); + + case FILES: + return !isDirectory; + } + throw new IllegalStateException( + "Unexpected unhandled Globber.Operation enum value: " + globberOperation); + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/Globber.java b/src/main/java/com/google/devtools/build/lib/packages/Globber.java index a580a47f9f7833..4bc21847eee00c 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Globber.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Globber.java @@ -21,6 +21,19 @@ public interface Globber { /** An opaque token for fetching the result of a glob computation. */ abstract class Token {} + /** + * Indiciates they type of globbing operations we're doing, whether looking for Files+Dirs or + * sub-packages. + */ + enum Operation { + // Return only files, + FILES, + // Return files and directories, but not sub-packages + FILES_AND_DIRS, + // Return only sub-packages + SUBPACKAGES, + } + /** Used to indicate an invalid glob pattern. */ class BadGlobException extends Exception { public BadGlobException(String message) { @@ -35,7 +48,7 @@ public BadGlobException(String message) { * invalid. */ Token runAsync( - List includes, List excludes, boolean excludeDirs, boolean allowEmpty) + List includes, List excludes, Operation operation, boolean allowEmpty) throws BadGlobException, InterruptedException; /** diff --git a/src/main/java/com/google/devtools/build/lib/packages/GlobberUtils.java b/src/main/java/com/google/devtools/build/lib/packages/GlobberUtils.java new file mode 100644 index 00000000000000..766cb83939d8d7 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/packages/GlobberUtils.java @@ -0,0 +1,56 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.packages; + +import com.google.errorprone.annotations.CheckReturnValue; + +/** Static functionality shared by different implementations of the Globber interface. */ +@CheckReturnValue +public final class GlobberUtils { + + private GlobberUtils() {} + + public static void throwBadGlobExceptionEmptyResult( + String pattern, Globber.Operation globberOperation) throws Globber.BadGlobException { + switch (globberOperation) { + case SUBPACKAGES: + throw new Globber.BadGlobException( + "subpackages pattern '" + + pattern + + "' didn't match anything, but allow_empty is set to False (the default value)"); + default: + throw new Globber.BadGlobException( + "glob pattern '" + + pattern + + "' didn't match anything, but allow_empty is set to False " + + "(the default value of allow_empty can be set with " + + "--incompatible_disallow_empty_glob)."); + } + } + + public static void throwBadGlobExceptionAllExcluded(Globber.Operation globberOperation) + throws Globber.BadGlobException { + switch (globberOperation) { + case SUBPACKAGES: + throw new Globber.BadGlobException( + "all subpackages in subpackages() have been excluded, but allow_empty is" + + " set to False "); + default: + throw new Globber.BadGlobException( + "all files in the glob have been excluded, but allow_empty is set to False " + + "(the default value of allow_empty can be set with " + + "--incompatible_disallow_empty_glob)."); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/packages/NonSkyframeGlobber.java b/src/main/java/com/google/devtools/build/lib/packages/NonSkyframeGlobber.java index 01246fc58f95cc..1a85563c40f0af 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/NonSkyframeGlobber.java +++ b/src/main/java/com/google/devtools/build/lib/packages/NonSkyframeGlobber.java @@ -30,27 +30,34 @@ public class NonSkyframeGlobber implements Globber { public static class Token extends Globber.Token { private final List includes; private final List excludes; - private final boolean excludeDirs; + private final Globber.Operation globberOperation; private final boolean allowEmpty; private Token( - List includes, List excludes, boolean excludeDirs, boolean allowEmpty) { + List includes, + List excludes, + Globber.Operation globberOperation, + boolean allowEmpty) { this.includes = includes; this.excludes = excludes; - this.excludeDirs = excludeDirs; + this.globberOperation = globberOperation; this.allowEmpty = allowEmpty; } } @Override public Token runAsync( - List includes, List excludes, boolean excludeDirs, boolean allowEmpty) + List includes, + List excludes, + Globber.Operation globberOperation, + boolean allowEmpty) throws BadGlobException { + for (String pattern : includes) { @SuppressWarnings("unused") - Future possiblyIgnoredError = globCache.getGlobUnsortedAsync(pattern, excludeDirs); + Future possiblyIgnoredError = globCache.getGlobUnsortedAsync(pattern, globberOperation); } - return new Token(includes, excludes, excludeDirs, allowEmpty); + return new Token(includes, excludes, globberOperation, allowEmpty); } @Override @@ -58,10 +65,7 @@ public List fetchUnsorted(Globber.Token token) throws BadGlobException, IOException, InterruptedException { Token ourToken = (Token) token; return globCache.globUnsorted( - ourToken.includes, - ourToken.excludes, - ourToken.excludeDirs, - ourToken.allowEmpty); + ourToken.includes, ourToken.excludes, ourToken.globberOperation, ourToken.allowEmpty); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java index dd57fe75e31934..b0e956743c53c2 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java @@ -601,6 +601,7 @@ public void executeBuildFile( Program buildFileProgram, ImmutableList globs, ImmutableList globsWithDirs, + ImmutableList subpackages, ImmutableMap predeclared, ImmutableMap loadedModules, StarlarkSemantics starlarkSemantics, @@ -610,8 +611,11 @@ public void executeBuildFile( if (maxDirectoriesToEagerlyVisitInGlobbing == -2) { try { boolean allowEmpty = true; - globber.runAsync(globs, ImmutableList.of(), /*excludeDirs=*/ true, allowEmpty); - globber.runAsync(globsWithDirs, ImmutableList.of(), /*excludeDirs=*/ false, allowEmpty); + globber.runAsync(globs, ImmutableList.of(), Globber.Operation.FILES, allowEmpty); + globber.runAsync( + globsWithDirs, ImmutableList.of(), Globber.Operation.FILES_AND_DIRS, allowEmpty); + globber.runAsync( + subpackages, ImmutableList.of(), Globber.Operation.SUBPACKAGES, allowEmpty); } catch (BadGlobException ex) { logger.atWarning().withCause(ex).log( "Suppressing exception for globs=%s, globsWithDirs=%s", globs, globsWithDirs); @@ -733,6 +737,7 @@ public static boolean checkBuildSyntax( StarlarkFile file, Collection globs, Collection globsWithDirs, + Collection subpackages, Map generatorNameByLocation, Consumer errors) { final boolean[] success = {true}; @@ -746,11 +751,16 @@ void error(Location loc, String message) { // Extract literal glob patterns from calls of the form: // glob(include = ["pattern"]) // glob(["pattern"]) - // This may spuriously match user-defined functions named glob; - // that's ok, it's only a heuristic. + // subpackages(include = ["pattern"]) + // This may spuriously match user-defined functions named glob or + // subpackages; that's ok, it's only a heuristic. void extractGlobPatterns(CallExpression call) { - if (call.getFunction() instanceof Identifier - && ((Identifier) call.getFunction()).getName().equals("glob")) { + if (call.getFunction() instanceof Identifier) { + String functionName = ((Identifier) call.getFunction()).getName(); + if (!functionName.equals("glob") && !functionName.equals("subpackages")) { + return; + } + Expression excludeDirectories = null, include = null; List arguments = call.getArguments(); for (int i = 0; i < arguments.size(); i++) { @@ -778,7 +788,11 @@ void extractGlobPatterns(CallExpression call) { exclude = false; } } - (exclude ? globs : globsWithDirs).add(pattern); + if (functionName.equals("glob")) { + (exclude ? globs : globsWithDirs).add(pattern); + } else { + subpackages.add(pattern); + } } } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java index b18598b1c8c88e..916f312351c6df 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java @@ -99,8 +99,9 @@ public Sequence glob( List includes = Type.STRING_LIST.convert(include, "'glob' argument"); List excludes = Type.STRING_LIST.convert(exclude, "'glob' argument"); + Globber.Operation op = + excludeDirs.signum() != 0 ? Globber.Operation.FILES : Globber.Operation.FILES_AND_DIRS; - List matches; boolean allowEmpty; if (allowEmptyArgument == Starlark.UNBOUND) { allowEmpty = @@ -112,36 +113,7 @@ public Sequence glob( "expected boolean for argument `allow_empty`, got `%s`", allowEmptyArgument); } - try { - Globber.Token globToken = - context.globber.runAsync(includes, excludes, excludeDirs.signum() != 0, allowEmpty); - matches = context.globber.fetchUnsorted(globToken); - } catch (IOException e) { - logger.atWarning().withCause(e).log( - "Exception processing includes=%s, excludes=%s)", includes, excludes); - String errorMessage = - String.format( - "error globbing [%s]%s: %s", - Joiner.on(", ").join(includes), - excludes.isEmpty() ? "" : " - [" + Joiner.on(", ").join(excludes) + "]", - e.getMessage()); - Location loc = thread.getCallerLocation(); - Event error = - Package.error( - loc, - errorMessage, - // If there are other IOExceptions that can result from user error, they should be - // tested for here. Currently FileNotFoundException is not one of those, because globs - // only encounter that error in the presence of an inconsistent filesystem. - e instanceof FileSymlinkException - ? Code.EVAL_GLOBS_SYMLINK_ERROR - : Code.GLOB_IO_EXCEPTION); - context.eventHandler.handle(error); - context.pkgBuilder.setIOException(e, errorMessage, error.getProperty(DetailedExitCode.class)); - matches = ImmutableList.of(); - } catch (BadGlobException e) { - throw new EvalException(e); - } + List matches = runGlobOperation(context, thread, includes, excludes, op, allowEmpty); ArrayList result = new ArrayList<>(matches.size()); for (String match : matches) { @@ -750,4 +722,62 @@ private static class NotRepresentableException extends EvalException { super(msg); } } + + @Override + public Sequence subpackages( + Sequence include, Sequence exclude, boolean allowEmpty, StarlarkThread thread) + throws EvalException, InterruptedException { + BazelStarlarkContext.from(thread).checkLoadingPhase("native.subpackages"); + PackageContext context = getContext(thread); + + List includes = Type.STRING_LIST.convert(include, "'subpackages' argument"); + List excludes = Type.STRING_LIST.convert(exclude, "'subpackages' argument"); + + List matches = + runGlobOperation( + context, thread, includes, excludes, Globber.Operation.SUBPACKAGES, allowEmpty); + matches.sort(naturalOrder()); + + return StarlarkList.copyOf(thread.mutability(), matches); + } + + private List runGlobOperation( + PackageContext context, + StarlarkThread thread, + List includes, + List excludes, + Globber.Operation operation, + boolean allowEmpty) + throws EvalException, InterruptedException { + try { + Globber.Token globToken = context.globber.runAsync(includes, excludes, operation, allowEmpty); + return context.globber.fetchUnsorted(globToken); + } catch (IOException e) { + logger.atWarning().withCause(e).log( + "Exception processing includes=%s, excludes=%s)", includes, excludes); + String errorMessage = + String.format( + "error globbing [%s]%s op=%s: %s", + Joiner.on(", ").join(includes), + excludes.isEmpty() ? "" : " - [" + Joiner.on(", ").join(excludes) + "]", + operation, + e.getMessage()); + Location loc = thread.getCallerLocation(); + Event error = + Package.error( + loc, + errorMessage, + // If there are other IOExceptions that can result from user error, they should be + // tested for here. Currently FileNotFoundException is not one of those, because globs + // only encounter that error in the presence of an inconsistent filesystem. + e instanceof FileSymlinkException + ? Code.EVAL_GLOBS_SYMLINK_ERROR + : Code.GLOB_IO_EXCEPTION); + context.eventHandler.handle(error); + context.pkgBuilder.setIOException(e, errorMessage, error.getProperty(DetailedExitCode.class)); + return ImmutableList.of(); + } catch (BadGlobException e) { + throw new EvalException(e); + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java index 7e7c5470e5e1fa..963956dab128fc 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java @@ -146,8 +146,9 @@ public void execute( List globs = new ArrayList<>(); // unused if (PackageFactory.checkBuildSyntax( file, - globs, - globs, + /*globs=*/ globs, + /*globsWithDirs=*/ globs, + /*subpackages=*/ globs, new HashMap<>(), error -> localReporter.handle( diff --git a/src/main/java/com/google/devtools/build/lib/query2/BUILD b/src/main/java/com/google/devtools/build/lib/query2/BUILD index 8e8486c08bf7ea..ed7f359e92b7af 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/BUILD +++ b/src/main/java/com/google/devtools/build/lib/query2/BUILD @@ -59,6 +59,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/graph", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper", "//src/main/java/com/google/devtools/build/lib/packages:exec_group", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/pkgcache:QueryTransitivePackagePreloader", diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD b/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD index dd2f4c8257acc4..2a382fdd065d7b 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD @@ -56,6 +56,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec:test_policy", "//src/main/java/com/google/devtools/build/lib/exec/local:options", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/profiler:profiler-output", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 8fc7732933dc0b..468b1290e45e5b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -293,6 +293,8 @@ java_library( "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception", "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/packages:exec_group", + "//src/main/java/com/google/devtools/build/lib/packages:globber", + "//src/main/java/com/google/devtools/build/lib/packages:globber_utils", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/pkgcache:QueryTransitivePackagePreloader", @@ -1441,6 +1443,7 @@ java_library( ":sky_functions", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/concurrent", + "//src/main/java/com/google/devtools/build/lib/packages:globber", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/util:string", "//src/main/java/com/google/devtools/build/lib/vfs", @@ -1465,6 +1468,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/io:file_symlink_infinite_expansion_exception", "//src/main/java/com/google/devtools/build/lib/io:file_symlink_infinite_expansion_uniqueness_function", "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception", + "//src/main/java/com/google/devtools/build/lib/packages:globber", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/build/skyframe", @@ -1482,6 +1486,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", + "//src/main/java/com/google/devtools/build/lib/packages:globber", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GlobDescriptor.java b/src/main/java/com/google/devtools/build/lib/skyframe/GlobDescriptor.java index f64d451c9e90d9..a436090f9841f5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/GlobDescriptor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/GlobDescriptor.java @@ -18,6 +18,7 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; +import com.google.devtools.build.lib.packages.Globber; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.util.StringCanonicalizer; import com.google.devtools.build.lib.vfs.PathFragment; @@ -46,7 +47,7 @@ public final class GlobDescriptor implements SkyKey { * @param subdir the subdirectory being looked at (must exist and must be a directory. It's * assumed that there are no other packages between {@code packageName} and {@code subdir}. * @param pattern a valid glob pattern - * @param excludeDirs true if directories should be excluded from results + * @param globberOperation type of Globber operation being tracked. */ @AutoCodec.Instantiator public static GlobDescriptor create( @@ -54,36 +55,35 @@ public static GlobDescriptor create( Root packageRoot, PathFragment subdir, String pattern, - boolean excludeDirs) { + Globber.Operation globberOperation) { return interner.intern( - new GlobDescriptor(packageId, packageRoot, subdir, pattern, excludeDirs)); - + new GlobDescriptor(packageId, packageRoot, subdir, pattern, globberOperation)); } private final PackageIdentifier packageId; private final Root packageRoot; private final PathFragment subdir; private final String pattern; - private final boolean excludeDirs; + private final Globber.Operation globberOperation; private GlobDescriptor( PackageIdentifier packageId, Root packageRoot, PathFragment subdir, String pattern, - boolean excludeDirs) { + Globber.Operation globberOperation) { this.packageId = Preconditions.checkNotNull(packageId); this.packageRoot = Preconditions.checkNotNull(packageRoot); this.subdir = Preconditions.checkNotNull(subdir); this.pattern = Preconditions.checkNotNull(StringCanonicalizer.intern(pattern)); - this.excludeDirs = excludeDirs; + this.globberOperation = globberOperation; } @Override public String toString() { return String.format( - "", - packageId, packageRoot, subdir, pattern, excludeDirs); + "", + packageId, packageRoot, subdir, pattern, globberOperation.name()); } /** @@ -117,11 +117,9 @@ public String getPattern() { return pattern; } - /** - * Returns true if directories should be excluded from results. - */ - public boolean excludeDirs() { - return excludeDirs; + /** Returns the type of Globber operation that produced the results. */ + public Globber.Operation globberOperation() { + return globberOperation; } @Override @@ -133,9 +131,11 @@ public boolean equals(Object obj) { return false; } GlobDescriptor other = (GlobDescriptor) obj; - return packageId.equals(other.packageId) && packageRoot.equals(other.packageRoot) - && subdir.equals(other.subdir) && pattern.equals(other.pattern) - && excludeDirs == other.excludeDirs; + return packageId.equals(other.packageId) + && packageRoot.equals(other.packageRoot) + && subdir.equals(other.subdir) + && pattern.equals(other.pattern) + && globberOperation == other.globberOperation; } @Override @@ -143,7 +143,7 @@ public int hashCode() { // Generated instead of Objects.hashCode to avoid intermediate array required for latter. final int prime = 31; int result = 1; - result = prime * result + (excludeDirs ? 1231 : 1237); + result = prime * result + globberOperation.hashCode(); result = prime * result + packageId.hashCode(); result = prime * result + packageRoot.hashCode(); result = prime * result + pattern.hashCode(); 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 9f3c88ed9d3c9c..ee4b2b16d3dc6c 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 @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.io.FileSymlinkInfiniteExpansionException; import com.google.devtools.build.lib.io.FileSymlinkInfiniteExpansionUniquenessFunction; import com.google.devtools.build.lib.io.InconsistentFilesystemException; +import com.google.devtools.build.lib.packages.Globber; import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.RootedPath; @@ -58,6 +59,7 @@ public GlobFunction(boolean alwaysUseDirListing) { public SkyValue compute(SkyKey skyKey, Environment env) throws GlobFunctionException, InterruptedException { GlobDescriptor glob = (GlobDescriptor) skyKey.argument(); + Globber.Operation globberOperation = glob.globberOperation(); RepositoryName repositoryName = glob.getPackageId().getRepository(); IgnoredPackagePrefixesValue ignoredPackagePrefixes = @@ -78,19 +80,32 @@ public SkyValue compute(SkyKey skyKey, Environment env) // Note that the glob's package is assumed to exist which implies that the package's BUILD file // exists which implies that the package's directory exists. if (!globSubdir.equals(PathFragment.EMPTY_FRAGMENT)) { + PathFragment subDirFragment = + glob.getPackageId().getPackageFragment().getRelative(globSubdir); + PackageLookupValue globSubdirPkgLookupValue = (PackageLookupValue) env.getValue( - PackageLookupValue.key( - PackageIdentifier.create( - repositoryName, - glob.getPackageId().getPackageFragment().getRelative(globSubdir)))); + PackageLookupValue.key(PackageIdentifier.create(repositoryName, subDirFragment))); if (globSubdirPkgLookupValue == null) { return null; } + if (globSubdirPkgLookupValue.packageExists()) { // We crossed the package boundary, that is, pkg/subdir contains a BUILD file and thus - // defines another package, so glob expansion should not descend into that subdir. + // defines another package, so glob expansion should not descend into + // that subdir. + // + // For SUBPACKAGES, we encounter this when the pattern is a recursive ** and we are a + // terminal package for that pattern. In that case we should include the subDirFragment + // PathFragment (relative to the glob's package) in the GlobValue.getMatches, + // otherwise for file/dir matching return EMPTY; + if (globberOperation == Globber.Operation.SUBPACKAGES) { + return new GlobValue( + NestedSetBuilder.stableOrder() + .add(subDirFragment.relativeTo(glob.getPackageId().getPackageFragment())) + .build()); + } return GlobValue.EMPTY; } else if (globSubdirPkgLookupValue instanceof PackageLookupValue.IncorrectRepositoryReferencePackageLookupValue) { @@ -117,7 +132,6 @@ public SkyValue compute(SkyKey skyKey, Environment env) boolean globMatchesBareFile = patternTail == null; - RootedPath dirRootedPath = RootedPath.toRootedPath(glob.getPackageRoot(), dirPathFragment); if (alwaysUseDirListing || containsGlobs(patternHead)) { // Pattern contains globs, so a directory listing is required. @@ -135,7 +149,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) // "**" also matches an empty segment, so try the case where it is not present. if (globMatchesBareFile) { // Recursive globs aren't supposed to match the package's directory. - if (!glob.excludeDirs() && !globSubdir.equals(PathFragment.EMPTY_FRAGMENT)) { + if (globberOperation == Globber.Operation.FILES_AND_DIRS + && !globSubdir.equals(PathFragment.EMPTY_FRAGMENT)) { matches.add(globSubdir); } } else { @@ -148,7 +163,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) glob.getPackageRoot(), globSubdir, patternTail, - glob.excludeDirs()); + globberOperation); Map listingAndRecursiveGlobMap = env.getValues( ImmutableList.of(keyForRecursiveGlobInCurrentDirectory, directoryListingKey)); @@ -209,7 +224,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) if (keyToRequest != null) { subdirMap.put(keyToRequest, dirent); } - } else if (globMatchesBareFile) { + } else if (globMatchesBareFile && globberOperation != Globber.Operation.SUBPACKAGES) { sortedResultMap.put(dirent, glob.getSubdir().getRelative(fileName)); } } @@ -266,7 +281,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) if (keyToRequest != null) { symlinkSubdirMap.put(keyToRequest, dirent); } - } else if (globMatchesBareFile) { + } else if (globMatchesBareFile && globberOperation != Globber.Operation.SUBPACKAGES) { sortedResultMap.put(dirent, glob.getSubdir().getRelative(fileName)); } } else { @@ -308,7 +323,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) addToMatches(fileMatches, matches); } } - } else if (globMatchesBareFile) { + } else if (globMatchesBareFile && globberOperation != Globber.Operation.SUBPACKAGES) { matches.add(glob.getSubdir().getRelative(fileName)); } } @@ -346,9 +361,10 @@ private static boolean containsGlobs(String pattern) { private static void addToMatches(Object toAdd, NestedSetBuilder matches) { if (toAdd instanceof PathFragment) { matches.add((PathFragment) toAdd); - } else { + } else if (toAdd instanceof NestedSet) { matches.addTransitive((NestedSet) toAdd); } + // else Not actually a valid type and ignore. } /** @@ -365,17 +381,19 @@ private static void addToMatches(Object toAdd, NestedSetBuilder ma private static SkyKey getSkyKeyForSubdir( String fileName, GlobDescriptor glob, String subdirPattern) { if (subdirPattern == null) { - if (glob.excludeDirs()) { + if (glob.globberOperation() == Globber.Operation.FILES) { return null; - } else { - return PackageLookupValue.key( - PackageIdentifier.create( - glob.getPackageId().getRepository(), - glob.getPackageId() - .getPackageFragment() - .getRelative(glob.getSubdir()) - .getRelative(fileName))); } + + // For FILES_AND_DIRS and SUBPACKAGES we want to maybe inspect a + // PackageLookupValue for it. + return PackageLookupValue.key( + PackageIdentifier.create( + glob.getPackageId().getRepository(), + glob.getPackageId() + .getPackageFragment() + .getRelative(glob.getSubdir()) + .getRelative(fileName))); } else { // There is some more pattern to match. Get the glob for the subdirectory. Note that this // directory may also match directly in the case of a pattern that starts with "**", but that @@ -385,43 +403,60 @@ private static SkyKey getSkyKeyForSubdir( glob.getPackageRoot(), glob.getSubdir().getRelative(fileName), subdirPattern, - glob.excludeDirs()); + glob.globberOperation()); } } /** - * Returns matches coming from the directory {@code fileName} if appropriate, either an individual - * file or a nested set of files. + * Returns an Object indicating a match was found for the given fileName in the given + * valueRequested. The Object will be one of: * - *

    {@code valueRequested} must be the SkyValue whose key was returned by - * {@link #getSkyKeyForSubdir} for these parameters. + *

      + *
    • {@code null} if no matches for the given parameters exists + *
    • {@code NestedSet} if a match exists, either because we are looking for + * files/directories or the SkyValue is a package and we're globbing for {@link + * Globber.Operation.SUBPACKAGES} + *
    + * + *

    {@code valueRequested} must be the SkyValue whose key was returned by {@link + * #getSkyKeyForSubdir} for these parameters. */ @Nullable private static Object getSubdirMatchesFromSkyValue( - String fileName, - GlobDescriptor glob, - SkyValue valueRequested) { + String fileName, GlobDescriptor glob, SkyValue valueRequested) { if (valueRequested instanceof GlobValue) { return ((GlobValue) valueRequested).getMatches(); - } else { - Preconditions.checkState( - valueRequested instanceof PackageLookupValue, - "%s is not a GlobValue or PackageLookupValue (%s %s)", - valueRequested, - fileName, - glob); - PackageLookupValue packageLookupValue = (PackageLookupValue) valueRequested; - if (packageLookupValue.packageExists()) { - // This is a separate package, so ignore it. - return null; - } else if (packageLookupValue - instanceof PackageLookupValue.IncorrectRepositoryReferencePackageLookupValue) { - // This is a separate repository, so ignore it. - return null; - } else { - return glob.getSubdir().getRelative(fileName); - } } + + Preconditions.checkState( + valueRequested instanceof PackageLookupValue, + "%s is not a GlobValue or PackageLookupValue (%s %s)", + valueRequested, + fileName, + glob); + + PackageLookupValue packageLookupValue = (PackageLookupValue) valueRequested; + if (packageLookupValue + instanceof PackageLookupValue.IncorrectRepositoryReferencePackageLookupValue) { + // This is a separate repository, so ignore it. + return null; + } + + boolean isSubpackagesOp = glob.globberOperation() == Globber.Operation.SUBPACKAGES; + boolean pkgExists = packageLookupValue.packageExists(); + + if (!isSubpackagesOp && pkgExists) { + // We're in our repo and fileName is a package. Since we're not doing SUBPACKAGES listing, we + // do not want to add it to the results. + return null; + } else if (isSubpackagesOp && !pkgExists) { + // We're in our repo and the package exists. Since we're doing SUBPACKAGES listing, we do + // want to add fileName to the results. + return null; + } + + // The fileName should be added to the results of the glob. + return glob.getSubdir().getRelative(fileName); } @Nullable diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java index bcc89fc82ff131..bc7900ab11d475 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java @@ -20,6 +20,7 @@ import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; +import com.google.devtools.build.lib.packages.Globber; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.UnixGlob; @@ -84,7 +85,7 @@ public static GlobDescriptor key( PackageIdentifier packageId, Root packageRoot, String pattern, - boolean excludeDirs, + Globber.Operation globOperation, PathFragment subdir) throws InvalidGlobPatternException { if (pattern.indexOf('?') != -1) { @@ -96,7 +97,7 @@ public static GlobDescriptor key( throw new InvalidGlobPatternException(pattern, error); } - return internalKey(packageId, packageRoot, subdir, pattern, excludeDirs); + return internalKey(packageId, packageRoot, subdir, pattern, globOperation); } /** @@ -110,8 +111,8 @@ static GlobDescriptor internalKey( Root packageRoot, PathFragment subdir, String pattern, - boolean excludeDirs) { - return GlobDescriptor.create(packageId, packageRoot, subdir, pattern, excludeDirs); + Globber.Operation globOperation) { + return GlobDescriptor.create(packageId, packageRoot, subdir, pattern, globOperation); } /** 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 b4e1bac183985a..5a27d6179db333 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 @@ -43,6 +43,7 @@ import com.google.devtools.build.lib.packages.BuildFileNotFoundException; import com.google.devtools.build.lib.packages.CachingPackageLocator; import com.google.devtools.build.lib.packages.Globber; +import com.google.devtools.build.lib.packages.GlobberUtils; import com.google.devtools.build.lib.packages.InvalidPackageNameException; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.NonSkyframeGlobber; @@ -139,6 +140,7 @@ public static class CompiledBuildFile { @Nullable private final Program prog; @Nullable private final ImmutableList globs; @Nullable private final ImmutableList globsWithDirs; + @Nullable private final ImmutableList subpackages; @Nullable private final ImmutableMap generatorMap; @Nullable private final ImmutableMap predeclared; @@ -151,11 +153,13 @@ boolean ok() { Program prog, ImmutableList globs, ImmutableList globsWithDirs, + ImmutableList subpackages, ImmutableMap generatorMap, ImmutableMap predeclared) { this.errors = null; this.prog = prog; this.globs = globs; + this.subpackages = subpackages; this.globsWithDirs = globsWithDirs; this.generatorMap = generatorMap; this.predeclared = predeclared; @@ -167,6 +171,7 @@ boolean ok() { this.prog = null; this.globs = null; this.globsWithDirs = null; + this.subpackages = null; this.generatorMap = null; this.predeclared = null; } @@ -923,9 +928,12 @@ public Set getGlobDepsRequested() { @Override public Token runAsync( - List includes, List excludes, boolean excludeDirs, boolean allowEmpty) + List includes, + List excludes, + Globber.Operation globberOperation, + boolean allowEmpty) throws BadGlobException, InterruptedException { - return delegate.runAsync(includes, excludes, excludeDirs, allowEmpty); + return delegate.runAsync(includes, excludes, globberOperation, allowEmpty); } @Override @@ -1025,10 +1033,11 @@ public Set getGlobDepsRequested() { return ImmutableSet.copyOf(globDepsRequested); } - private SkyKey getGlobKey(String pattern, boolean excludeDirs) throws BadGlobException { + private SkyKey getGlobKey(String pattern, Globber.Operation globberOperation) + throws BadGlobException { try { return GlobValue.key( - packageId, packageRoot, pattern, excludeDirs, PathFragment.EMPTY_FRAGMENT); + packageId, packageRoot, pattern, globberOperation, PathFragment.EMPTY_FRAGMENT); } catch (InvalidGlobPatternException e) { throw new BadGlobException(e.getMessage()); } @@ -1036,13 +1045,16 @@ private SkyKey getGlobKey(String pattern, boolean excludeDirs) throws BadGlobExc @Override public Token runAsync( - List includes, List excludes, boolean excludeDirs, boolean allowEmpty) + List includes, + List excludes, + Globber.Operation globberOperation, + boolean allowEmpty) throws BadGlobException, InterruptedException { LinkedHashSet globKeys = Sets.newLinkedHashSetWithExpectedSize(includes.size()); Map globKeyToPatternMap = Maps.newHashMapWithExpectedSize(includes.size()); for (String pattern : includes) { - SkyKey globKey = getGlobKey(pattern, excludeDirs); + SkyKey globKey = getGlobKey(pattern, globberOperation); globKeys.add(globKey); globKeyToPatternMap.put(globKey, pattern); } @@ -1066,9 +1078,9 @@ public Token runAsync( globsToDelegate.isEmpty() ? null : nonSkyframeGlobber.runAsync( - globsToDelegate, ImmutableList.of(), excludeDirs, allowEmpty); + globsToDelegate, ImmutableList.of(), globberOperation, allowEmpty); return new HybridToken( - globValueMap, globKeys, nonSkyframeIncludesToken, excludes, allowEmpty); + globValueMap, globKeys, nonSkyframeIncludesToken, excludes, globberOperation, allowEmpty); } private static Collection getMissingKeys( @@ -1126,6 +1138,8 @@ private static class HybridToken extends Globber.Token { private final List excludes; + private final Globber.Operation globberOperation; + private final boolean allowEmpty; private HybridToken( @@ -1133,11 +1147,13 @@ private HybridToken( Iterable includesGlobKeys, @Nullable NonSkyframeGlobber.Token nonSkyframeGlobberIncludesToken, List excludes, + Globber.Operation globberOperation, boolean allowEmpty) { this.globValueMap = globValueMap; this.includesGlobKeys = includesGlobKeys; this.nonSkyframeGlobberIncludesToken = nonSkyframeGlobberIncludesToken; this.excludes = excludes; + this.globberOperation = globberOperation; this.allowEmpty = allowEmpty; } @@ -1152,12 +1168,8 @@ private List resolve(NonSkyframeGlobber nonSkyframeGlobber) foundMatch = true; } if (!allowEmpty && !foundMatch) { - throw new BadGlobException( - "glob pattern '" - + ((GlobDescriptor) includeGlobKey.argument()).getPattern() - + "' didn't match anything, but allow_empty is set to False " - + "(the default value of allow_empty can be set with " - + "--incompatible_disallow_empty_glob)."); + GlobberUtils.throwBadGlobExceptionEmptyResult( + ((GlobDescriptor) includeGlobKey.argument()).getPattern(), globberOperation); } } if (nonSkyframeGlobberIncludesToken != null) { @@ -1171,10 +1183,7 @@ private List resolve(NonSkyframeGlobber nonSkyframeGlobber) List result = new ArrayList<>(matches); if (!allowEmpty && result.isEmpty()) { - throw new BadGlobException( - "all files in the glob have been excluded, but allow_empty is set to False " - + "(the default value of allow_empty can be set with " - + "--incompatible_disallow_empty_glob)."); + GlobberUtils.throwBadGlobExceptionAllExcluded(globberOperation); } return result; } @@ -1370,6 +1379,7 @@ private LoadedPackageCacheEntry loadPackage( compiled.prog, compiled.globs, compiled.globsWithDirs, + compiled.subpackages, compiled.predeclared, loadedModules, starlarkBuiltinsValue.starlarkSemantics, @@ -1470,9 +1480,11 @@ private CompiledBuildFile compileBuildFile( // - record the generator_name of each top-level macro call Set globs = new HashSet<>(); Set globsWithDirs = new HashSet<>(); + Set subpackages = new HashSet<>(); Map generatorMap = new HashMap<>(); ImmutableList.Builder errors = ImmutableList.builder(); - if (!PackageFactory.checkBuildSyntax(file, globs, globsWithDirs, generatorMap, errors::add)) { + if (!PackageFactory.checkBuildSyntax( + file, globs, globsWithDirs, subpackages, generatorMap, errors::add)) { return new CompiledBuildFile(errors.build()); } @@ -1520,6 +1532,7 @@ private CompiledBuildFile compileBuildFile( prog, ImmutableList.copyOf(globs), ImmutableList.copyOf(globsWithDirs), + ImmutableList.copyOf(subpackages), ImmutableMap.copyOf(generatorMap), ImmutableMap.copyOf(predeclared)); } diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java index 4776b8d5427249..efdb8e57a86e0a 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java @@ -237,4 +237,41 @@ NoneType exportsFiles(Sequence srcs, Object visibility, Object licenses, Star + "REPOSITORY_NAME.", useStarlarkThread = true) String repositoryName(StarlarkThread thread) throws EvalException; + + @StarlarkMethod( + name = "subpackages", + doc = + "Returns a new mutable list of every direct subpackage of the current package," + + " regardless of file-system directory depth. List returned is sorted and contains" + + " the names of subpackages relative to the current package. It is advised to" + + " prefer using the methods in bazel_skylib.subpackages module rather than calling" + + " this function directly.", + parameters = { + @Param( + name = "include", + allowedTypes = {@ParamType(type = Sequence.class, generic1 = String.class)}, + positional = false, + named = true, + doc = "The list of glob patterns to include in subpackages scan."), + @Param( + name = "exclude", + allowedTypes = {@ParamType(type = Sequence.class, generic1 = String.class)}, + defaultValue = "[]", + positional = false, + named = true, + doc = "The list of glob patterns to exclude from subpackages scan."), + @Param( + name = "allow_empty", + defaultValue = "False", + positional = false, + named = true, + doc = + "Whether we fail if the call returns an empty list. By default empty list indicates" + + " potential error in BUILD file where the call to subpackages() is" + + " superflous. Setting to true allows this function to succeed in that case.") + }, + useStarlarkThread = true) + Sequence subpackages( + Sequence include, Sequence exclude, boolean allowEmpty, StarlarkThread thread) + throws EvalException, InterruptedException; } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java b/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java index 23ab5ab5e0512d..92f1c331c0b78b 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java @@ -17,8 +17,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; import com.google.common.base.Preconditions; -import com.google.common.base.Predicate; -import com.google.common.base.Predicates; import com.google.common.base.Splitter; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; @@ -60,6 +58,9 @@ *

    Importantly, note that the glob matches are in an unspecified order. */ public final class UnixGlob { + private static final UnixGlobPathDiscriminator DEFAULT_DISCRIMINATOR = + new UnixGlobPathDiscriminator() {}; + private UnixGlob() {} /** Indicates an invalid glob pattern. */ @@ -72,51 +73,46 @@ private BadPattern(String message) { private static List globInternal( Path base, Collection patterns, - boolean excludeDirectories, - Predicate dirPred, + UnixGlobPathDiscriminator pathDiscriminator, FilesystemCalls syscalls, Executor executor) throws IOException, InterruptedException, BadPattern { GlobVisitor visitor = new GlobVisitor(executor); - return visitor.glob(base, patterns, excludeDirectories, dirPred, syscalls); + return visitor.glob(base, patterns, pathDiscriminator, syscalls); } private static List globInternalUninterruptible( Path base, Collection patterns, - boolean excludeDirectories, - Predicate dirPred, + UnixGlobPathDiscriminator pathDiscriminator, FilesystemCalls syscalls, Executor executor) throws IOException, BadPattern { GlobVisitor visitor = new GlobVisitor(executor); - return visitor.globUninterruptible(base, patterns, excludeDirectories, dirPred, syscalls); + return visitor.globUninterruptible(base, patterns, pathDiscriminator, syscalls); } private static long globInternalAndReturnNumGlobTasksForTesting( Path base, Collection patterns, - boolean excludeDirectories, - Predicate dirPred, + UnixGlobPathDiscriminator pathDiscriminator, FilesystemCalls syscalls, Executor executor) throws IOException, InterruptedException, BadPattern { GlobVisitor visitor = new GlobVisitor(executor); - visitor.glob(base, patterns, excludeDirectories, dirPred, syscalls); + visitor.glob(base, patterns, pathDiscriminator, syscalls); return visitor.getNumGlobTasksForTesting(); } private static Future> globAsyncInternal( Path base, Collection patterns, - boolean excludeDirectories, - Predicate dirPred, + UnixGlobPathDiscriminator pathDiscriminator, FilesystemCalls syscalls, Executor executor) throws BadPattern { Preconditions.checkNotNull(executor, "%s %s", base, patterns); - return new GlobVisitor(executor) - .globAsync(base, patterns, excludeDirectories, dirPred, syscalls); + return new GlobVisitor(executor).globAsync(base, patterns, pathDiscriminator, syscalls); } /** @@ -345,8 +341,7 @@ public static Builder forPath(Path path) { public static class Builder { private Path base; private List patterns; - private boolean excludeDirectories; - private Predicate pathFilter; + private UnixGlobPathDiscriminator pathDiscriminator = DEFAULT_DISCRIMINATOR; private Executor executor; private AtomicReference syscalls = new AtomicReference<>(DEFAULT_SYSCALLS); @@ -357,8 +352,6 @@ public static class Builder { public Builder(Path base) { this.base = base; this.patterns = Lists.newArrayList(); - this.excludeDirectories = false; - this.pathFilter = Predicates.alwaysTrue(); } /** @@ -401,14 +394,6 @@ public Builder setFilesystemCalls(AtomicReference sys return this; } - /** - * If set to true, directories are not returned in the glob result. - */ - public Builder setExcludeDirectories(boolean excludeDirectories) { - this.excludeDirectories = excludeDirectories; - return this; - } - /** * Sets the executor to use for parallel glob evaluation. If unset, evaluation is done * in-thread. @@ -418,21 +403,27 @@ public Builder setExecutor(Executor pool) { return this; } - /** - * If set, the given predicate is called for every directory - * encountered. If it returns false, the corresponding item is not - * returned in the output and directories are not traversed either. + * Sets the UnixGlobPathDiscriminator which determines how to handle Path entries encountered + * during glob traversal. The interface determines if Paths should be added to the {@code + * List} results and whether to traverse a given directory during recursion. + * + *

    The UnixGlobPathDiscriminator should only be called with Paths that have been resolved to + * a regular file or regular directory, it will not properly handle symlinks or special files. + * + *

    This is used for handling the previous use case of 'excludeDirectories' where we wish to + * exclude files from the glob and decide which directories to traverse, like skipping sub-dirs + * containing BUILD files. */ - public Builder setDirectoryFilter(Predicate pathFilter) { - this.pathFilter = pathFilter; + public Builder setPathDiscriminator(UnixGlobPathDiscriminator pathDiscriminator) { + this.pathDiscriminator = pathDiscriminator; return this; } /** Executes the glob. */ public List glob() throws IOException, BadPattern { return globInternalUninterruptible( - base, patterns, excludeDirectories, pathFilter, syscalls.get(), executor); + base, patterns, pathDiscriminator, syscalls.get(), executor); } /** @@ -441,14 +432,14 @@ public List glob() throws IOException, BadPattern { * @throws InterruptedException if the thread is interrupted. */ public List globInterruptible() throws IOException, InterruptedException, BadPattern { - return globInternal(base, patterns, excludeDirectories, pathFilter, syscalls.get(), executor); + return globInternal(base, patterns, pathDiscriminator, syscalls.get(), executor); } @VisibleForTesting public long globInterruptibleAndReturnNumGlobTasksForTesting() throws IOException, InterruptedException, BadPattern { return globInternalAndReturnNumGlobTasksForTesting( - base, patterns, excludeDirectories, pathFilter, syscalls.get(), executor); + base, patterns, pathDiscriminator, syscalls.get(), executor); } /** @@ -456,8 +447,7 @@ public long globInterruptibleAndReturnNumGlobTasksForTesting() * non-null argument. */ public Future> globAsync() throws BadPattern { - return globAsyncInternal( - base, patterns, excludeDirectories, pathFilter, syscalls.get(), executor); + return globAsyncInternal(base, patterns, pathDiscriminator, syscalls.get(), executor); } } @@ -522,9 +512,11 @@ private static final class GlobVisitor { /** * Performs wildcard globbing: returns the list of filenames that match any of {@code patterns} - * relative to {@code base}. Directories are traversed if and only if they match {@code - * dirPred}. The predicate is also called for the root of the traversal. The order of the - * returned list is unspecified. + * relative to {@code base}. Directories are traversed if and only if they return true from + * {@code pathDiscriminator.shouldTraverseDirectory}. The predicate is also called for the root + * of the traversal. {@code pathDiscriminator.shouldIncludePathInResult} is called to determine + * if a directory result should be included in the output. The The order of the returned list is + * unspecified. * *

    Patterns may include "*" and "?", but not "[a-z]". * @@ -538,12 +530,11 @@ private static final class GlobVisitor { List glob( Path base, Collection patterns, - boolean excludeDirectories, - Predicate dirPred, + UnixGlobPathDiscriminator pathDiscriminator, FilesystemCalls syscalls) throws IOException, InterruptedException, BadPattern { try { - return globAsync(base, patterns, excludeDirectories, dirPred, syscalls).get(); + return globAsync(base, patterns, pathDiscriminator, syscalls).get(); } catch (ExecutionException e) { Throwable cause = e.getCause(); Throwables.propagateIfPossible(cause, IOException.class); @@ -554,13 +545,12 @@ List glob( List globUninterruptible( Path base, Collection patterns, - boolean excludeDirectories, - Predicate dirPred, + UnixGlobPathDiscriminator pathDiscriminator, FilesystemCalls syscalls) throws IOException, BadPattern { try { return Uninterruptibles.getUninterruptibly( - globAsync(base, patterns, excludeDirectories, dirPred, syscalls)); + globAsync(base, patterns, pathDiscriminator, syscalls)); } catch (ExecutionException e) { Throwable cause = e.getCause(); Throwables.propagateIfPossible(cause, IOException.class); @@ -580,8 +570,7 @@ private static boolean isRecursivePattern(String pattern) { Future> globAsync( Path base, Collection patterns, - boolean excludeDirectories, - Predicate dirPred, + UnixGlobPathDiscriminator pathDiscriminator, FilesystemCalls syscalls) throws BadPattern { FileStatus baseStat; @@ -610,9 +599,10 @@ Future> globAsync( ++numRecursivePatterns; } } - GlobTaskContext context = numRecursivePatterns > 1 - ? new RecursiveGlobTaskContext(splitPattern, excludeDirectories, dirPred, syscalls) - : new GlobTaskContext(splitPattern, excludeDirectories, dirPred, syscalls); + GlobTaskContext context = + numRecursivePatterns > 1 + ? new RecursiveGlobTaskContext(splitPattern, pathDiscriminator, syscalls) + : new GlobTaskContext(splitPattern, pathDiscriminator, syscalls); context.queueGlob(base, baseStat.isDirectory(), 0); } } finally { @@ -657,10 +647,9 @@ public void run() { @Override public String toString() { return String.format( - "%s glob(include=[%s], exclude_directories=%s)", + "%s glob(include=[%s])", base.getPathString(), - "\"" + Joiner.on("\", \"").join(context.patternParts) + "\"", - context.excludeDirectories); + "\"" + Joiner.on("\", \"").join(context.patternParts) + "\""); } }); } @@ -720,18 +709,15 @@ private void decrementAndCheckDone() { /** A context for evaluating all the subtasks of a single top-level glob task. */ private class GlobTaskContext { private final String[] patternParts; - private final boolean excludeDirectories; - private final Predicate dirPred; + private final UnixGlobPathDiscriminator pathDiscriminator; private final FilesystemCalls syscalls; GlobTaskContext( String[] patternParts, - boolean excludeDirectories, - Predicate dirPred, + UnixGlobPathDiscriminator pathDiscriminator, FilesystemCalls syscalls) { this.patternParts = patternParts; - this.excludeDirectories = excludeDirectories; - this.dirPred = dirPred; + this.pathDiscriminator = pathDiscriminator; this.syscalls = syscalls; } @@ -779,10 +765,9 @@ public int hashCode() { private RecursiveGlobTaskContext( String[] patternParts, - boolean excludeDirectories, - Predicate dirPred, + UnixGlobPathDiscriminator pathDiscriminator, FilesystemCalls syscalls) { - super(patternParts, excludeDirectories, dirPred, syscalls); + super(patternParts, pathDiscriminator, syscalls); } @Override @@ -811,14 +796,14 @@ protected void queueGlob(Path base, boolean baseIsDir, int patternIdx) { */ private void reallyGlob(Path base, boolean baseIsDir, int idx, GlobTaskContext context) throws IOException { - if (baseIsDir && !context.dirPred.apply(base)) { + + if (baseIsDir && !context.pathDiscriminator.shouldTraverseDirectory(base)) { + maybeAddResult(context, base, baseIsDir); return; } if (idx == context.patternParts.length) { // Base case. - if (!(context.excludeDirectories && baseIsDir)) { - results.add(base); - } + maybeAddResult(context, base, baseIsDir); return; } @@ -868,6 +853,12 @@ private void reallyGlob(Path base, boolean baseIsDir, int idx, GlobTaskContext c } } + private void maybeAddResult(GlobTaskContext context, Path base, boolean isDirectory) { + if (context.pathDiscriminator.shouldIncludePathInResult(base, isDirectory)) { + results.add(base); + } + } + /** * Process symlinks asynchronously. If we should used readdir(..., Symlinks.FOLLOW), that would * result in a sequential symlink resolution with many file system implementations. If the @@ -894,7 +885,7 @@ private void processFileOrDirectory( if (isDir) { context.queueGlob(path, /* baseIsDir= */ true, idx + (isRecursivePattern ? 0 : 1)); } else if (idx + 1 == context.patternParts.length) { - results.add(path); + maybeAddResult(context, path, /* isDirectory= */ false); } } } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/UnixGlobPathDiscriminator.java b/src/main/java/com/google/devtools/build/lib/vfs/UnixGlobPathDiscriminator.java new file mode 100644 index 00000000000000..04e49b00c10195 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/vfs/UnixGlobPathDiscriminator.java @@ -0,0 +1,41 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.vfs; + +import com.google.errorprone.annotations.CheckReturnValue; + +/** + * Interface that provides details on how UnixGlob should discriminate between different Paths. + * Instances of this interface will be handed either real directories or real files after symlink + * resolution and excluding special files. + */ +@CheckReturnValue +public interface UnixGlobPathDiscriminator { + + /** + * Determine if UnixGlob should enumerate entries in this directory and traverse it during + * recursive globbing. Defaults to true. + */ + default boolean shouldTraverseDirectory(Path path) { + return true; + } + + /** + * Determine if UnixGlob should include the given path in a {@code List} result. Defaults to + * true; + */ + default boolean shouldIncludePathInResult(Path path, boolean isDirectory) { + return true; + } +} diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkNativeModuleApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkNativeModuleApi.java index c22d07c2dec750..b4fd592582fc5f 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkNativeModuleApi.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkNativeModuleApi.java @@ -77,6 +77,13 @@ public String repositoryName(StarlarkThread thread) { return ""; } + @Override + public Sequence subpackages( + Sequence include, Sequence exclude, boolean allowEmpty, StarlarkThread thread) + throws EvalException, InterruptedException { + return StarlarkList.of(thread.mutability()); + } + @Nullable @Override public Object getValue(String name) throws EvalException { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD index 267d082f764627..83cb5eb8349923 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD @@ -122,6 +122,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/exec:bin_tools", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper", "//src/main/java/com/google/devtools/build/lib/packages:exec_group", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/pkgcache", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD index 65c8b002629696..b6a36d5d9c5a85 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BUILD @@ -90,6 +90,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/exec:execution_options", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/rules:repository/local_repository_rule", diff --git a/src/test/java/com/google/devtools/build/lib/packages/BUILD b/src/test/java/com/google/devtools/build/lib/packages/BUILD index 61ef029a9d0bf0..b19ea2294f78e8 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/BUILD +++ b/src/test/java/com/google/devtools/build/lib/packages/BUILD @@ -69,9 +69,12 @@ java_test( "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/exec:test_policy", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper", "//src/main/java/com/google/devtools/build/lib/packages:exec_group", + "//src/main/java/com/google/devtools/build/lib/packages:globber", "//src/main/java/com/google/devtools/build/lib/pkgcache", "//src/main/java/com/google/devtools/build/lib/runtime/commands", + "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data", "//src/main/java/com/google/devtools/build/lib/skyframe:tests_for_target_pattern_value", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization", "//src/main/java/com/google/devtools/build/lib/util", 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 f1b693fe7aceac..93e2d42ddfa660 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 @@ -131,7 +131,7 @@ public final void deleteFiles() throws Exception { @Test public void testIgnoredDirectory() throws Exception { createCache(PathFragment.create("isolated/foo")); - List paths = cache.safeGlobUnsorted("**/*.js", true).get(); + List paths = cache.safeGlobUnsorted("**/*.js", Globber.Operation.FILES).get(); assertPathsAre( paths, "/workspace/isolated/first.js", @@ -142,7 +142,7 @@ public void testIgnoredDirectory() throws Exception { @Test public void testSafeGlob() throws Exception { - List paths = cache.safeGlobUnsorted("*.js", false).get(); + List paths = cache.safeGlobUnsorted("*.js", Globber.Operation.FILES_AND_DIRS).get(); assertPathsAre(paths, "/workspace/isolated/first.js", "/workspace/isolated/second.js"); } @@ -150,7 +150,9 @@ public void testSafeGlob() throws Exception { @Test public void testSafeGlobInvalidPattern() throws Exception { String invalidPattern = "Foo?.txt"; - assertThrows(BadGlobException.class, () -> cache.safeGlobUnsorted(invalidPattern, false).get()); + assertThrows( + BadGlobException.class, + () -> cache.safeGlobUnsorted(invalidPattern, Globber.Operation.FILES_AND_DIRS).get()); } @Test @@ -170,38 +172,53 @@ public void testGetKeySet() throws Exception { assertThat(cache.getKeySet()).isEmpty(); cache.getGlobUnsorted("*.java"); - assertThat(cache.getKeySet()).containsExactly(Pair.of("*.java", false)); + assertThat(cache.getKeySet()) + .containsExactly(Pair.of("*.java", Globber.Operation.FILES_AND_DIRS)); cache.getGlobUnsorted("*.java"); - assertThat(cache.getKeySet()).containsExactly(Pair.of("*.java", false)); + assertThat(cache.getKeySet()) + .containsExactly(Pair.of("*.java", Globber.Operation.FILES_AND_DIRS)); cache.getGlobUnsorted("*.js"); - assertThat(cache.getKeySet()).containsExactly(Pair.of("*.java", false), Pair.of("*.js", false)); + assertThat(cache.getKeySet()) + .containsExactly( + Pair.of("*.java", Globber.Operation.FILES_AND_DIRS), + Pair.of("*.js", Globber.Operation.FILES_AND_DIRS)); - cache.getGlobUnsorted("*.java", true); - assertThat(cache.getKeySet()).containsExactly(Pair.of("*.java", false), Pair.of("*.js", false), - Pair.of("*.java", true)); + cache.getGlobUnsorted("*.java", Globber.Operation.FILES); + assertThat(cache.getKeySet()) + .containsExactly( + Pair.of("*.java", Globber.Operation.FILES_AND_DIRS), + Pair.of("*.js", Globber.Operation.FILES_AND_DIRS), + Pair.of("*.java", Globber.Operation.FILES)); assertThrows(BadGlobException.class, () -> cache.getGlobUnsorted("invalid?")); - assertThat(cache.getKeySet()).containsExactly(Pair.of("*.java", false), Pair.of("*.js", false), - Pair.of("*.java", true)); + assertThat(cache.getKeySet()) + .containsExactly( + Pair.of("*.java", Globber.Operation.FILES_AND_DIRS), + Pair.of("*.js", Globber.Operation.FILES_AND_DIRS), + Pair.of("*.java", Globber.Operation.FILES)); cache.getGlobUnsorted("foo/first.*"); - assertThat(cache.getKeySet()).containsExactly(Pair.of("*.java", false), Pair.of("*.java", true), - Pair.of("*.js", false), Pair.of("foo/first.*", false)); + assertThat(cache.getKeySet()) + .containsExactly( + Pair.of("*.java", Globber.Operation.FILES_AND_DIRS), + Pair.of("*.java", Globber.Operation.FILES), + Pair.of("*.js", Globber.Operation.FILES_AND_DIRS), + Pair.of("foo/first.*", Globber.Operation.FILES_AND_DIRS)); } @Test public void testGlob() throws Exception { - assertEmpty(cache.globUnsorted(list("*.java"), NONE, false, true)); + assertEmpty(cache.globUnsorted(list("*.java"), NONE, Globber.Operation.FILES, true)); - assertThat(cache.globUnsorted(list("*.*"), NONE, false, true)) + assertThat(cache.globUnsorted(list("*.*"), NONE, Globber.Operation.FILES, true)) .containsExactly("first.js", "first.txt", "second.js", "second.txt"); - assertThat(cache.globUnsorted(list("*.*"), list("first.js"), false, true)) + assertThat(cache.globUnsorted(list("*.*"), list("first.js"), Globber.Operation.FILES, true)) .containsExactly("first.txt", "second.js", "second.txt"); - assertThat(cache.globUnsorted(list("*.txt", "first.*"), NONE, false, true)) + assertThat(cache.globUnsorted(list("*.txt", "first.*"), NONE, Globber.Operation.FILES, true)) .containsExactly("first.txt", "second.txt", "first.js"); } @@ -214,13 +231,17 @@ public void testRecursiveGlobDoesNotMatchSubpackage() throws Exception { @Test public void testSingleFileExclude_star() throws Exception { - assertThat(cache.globUnsorted(list("*"), list("first.txt"), false, true)) + assertThat( + cache.globUnsorted( + list("*"), list("first.txt"), Globber.Operation.FILES_AND_DIRS, true)) .containsExactly("BUILD", "bar", "first.js", "foo", "second.js", "second.txt"); } @Test public void testSingleFileExclude_starStar() throws Exception { - assertThat(cache.globUnsorted(list("**"), list("first.txt"), false, true)) + assertThat( + cache.globUnsorted( + list("**"), list("first.txt"), Globber.Operation.FILES_AND_DIRS, true)) .containsExactly( "BUILD", "bar", @@ -236,48 +257,78 @@ public void testSingleFileExclude_starStar() throws Exception { @Test public void testExcludeAll_star() throws Exception { - assertThat(cache.globUnsorted(list("*"), list("*"), false, true)).isEmpty(); + assertThat(cache.globUnsorted(list("*"), list("*"), Globber.Operation.FILES_AND_DIRS, true)) + .isEmpty(); } @Test public void testExcludeAll_star_noMatchesAnyway() throws Exception { - assertThat(cache.globUnsorted(list("nope"), list("*"), false, true)).isEmpty(); + assertThat(cache.globUnsorted(list("nope"), list("*"), Globber.Operation.FILES_AND_DIRS, true)) + .isEmpty(); } @Test public void testExcludeAll_starStar() throws Exception { - assertThat(cache.globUnsorted(list("**"), list("**"), false, true)).isEmpty(); + assertThat(cache.globUnsorted(list("**"), list("**"), Globber.Operation.FILES_AND_DIRS, true)) + .isEmpty(); } @Test public void testExcludeAll_manual() throws Exception { - assertThat(cache.globUnsorted(list("**"), list("*", "*/*", "*/*/*"), false, true)).isEmpty(); + assertThat( + cache.globUnsorted( + list("**"), list("*", "*/*", "*/*/*"), Globber.Operation.FILES_AND_DIRS, true)) + .isEmpty(); } @Test public void testSingleFileExcludeDoesntMatch() throws Exception { - assertThat(cache.globUnsorted(list("first.txt"), list("nope.txt"), false, true)) + assertThat( + cache.globUnsorted( + list("first.txt"), list("nope.txt"), Globber.Operation.FILES_AND_DIRS, true)) .containsExactly("first.txt"); } @Test public void testExcludeDirectory() throws Exception { - assertThat(cache.globUnsorted(list("foo/*"), NONE, true, true)) + assertThat(cache.globUnsorted(list("foo/*"), NONE, Globber.Operation.FILES, true)) .containsExactly("foo/first.js", "foo/second.js"); - assertThat(cache.globUnsorted(list("foo/*"), list("foo"), false, true)) + assertThat( + cache.globUnsorted(list("foo/*"), list("foo"), Globber.Operation.FILES_AND_DIRS, true)) .containsExactly("foo/first.js", "foo/second.js"); } @Test public void testChildGlobWithChildExclude() throws Exception { - assertThat(cache.globUnsorted(list("foo/*"), list("foo/*"), false, true)).isEmpty(); assertThat( - cache.globUnsorted(list("foo/first.js", "foo/second.js"), list("foo/*"), false, true)) + cache.globUnsorted( + list("foo/*"), list("foo/*"), Globber.Operation.FILES_AND_DIRS, true)) + .isEmpty(); + assertThat( + cache.globUnsorted( + list("foo/first.js", "foo/second.js"), + list("foo/*"), + Globber.Operation.FILES_AND_DIRS, + true)) .isEmpty(); - assertThat(cache.globUnsorted(list("foo/first.js"), list("foo/first.js"), false, true)) + assertThat( + cache.globUnsorted( + list("foo/first.js"), list("foo/first.js"), Globber.Operation.FILES_AND_DIRS, true)) + .isEmpty(); + assertThat( + cache.globUnsorted( + list("foo/first.js"), list("*/first.js"), Globber.Operation.FILES_AND_DIRS, true)) .isEmpty(); - assertThat(cache.globUnsorted(list("foo/first.js"), list("*/first.js"), false, true)).isEmpty(); - assertThat(cache.globUnsorted(list("foo/first.js"), list("*/*"), false, true)).isEmpty(); + assertThat( + cache.globUnsorted( + list("foo/first.js"), list("*/*"), Globber.Operation.FILES_AND_DIRS, true)) + .isEmpty(); + } + + @Test + public void testSubpackages() throws Exception { + assertThat(cache.globUnsorted(list("**"), list(), Globber.Operation.SUBPACKAGES, true)) + .containsExactly("sub"); } private void assertEmpty(Collection glob) { diff --git a/src/test/java/com/google/devtools/build/lib/packages/NativeGlobTest.java b/src/test/java/com/google/devtools/build/lib/packages/NativeGlobTest.java new file mode 100644 index 00000000000000..19e40a93995673 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/packages/NativeGlobTest.java @@ -0,0 +1,161 @@ +// Copyright 2021 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.packages; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; +import com.google.devtools.build.lib.vfs.ModifiedFileSet; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.Root; +import java.io.IOException; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@code native.glob} function. */ +@RunWith(JUnit4.class) +public class NativeGlobTest extends BuildViewTestCase { + + @Test + public void glob_simple() throws Exception { + makeFile("test/starlark/file1.txt"); + makeFile("test/starlark/file2.txt"); + makeFile("test/starlark/file3.txt"); + + makeGlobFilegroup("test/starlark/BUILD", "glob(['*'])"); + + assertAttrLabelList( + "//test/starlark:files", + "srcs", + ImmutableList.of( + "//test/starlark:BUILD", + "//test/starlark:file1.txt", + "//test/starlark:file2.txt", + "//test/starlark:file3.txt")); + } + + @Test + public void glob_not_empty() throws Exception { + + makeGlobFilegroup("test/starlark/BUILD", "glob(['foo*'], allow_empty=False)"); + + AssertionError e = + assertThrows( + AssertionError.class, + () -> assertAttrLabelList("//test/starlark:files", "srcs", ImmutableList.of())); + assertThat(e).hasMessageThat().contains("allow_empty"); + } + + @Test + public void glob_simple_subdirs() throws Exception { + makeFile("test/starlark/sub/file1.txt"); + makeFile("test/starlark/sub2/file2.txt"); + makeFile("test/starlark/sub3/file3.txt"); + + makeGlobFilegroup("test/starlark/BUILD", "glob(['**'])"); + + assertAttrLabelList( + "//test/starlark:files", + "srcs", + ImmutableList.of( + "//test/starlark:BUILD", + "//test/starlark:sub/file1.txt", + "//test/starlark:sub2/file2.txt", + "//test/starlark:sub3/file3.txt")); + } + + @Test + public void glob_incremental() throws Exception { + makeFile("test/starlark/file1.txt"); + makeGlobFilegroup("test/starlark/BUILD", "glob(['**'])"); + + assertAttrLabelList( + "//test/starlark:files", + "srcs", + ImmutableList.of("//test/starlark:BUILD", "//test/starlark:file1.txt")); + + scratch.file("test/starlark/file2.txt"); + scratch.file("test/starlark/sub/subfile3.txt"); + + // Poke SkyFrame to tell it what changed. + invalidateSkyFrameFiles( + "test/starlark", "test/starlark/file2.txt", "test/starlark/sub/subfile3.txt"); + + assertAttrLabelList( + "//test/starlark:files", + "srcs", + ImmutableList.of( + "//test/starlark:BUILD", + "//test/starlark:file1.txt", + "//test/starlark:file2.txt", + "//test/starlark:sub/subfile3.txt")); + } + + /** + * Constructs a BUILD file containing a single rule with uses glob() to list files look for a rule + * called :files in it. + */ + private void makeGlobFilegroup(String buildPath, String glob) throws IOException { + scratch.file(buildPath, "filegroup(", " name = 'files',", " srcs = " + glob, ")"); + } + + private void assertAttrLabelList(String target, String attrName, List expectedLabels) + throws Exception { + ConfiguredTargetAndData cfgTarget = getConfiguredTargetAndData(target); + assertThat(cfgTarget).isNotNull(); + + ImmutableList