Update the glob documentation to reflect a semantic change made a very long time ago where glob(['**'], exclude_directories = 0) doesn't match the package's directory. Also add tests for this behavior.
Also update Skyframe globbing to have these semantics. Any discrepancy has always been problematic, but now that we have Skyframe-hybrid globbing it's a lot more dangerous and consequential.
Alternatives considered: do this the other way around (keep the stale documentation as-is and instead update legacy globbing). This would potentially require changing existing usages from stuff like 'data = glob(["**"], exclude_directories = 0)' to 'data = [x for x in glob(["**"], exclude_directories = 0) where x != '']'. I think this is too messy, so long as there is a valid use-case for globs matching directories in the first place.
--
MOS_MIGRATED_REVID=115511504
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
index 8cd8003..c985256 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
@@ -570,6 +570,46 @@
assertThat(newValue.getPackage()).isNotSameAs(value.getPackage());
}
+ // Regression test for Skyframe globbing incorrectly matching the package's directory path on
+ // 'glob(['**'], exclude_directories = 0)'. We test for this directly by triggering
+ // hybrid globbing (gives coverage for both legacy globbing and skyframe globbing).
+ @Test
+ public void testRecursiveGlobNeverMatchesPackageDirectory() throws Exception {
+ scratch.file("foo/BUILD",
+ "[sh_library(name = x + '-matched') for x in glob(['**'], exclude_directories = 0)]");
+ scratch.file("foo/bar");
+
+ getSkyframeExecutor().preparePackageLoading(
+ new PathPackageLocator(outputBase, ImmutableList.of(rootDirectory)),
+ ConstantRuleVisibility.PUBLIC, true,
+ 7, "", UUID.randomUUID());
+
+ SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("foo"));
+ PackageValue value = validPackage(skyKey);
+ assertFalse(value.getPackage().containsErrors());
+ assertThat(value.getPackage().getTarget("bar-matched").getName()).isEqualTo("bar-matched");
+ try {
+ value.getPackage().getTarget("-matched");
+ fail();
+ } catch (NoSuchTargetException expected) {
+ }
+
+ scratch.overwriteFile("foo/BUILD",
+ "[sh_library(name = x + '-matched') for x in glob(['**'], exclude_directories = 0)]",
+ "#some-irrelevant-comment");
+ getSkyframeExecutor().invalidateFilesUnderPathForTesting(reporter,
+ ModifiedFileSet.builder().modify(new PathFragment("foo/BUILD")).build(), rootDirectory);
+
+ value = validPackage(skyKey);
+ assertFalse(value.getPackage().containsErrors());
+ assertThat(value.getPackage().getTarget("bar-matched").getName()).isEqualTo("bar-matched");
+ try {
+ value.getPackage().getTarget("-matched");
+ fail();
+ } catch (NoSuchTargetException expected) {
+ }
+ }
+
private static class CustomInMemoryFs extends InMemoryFileSystem {
private abstract static class FileStatusOrException {
abstract FileStatus get() throws IOException;
