Automated rollback of commit e791618f27f8ba63240fa8e300bae0dd4014cf80.
PiperOrigin-RevId: 589948907
Change-Id: I4bcc84cf07893b4c5124581d8ad33ab2f44ad64f
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 caaea0f..5e92910 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
@@ -390,8 +390,7 @@
The <code>include</code> and <code>exclude</code> lists contain path patterns
that are relative to the current package. Every pattern may consist of one or
more path segments. As usual with Unix paths, these segments are separated by
-<code>/</code>. The segments in the pattern are matched against the segments of
-the path. Segments may contain the <code>*</code> wildcard: this matches
+<code>/</code>. Segments may contain the <code>*</code> wildcard: this matches
any substring in the path segment (even the empty substring), excluding the
directory separator <code>/</code>. This wildcard can be used multiple times
within one path segment. Additionally, the <code>**</code> wildcard can match
@@ -402,33 +401,26 @@
Examples:
<ul>
<li><code>foo/bar.txt</code> matches exactly the <code>foo/bar.txt</code> file
-in this package (unless <code>foo/</code> is a subpackage)</li>
+in this package</li>
<li><code>foo/*.txt</code> matches every file in the <code>foo/</code> directory
-if the file ends with <code>.txt</code> (unless <code>foo/</code> is a
-subpackage)</li>
-<li><code>foo/*</code> matches every file in the <code>foo/</code> directory,
-(unless <code>foo/</code> is a subpackage). It does not match <code>foo</code>
-directory itself.</li>
+if the file ends with
+<code>.txt</code> (unless <code>foo/</code> is a subpackage)</li>
<li><code>foo/a*.htm*</code> matches every file in the <code>foo/</code>
directory that starts with <code>a</code>, then has an arbitrary string (could
-be empty), then has <code>.htm</code>, and ends with another arbitrary string
-(unless <code>foo/</code> is a subpackage) ; such as <code>foo/axx.htm</code>
-and <code>foo/a.html</code> or <code>foo/axxx.html</code></li>
+be empty), then has <code>.htm</code>, and ends with another arbitrary string;
+such as <code>foo/axx.htm</code> and <code>foo/a.html</code> or
+<code>foo/axxx.html</code></li>
<li><code>**/a.txt</code> matches every <code>a.txt</code> file in every
-non-subpackage subdirectory of this package</li>
+subdirectory of this package</li>
<li><code>**/bar/**/*.txt</code> matches every <code>.txt</code> file in every
-non-subpackage subdirectory of this package, if at least one directory on the
-resulting path is called <code>bar</code>, such as
-<code>xxx/bar/yyy/zzz/a.txt</code> or <code>bar/a.txt</code> (remember that
-<code>**</code> also matches zero segments) or <code>bar/zzz/a.txt</code></li>
-<li><code>**</code> matches every file in every non-subpackage subdirectory of
-this package</li>
-<li><code>foo/**</code> matches every file in every non-subpackage subdirectory
-under package's first level subdirectory <code>foo/</code></li>
+subdirectory of this package, if at least one directory on the resulting path is
+called <code>bar</code>, such as <code>xxx/bar/yyy/zzz/a.txt</code> or
+<code>bar/a.txt</code> (remember that <code>**</code> also matches zero
+segments) or <code>bar/zzz/a.txt</code></li>
+<li><code>**</code> matches every file in every subdirectory of this
+package</li>
<li><code>foo**/a.txt</code> is an invalid pattern, because <code>**</code> must
stand on its own as a segment</li>
-<li><code>foo/</code> is an invalid pattern, because the second segment defined
-after <code>/</code> is an empty string</li>
</ul>
<p>
@@ -745,42 +737,16 @@
# The following BUILD files exist:
# foo/BUILD
# foo/bar/baz/BUILD
-# foo/bar/but/bad/BUILD
# foo/sub/BUILD
# foo/sub/deeper/BUILD
#
# In foo/BUILD a call to
-subs1 = subpackages(include = ["**"])
+subs = subpackages(include = ["**"])
-# results in subs1 == ["sub", "bar/baz", "bar/but/bad"]
+# results in subs == ["sub", "bar/baz"]
#
# 'sub/deeper' is not included because it is a subpackage of 'foo/sub' not of
# 'foo'
-
-subs2 = subpackages(include = ["bar/*"])
-# results in subs2 = ["bar/baz"]
-#
-# Since 'bar' is not a subpackage itself, bazel looks for any subpackages under
-# all first level subdirectories of 'bar'.
-
-subs3 = subpackages(include = ["bar/**"])
-# results in subs3 = ["bar/baz", "bar/but/bad"]
-#
-# Since bar is not a subpackage itself, bazel looks for any subpackages which
-# are (1) under all subdirectories of 'bar' which can be at any level, (2) not a
-# subpackage of another subpackages.
-
-subs4 = subpackages(include = ["sub"])
-subs5 = subpackages(include = ["sub/*"])
-subs6 = subpackages(include = ["sub/**"])
-# results in subs4 = ["sub"]
-# results in both subs5 and subs6 being [].
-#
-# In subs4, expression "sub" checks whether 'foo/sub' is a package (i.e. is a
-# subpackage of 'foo').
-# In subs5 and subs5, "sub/*" and "sub/**" try to look for subpackages under
-# directory 'foo/sub'. Since 'foo/sub' is already a subpackage itself, the
-# subdirectory will not be traversed anymore.
</pre>
<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 0fa4c79..bb3346c 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
@@ -78,22 +78,6 @@
}
}
- String pattern = glob.getPattern();
- // Split off the first path component of the pattern.
- int slashPos = pattern.indexOf('/');
- String patternHead;
- String patternTail;
- if (slashPos == -1) {
- patternHead = pattern;
- patternTail = null;
- } else {
- // Substrings will share the backing array of the original glob string. That should be fine.
- patternHead = pattern.substring(0, slashPos);
- patternTail = pattern.substring(slashPos + 1);
- }
-
- boolean globMatchesBareFile = patternTail == null;
-
// 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)) {
@@ -113,11 +97,11 @@
// defines another package, so glob expansion should not descend into
// that subdir.
//
- // For SUBPACKAGES, we should check whether all pattern splits have been matched so that
- // this is 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 && globMatchesBareFile) {
+ // 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.<PathFragment>stableOrder()
.add(subDirFragment.relativeTo(glob.getPackageId().getPackageFragment()))
@@ -131,8 +115,24 @@
}
}
+ String pattern = glob.getPattern();
+ // Split off the first path component of the pattern.
+ int slashPos = pattern.indexOf('/');
+ String patternHead;
+ String patternTail;
+ if (slashPos == -1) {
+ patternHead = pattern;
+ patternTail = null;
+ } else {
+ // Substrings will share the backing array of the original glob string. That should be fine.
+ patternHead = pattern.substring(0, slashPos);
+ patternTail = pattern.substring(slashPos + 1);
+ }
+
NestedSetBuilder<PathFragment> matches = NestedSetBuilder.stableOrder();
+ boolean globMatchesBareFile = patternTail == null;
+
RootedPath dirRootedPath = RootedPath.toRootedPath(glob.getPackageRoot(), dirPathFragment);
if (containsGlobs(patternHead)) {
// Pattern contains globs, so a directory listing is required.
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 0845f1f..4272102 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
@@ -737,13 +737,9 @@
*/
private void reallyGlob(Path base, boolean baseIsDir, int idx, GlobTaskContext context)
throws IOException {
- // Make sure that `patternParts` index is not out of bounds.
- if (idx >= context.patternParts.length) {
- throw new IllegalArgumentException(
- String.format(
- "non skyframe glob index runs out of bounds. glob pattern = %s; current base = %s;"
- + " index = %d",
- String.join("/", context.patternParts), base, idx));
+ if (idx == context.patternParts.length) { // Base case.
+ maybeAddResult(context, base, baseIsDir);
+ return;
}
// Do an early readdir() call here if the pattern contains a wildcard (* or ?). The reason is
@@ -758,6 +754,11 @@
dents = context.syscalls.readdir(base);
}
+ if (baseIsDir && !context.pathDiscriminator.shouldTraverseDirectory(base)) {
+ maybeAddResult(context, base, baseIsDir);
+ return;
+ }
+
if (!baseIsDir) {
// Nothing to find here.
return;
@@ -766,13 +767,7 @@
// ** is special: it can match nothing at all.
// For example, x/** matches x, **/y matches y, and x/**/y matches x/y.
if (isRecursivePattern(pattern)) {
- if (idx + 1 == context.patternParts.length) {
- // If ** is the last pattern, decide whether base is a matching result.
- maybeAddResult(context, base, baseIsDir);
- } else {
- // If there are patterns trailing this **, enqueue the next Glob.
- context.queueGlob(base, baseIsDir, idx + 1);
- }
+ context.queueGlob(base, baseIsDir, idx + 1);
}
if (!patternContainsWildcard) {
@@ -783,7 +778,8 @@
// The file is a dangling symlink, fifo, does not exist, etc.
return;
}
- processFileOrDirectory(child, status.isDirectory(), idx, context);
+
+ context.queueGlob(child, status.isDirectory(), idx + 1);
return;
}
@@ -834,18 +830,10 @@
private void processFileOrDirectory(
Path path, boolean isDir, int idx, GlobTaskContext context) {
boolean isRecursivePattern = isRecursivePattern(context.patternParts[idx]);
- int nextIdx = idx + (isRecursivePattern ? 0 : 1);
-
- if (isDir
- && nextIdx < context.patternParts.length
- && context.pathDiscriminator.shouldTraverseDirectory(path)) {
- // Call queueGlob iff (1) path is a directory, (2) next index is in-bound and (3) no
- // subpackage crossing exists.
- context.queueGlob(path, /* baseIsDir= */ true, nextIdx);
+ if (isDir) {
+ context.queueGlob(path, /* baseIsDir= */ true, idx + (isRecursivePattern ? 0 : 1));
} else if (idx + 1 == context.patternParts.length) {
- // All glob patterns have been matched, so we should decide whether the path is a matching
- // result.
- maybeAddResult(context, path, isDir);
+ maybeAddResult(context, path, /* isDirectory= */ false);
}
}
}
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 3b6a3f5..db5b460 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
@@ -21,7 +21,6 @@
import com.google.devtools.build.lib.actions.ThreadStateReceiver;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.packages.Globber.BadGlobException;
-import com.google.devtools.build.lib.packages.Globber.Operation;
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.Path;
@@ -339,23 +338,11 @@
}
@Test
- public void testSubpackages_noWildcard() throws Exception {
- assertThat(cache.globUnsorted(list("sub/sub.js"), list(), Operation.SUBPACKAGES, true))
- .isEmpty();
- }
-
- @Test
- public void testSubpackages_simpleDoubleStar() throws Exception {
+ public void testSubpackages() throws Exception {
assertThat(cache.globUnsorted(list("**"), list(), Globber.Operation.SUBPACKAGES, true))
.containsExactly("sub");
}
- @Test
- public void testSubpackages_doubleStarWithTrailingPattern() throws Exception {
- assertThat(cache.globUnsorted(list("**/bar"), list(), Globber.Operation.SUBPACKAGES, true))
- .isEmpty();
- }
-
private void assertEmpty(Collection<?> glob) {
assertThat(glob).isEmpty();
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/NativeSubpackagesTest.java b/src/test/java/com/google/devtools/build/lib/packages/NativeSubpackagesTest.java
index ceb9ce8..91f3410 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/NativeSubpackagesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/NativeSubpackagesTest.java
@@ -50,7 +50,7 @@
@Test
public void subpackages_simple_include() throws Exception {
- makeSubpackageFileGroup("test/starlark/BUILD", "sub1", null, null);
+ makeSubpackageFileGroup("test/starlark/BUILD", "sub1/**", null, null);
makeFilesSubPackage("test/starlark/sub");
makeFilesSubPackage("test/starlark/sub1");
@@ -232,7 +232,9 @@
@Test
public void includeValidMatchSubdir() throws Exception {
scratch.file("foo/subdir/BUILD");
- scratch.file("foo/BUILD", "[sh_library(name = p) for p in subpackages(include = ['subdir'])]");
+ scratch.file(
+ "foo/BUILD", "[sh_library(name = p) for p in subpackages(include = ['subdir/*'])]");
+
getConfiguredTargetAndData("//foo:subdir");
}
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 e7e6f9a..320b8e8 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
@@ -899,11 +899,6 @@
}
@Test
- public void subpackages_doubleStarPatternWithNamedChild() throws Exception {
- assertSubpackageMatches("**/bar");
- }
-
- @Test
public void subpackages_oneLevelDeep() throws Exception {
makeEmptyPackage("base/sub");
makeEmptyPackage("base/sub2");
diff --git a/src/test/java/com/google/devtools/build/lib/vfs/RecursiveGlobTest.java b/src/test/java/com/google/devtools/build/lib/vfs/RecursiveGlobTest.java
index 7180aed..4b98faa 100644
--- a/src/test/java/com/google/devtools/build/lib/vfs/RecursiveGlobTest.java
+++ b/src/test/java/com/google/devtools/build/lib/vfs/RecursiveGlobTest.java
@@ -150,7 +150,7 @@
// regressions. In other words, if you're a developer reading this comment because this test
// case is failing, you should be very sure you know what you're doing before you change the
// expectation of the test.
- assertThat(numGlobTasks).isEqualTo(14);
+ assertThat(numGlobTasks).isEqualTo(28);
}
private void assertIllegalWildcard(String pattern)
