aboutsummaryrefslogtreecommitdiffhomepage
path: root/src
diff options
context:
space:
mode:
authorGravatar Nathan Harmata <nharmata@google.com>2016-02-25 01:12:22 +0000
committerGravatar Philipp Wollermann <philwo@google.com>2016-02-25 14:14:35 +0000
commit86c319e313529b52da0f0fc6062c27a7e75afe5b (patch)
tree8ae5b90ffc8fa2783c53b4af75bebe53769747b2 /src
parent63da2e7998720cf20d35a961e316c786f3f4ec87 (diff)
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
Diffstat (limited to 'src')
-rw-r--r--src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm9
-rw-r--r--src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java3
-rw-r--r--src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java15
-rw-r--r--src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java2
-rw-r--r--src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java40
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;