diff options
5 files changed, 66 insertions, 3 deletions
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 e6d89c0054..b1b94dc40b 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 @@ -454,6 +454,15 @@ There are several important limitations and caveats: If a rule and a source file with the same name both exist in the package, the glob will return the outputs of the rule instead of the source file. </li> + + <li> + The "**" wildcard has one corner case: the pattern + <code>"**"</code> doesn't match the package's directory path. That is to + say, <code>glob(["**"], exclude_directories = 0)</code> matches all files + and directories transitively strictly under the current package's directory + (but of course not going into directories of subpackages - see the previous + note about that). + </li> </ol> <p> 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 e1c877f455..e233cdfdd5 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 @@ -90,7 +90,8 @@ public final class GlobFunction implements SkyFunction { // "**" also matches an empty segment, so try the case where it is not present. if ("**".equals(patternHead)) { if (patternTail == null) { - if (!glob.excludeDirs()) { + // Recursive globs aren't supposed to match the package's directory. + if (!glob.excludeDirs() && !globSubdir.equals(PathFragment.EMPTY_FRAGMENT)) { matches.add(globSubdir); } } else { diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java index d8d799afbc..d9c175bafe 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java @@ -590,6 +590,21 @@ public class PackageFactoryTest extends PackageFactoryTestBase { assertEvaluates( pkg, ImmutableList.of( + "BUILD", + "a.cc", + "foo", + "foo/bar.cc", + "foo/foo.cc", + "foo/wiz", + "foo/wiz/bam.cc", + "foo/wiz/bum.cc", + "foo/wiz/quid", + "foo/wiz/quid/gav.cc"), + "**"); + + assertEvaluates( + pkg, + ImmutableList.of( "a.cc", "foo/bar.cc", "foo/foo.cc", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java index 0246f864e2..8fc2eb3393 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java @@ -458,7 +458,6 @@ public abstract class GlobFunctionTest { public void testDoubleStar() throws Exception { assertGlobMatches( "**", - "", "BUILD", "a1", "a1/b1", @@ -487,7 +486,6 @@ public abstract class GlobFunctionTest { public void testDoubleDoubleStar() throws Exception { assertGlobMatches( "**/**", - "", "BUILD", "a1", "a1/b1", 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 8cd8003f26..c985256e34 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 @@ public class PackageFunctionTest extends BuildViewTestCase { 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; |