Automated rollback of commit 07eb2d77f94f9e11e64082dab9581ad7acf44dc7.
*** Reason for rollback ***
Re-fix the `subpackages()` bug when considering a corner case
In the initial change, we have introduced another bug:
Consider computing `subpackages(["sub/**"])` while `sub/BUILD` exists, we should return "sub", which is the behavior of file/directory glob matching. However, the initial fix returns EMPTY in this case.
This change also adds more test coverage for calling `subpackages(["sub/**"])` when `sub` is a subpackage.
PiperOrigin-RevId: 591917000
Change-Id: Iab41cc59b0da7f7794a90cd10aaac8900ee0d79b
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 db5b460..0469085 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
@@ -338,11 +338,54 @@
}
@Test
- public void testSubpackages() throws Exception {
+ public void testSubpackages_noWildcard() throws Exception {
+ assertThat(cache.globUnsorted(list("sub/sub.js"), list(), Globber.Operation.SUBPACKAGES, true))
+ .isEmpty();
+ }
+
+ @Test
+ public void testSubpackages_simpleDoubleStar() throws Exception {
assertThat(cache.globUnsorted(list("**"), list(), Globber.Operation.SUBPACKAGES, true))
.containsExactly("sub");
}
+ @Test
+ public void testSubpackages_onlySub() throws Exception {
+ assertThat(cache.globUnsorted(list("sub"), list(), Globber.Operation.SUBPACKAGES, true))
+ .containsExactly("sub");
+ }
+
+ @Test
+ public void testSubpackages_singleStarsAfterSub() throws Exception {
+ assertThat(cache.globUnsorted(list("sub/*"), list(), Globber.Operation.SUBPACKAGES, true))
+ .isEmpty();
+ }
+
+ @Test
+ public void testSubpackages_doubleStarsAfterSub() throws Exception {
+ assertThat(cache.globUnsorted(list("sub/**"), list(), Globber.Operation.SUBPACKAGES, true))
+ .containsExactly("sub");
+ }
+
+ @Test
+ public void testSubpackages_twoDoubleStarsAfterSub() throws Exception {
+ // Both `**`s are considered to match no path fragments.
+ assertThat(cache.globUnsorted(list("sub/**/**"), list(), Globber.Operation.SUBPACKAGES, true))
+ .containsExactly("sub");
+ }
+
+ @Test
+ public void testSubpackages_doubleStarsAndOtherPathAfterSub() throws Exception {
+ assertThat(cache.globUnsorted(list("sub/**/foo"), list(), Globber.Operation.SUBPACKAGES, true))
+ .isEmpty();
+ }
+
+ @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 91f3410..f7b41a6 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
@@ -26,15 +26,16 @@
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 com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
import java.io.IOException;
import java.util.List;
import java.util.stream.Collectors;
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
/** Tests for {@code native.subpackages} function. */
-@RunWith(JUnit4.class)
+@RunWith(TestParameterInjector.class)
public class NativeSubpackagesTest extends BuildViewTestCase {
private static final String ALL_SUBDIRS = "**";
@@ -50,7 +51,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,19 +233,26 @@
@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");
}
@Test
- public void includeValidSubMatchSubdir() throws Exception {
+ public void includeValidSubMatchSubdir(
+ @TestParameter({
+ "subdir/*/deeper",
+ "subdir/sub*/deeper",
+ "subdir/**",
+ "subdir/*/deeper/**",
+ "subdir/**/deeper/**"
+ })
+ String expression)
+ throws Exception {
makeFilesSubPackage("test/starlark/subdir/sub/deeper");
makeFilesSubPackage("test/starlark/subdir/sub2/deeper");
makeFilesSubPackage("test/starlark/subdir/sub3/deeper");
- makeSubpackageFileGroup("test/starlark/BUILD", "subdir/*/deeper", null, null);
+ makeSubpackageFileGroup("test/starlark/BUILD", expression, null, null);
assertAttrLabelList(
"//test/starlark:files",
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 320b8e8..0b0bd54 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
@@ -73,7 +73,9 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
+import java.util.Arrays;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.UUID;
@@ -106,7 +108,7 @@
private static final PackageIdentifier PKG_ID = PackageIdentifier.createInMainRepo("pkg");
@Before
- public final void setUp() throws Exception {
+ public final void setUp() throws Exception {
fs = new CustomInMemoryFs(new ManualClock());
root = fs.getPath("/root/workspace");
writableRoot = fs.getPath("/writableRoot/workspace");
@@ -180,7 +182,7 @@
.getPackageFactoryBuilderForTesting(directories)
.build(ruleClassProvider, fs),
directories,
- /*bzlLoadFunctionForInlining=*/ null));
+ /* bzlLoadFunctionForInlining= */ null));
skyFunctions.put(
SkyFunctions.EXTERNAL_PACKAGE,
new ExternalPackageFunction(BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER));
@@ -454,9 +456,7 @@
private void assertGlobsEqual(String pattern1, String pattern2) throws Exception {
GlobValue value1 = runGlob(pattern1, Globber.Operation.FILES_AND_DIRS);
GlobValue value2 = runGlob(pattern2, Globber.Operation.FILES_AND_DIRS);
- new EqualsTester()
- .addEqualityGroup(value1, value2)
- .testEquals();
+ new EqualsTester().addEqualityGroup(value1, value2).testEquals();
}
private GlobValue runGlob(String pattern, Globber.Operation globberOperation) throws Exception {
@@ -536,9 +536,7 @@
PathFragment.EMPTY_FRAGMENT));
}
- /**
- * Tests that globs can contain Java regular expression special characters
- */
+ /** Tests that globs can contain Java regular expression special characters */
@Test
public void testSpecialRegexCharacter() throws Exception {
Path aDotB = pkgPath.getChild("a.b");
@@ -702,7 +700,7 @@
fs.stubStat(pkgPath, null);
RootedPath pkgRootedPath = RootedPath.toRootedPath(Root.fromPath(root), pkgPath);
FileStateValue pkgDirFileStateValue =
- FileStateValue.create(pkgRootedPath, SyscallCache.NO_CACHE, /*tsgm=*/ null);
+ FileStateValue.create(pkgRootedPath, SyscallCache.NO_CACHE, /* tsgm= */ null);
FileValue pkgDirValue =
FileValue.value(
ImmutableList.of(pkgRootedPath),
@@ -899,83 +897,8 @@
}
@Test
- public void subpackages_oneLevelDeep() throws Exception {
- makeEmptyPackage("base/sub");
- makeEmptyPackage("base/sub2");
- makeEmptyPackage("base/sub3");
-
- assertSubpackageMatches("base/*", /* => */ "base/sub", "base/sub2", "base/sub3");
- assertSubpackageMatches("base/**", /* => */ "base/sub", "base/sub2", "base/sub3");
- }
-
- @Test
- public void subpackages_oneLevel_notDeepEnough() throws Exception {
- makeEmptyPackage("base/sub/pkg");
- makeEmptyPackage("base/sub2/pkg");
- makeEmptyPackage("base/sub3/pkg");
-
- // * doesn't go deep enough
- assertSubpackageMatches("base/*");
- // But if we go with ** it works fine.
- assertSubpackageMatches("base/**", /* => */ "base/sub/pkg", "base/sub2/pkg", "base/sub3/pkg");
- }
-
- @Test
- public void subpackages_deepRecurse() throws Exception {
- makeEmptyPackage("base/sub/1");
- makeEmptyPackage("base/sub/2");
- makeEmptyPackage("base/sub2/3");
- makeEmptyPackage("base/sub2/4");
- makeEmptyPackage("base/sub3/5");
- makeEmptyPackage("base/sub3/6");
-
- FileSystemUtils.createEmptyFile(pkgPath.getRelative("foo/bar/BUILD"));
- // "foo/bar" should not be in the results because foo/bar is a separate package.
- assertSubpackageMatches(
- "base/*/*",
- "base/sub/1",
- "base/sub/2",
- "base/sub2/3",
- "base/sub2/4",
- "base/sub3/5",
- "base/sub3/6");
-
- assertSubpackageMatches(
- "base/**",
- "base/sub/1",
- "base/sub/2",
- "base/sub2/3",
- "base/sub2/4",
- "base/sub3/5",
- "base/sub3/6");
- }
-
- @Test
- public void subpackages_middleWidlcard() throws Exception {
- makeEmptyPackage("base/sub1/same");
- makeEmptyPackage("base/sub2/same");
- makeEmptyPackage("base/sub3/same");
- makeEmptyPackage("base/sub4/same");
- makeEmptyPackage("base/sub5/same");
- makeEmptyPackage("base/sub6/same");
-
- assertSubpackageMatches(
- "base/*/same",
- "base/sub1/same",
- "base/sub2/same",
- "base/sub3/same",
- "base/sub4/same",
- "base/sub5/same",
- "base/sub6/same");
-
- assertSubpackageMatches(
- "base/**/same",
- "base/sub1/same",
- "base/sub2/same",
- "base/sub3/same",
- "base/sub4/same",
- "base/sub5/same",
- "base/sub6/same");
+ public void subpackages_doubleStarPatternWithNamedChild() throws Exception {
+ assertSubpackageMatches("**/bar");
}
@Test
@@ -993,6 +916,112 @@
}
@Test
+ public void subpackages_zeroLevelDeep() throws Exception {
+ makeEmptyPackage("sub");
+
+ assertSubpackageMatches("sub/*");
+
+ // `**` is considered to matching nothing below.
+ assertSubpackageMatches("sub/**", "sub");
+ assertSubpackageMatches("sub/**/**", "sub");
+
+ assertSubpackageMatches("sub/**/foo");
+ assertSubpackageMatches("sub/**/foo/**");
+ }
+
+ @Test
+ public void subpackages_zeroAndOneLevelDeep() throws Exception {
+ makeEmptyPackage("sub");
+ makeEmptyPackage("sub/subOfSub");
+
+ assertSubpackageMatches("sub/*");
+
+ // `**` is considered to matching nothing below.
+ assertSubpackageMatches("sub/**", "sub");
+ assertSubpackageMatches("sub/**/**", "sub");
+
+ assertSubpackageMatches("sub/**/foo");
+ assertSubpackageMatches("sub/**/foo/**");
+ }
+
+ @Test
+ public void subpackages_oneLevelDeep() throws Exception {
+ makeEmptyPackage("base/sub");
+ makeEmptyPackage("base/sub2");
+ makeEmptyPackage("base/sub3");
+
+ List<String> matchingPatterns =
+ Arrays.asList("base/*", "base/**", "base/**/**", "base/**/sub*", "base/**/sub*/**");
+
+ for (String pattern : matchingPatterns) {
+ assertSubpackageMatches(pattern, /* => */ "base/sub", "base/sub2", "base/sub3");
+ }
+ }
+
+ @Test
+ public void subpackages_deepRecurse() throws Exception {
+ makeEmptyPackage("base/sub/1");
+ makeEmptyPackage("base/sub/2");
+ makeEmptyPackage("base/sub2/3");
+ makeEmptyPackage("base/sub2/4");
+ makeEmptyPackage("base/sub3/5");
+ makeEmptyPackage("base/sub3/6");
+
+ FileSystemUtils.createEmptyFile(pkgPath.getRelative("foo/bar/BUILD"));
+
+ // * doesn't go deep enough, so no matches
+ assertSubpackageMatches("base/*");
+
+ List<String> matchingPatterns =
+ Arrays.asList("base/**", "base/*/*", "base/*/*/**", "base/*/*/**/**", "base/**/sub*/**");
+
+ for (String pattern : matchingPatterns) {
+ assertSubpackageMatches(
+ pattern,
+ "base/sub/1",
+ "base/sub/2",
+ "base/sub2/3",
+ "base/sub2/4",
+ "base/sub3/5",
+ "base/sub3/6");
+ }
+ }
+
+ @Test
+ public void subpackages_middleWildcard() throws Exception {
+ makeEmptyPackage("base/same");
+ makeEmptyPackage("base/sub1/same");
+ makeEmptyPackage("base/sub2/same");
+ makeEmptyPackage("base/sub3/same");
+ makeEmptyPackage("base/sub4/same");
+ makeEmptyPackage("base/sub5/same");
+ makeEmptyPackage("base/sub6/same");
+ makeEmptyPackage("base/sub7/sub8/same");
+ makeEmptyPackage("base/sub9/sub10/sub11/same");
+
+ assertSubpackageMatches(
+ "base/*/same",
+ "base/sub1/same",
+ "base/sub2/same",
+ "base/sub3/same",
+ "base/sub4/same",
+ "base/sub5/same",
+ "base/sub6/same");
+
+ assertSubpackageMatches(
+ "base/**/same",
+ "base/same",
+ "base/sub1/same",
+ "base/sub2/same",
+ "base/sub3/same",
+ "base/sub4/same",
+ "base/sub5/same",
+ "base/sub6/same",
+ "base/sub7/sub8/same",
+ "base/sub9/sub10/sub11/same");
+ }
+
+ @Test
public void subpackages_testSymlinks() throws Exception {
Path newPackagePath = pkgPath.getRelative("path/to/pkg");
makeEmptyPackage(newPackagePath);
