Fix two issues with incremental Skyframe hybrid globbing and the 'allow_empty' param to the 'glob' function
(1) Previously, the 'allow_empty=False' enforcement was solely in legacy globbing. So on incremental package evaluation where all the GlobValue nodes already existed in the Skyframe graph, we would not do any enforcement. This was fixed by having by Skyframe globbing and legacy globbing do enforcement.
(2) On incremental package evaluation where all the GlobValue nodes already existed in the Skyframe graph, we would still call into legacy globbing with an empty list of glob patterns. Legacy globbing would then error out just as it does normally when a glob *include* pattern matches a bunch of stuff, but then all the matches are removed by an *exclude* pattern. This was fixed by having Skyframe hybrid globbing only call into legacy globbing if there is work to do.
I added a bunch of unit tests for various incremental situations. I also added explicit unit tests for 'allow_empty=True'. We were previous missing all/some of this coverage, respectively.
Fixes #8517
RELNOTES: None
PiperOrigin-RevId: 253046516
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index de658d7..23a6c77 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -914,9 +914,11 @@
}
}
Token legacyIncludesToken =
- legacyGlobber.runAsync(globsToDelegate, ImmutableList.of(), excludeDirs, allowEmpty);
-
- return new HybridToken(globValueMap, globKeys, legacyIncludesToken, excludes);
+ globsToDelegate.isEmpty()
+ ? null
+ : legacyGlobber.runAsync(
+ globsToDelegate, ImmutableList.of(), excludeDirs, allowEmpty);
+ return new HybridToken(globValueMap, globKeys, legacyIncludesToken, excludes, allowEmpty);
}
private Collection<SkyKey> getMissingKeys(Collection<SkyKey> globKeys,
@@ -970,19 +972,23 @@
// (this is includes_sky above).
private final Iterable<SkyKey> includesGlobKeys;
// A token for computing legacy globs.
- private final Token legacyIncludesToken;
+ @Nullable private final Token legacyIncludesToken;
private final List<String> excludes;
+ private final boolean allowEmpty;
+
private HybridToken(
Map<SkyKey, ValueOrException2<IOException, BuildFileNotFoundException>> globValueMap,
Iterable<SkyKey> includesGlobKeys,
- Token delegateIncludesToken,
- List<String> excludes) {
+ @Nullable Token delegateIncludesToken,
+ List<String> excludes,
+ boolean allowEmpty) {
this.globValueMap = globValueMap;
this.includesGlobKeys = includesGlobKeys;
this.legacyIncludesToken = delegateIncludesToken;
this.excludes = excludes;
+ this.allowEmpty = allowEmpty;
}
private List<String> resolve(Globber delegate)
@@ -990,16 +996,31 @@
HashSet<String> matches = new HashSet<>();
for (SkyKey includeGlobKey : includesGlobKeys) {
// TODO(bazel-team): NestedSet expansion here is suboptimal.
+ boolean foundMatch = false;
for (PathFragment match : getGlobMatches(includeGlobKey, globValueMap)) {
matches.add(match.getPathString());
+ foundMatch = true;
+ }
+ if (!allowEmpty && !foundMatch) {
+ throw new BadGlobException(
+ "glob pattern '"
+ + ((GlobDescriptor) includeGlobKey.argument()).getPattern()
+ + "' didn't match anything, but allow_empty is set to False.");
}
}
- matches.addAll(delegate.fetch(legacyIncludesToken));
+ if (legacyIncludesToken != null) {
+ matches.addAll(delegate.fetch(legacyIncludesToken));
+ }
UnixGlob.removeExcludes(matches, excludes);
List<String> result = new ArrayList<>(matches);
// Skyframe glob results are unsorted. And we used a LegacyGlobber that doesn't sort.
// Therefore, we want to unconditionally sort here.
Collections.sort(result);
+
+ if (!allowEmpty && result.isEmpty()) {
+ throw new BadGlobException(
+ "all files in the glob have been excluded, but allow_empty is set to False.");
+ }
return result;
}
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 4a606a8..1ea1658c 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
@@ -242,33 +242,6 @@
}
@Test
- public void testGlobAllowEmpty() throws Exception {
- assertEmpty(cache.globUnsorted(list("*.java"), NONE, false, true));
-
- BadGlobException expected =
- assertThrows(
- BadGlobException.class, () -> cache.globUnsorted(list("*.java"), NONE, false, false));
- assertThat(expected).hasMessageThat().contains("allow_empty");
-
- assertThat(cache.globUnsorted(list("*.txt", "*.java"), NONE, false, true))
- .containsExactly("first.txt", "second.txt");
-
- expected =
- assertThrows(
- BadGlobException.class,
- () -> cache.globUnsorted(list("*.txt", "*.java"), NONE, false, false));
- assertThat(expected).hasMessageThat().contains("allow_empty");
-
- assertEmpty(cache.globUnsorted(list("*.txt"), list("*.*"), false, true));
-
- expected =
- assertThrows(
- BadGlobException.class,
- () -> cache.globUnsorted(list("*.txt"), list("*.*"), false, false));
- assertThat(expected).hasMessageThat().contains("allow_empty");
- }
-
- @Test
public void testChildGlobWithChildExclude() throws Exception {
assertThat(cache.globUnsorted(list("foo/*"), list("foo/*"), false, true)).isEmpty();
assertThat(
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 7583924..be4f723 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
@@ -809,48 +809,6 @@
}
@Test
- public void testGlobDisallowEmpty() throws Exception {
- Path buildFile = scratch.file("/pkg/BUILD", "x = " + "glob(['*.foo'])");
- Package pkg =
- packages.createPackage(
- "pkg",
- RootedPath.toRootedPath(root, buildFile),
- "--incompatible_disallow_empty_glob=false");
- assertThat(pkg.containsErrors()).isFalse();
- }
-
- @Test
- public void testGlobAllowEmpty() throws Exception {
- events.setFailFast(false);
-
- Path buildFile = scratch.file("/pkg/BUILD", "x = " + "glob(['*.foo'])");
- Package pkg =
- packages.createPackage(
- "pkg",
- RootedPath.toRootedPath(root, buildFile),
- "--incompatible_disallow_empty_glob=true");
-
- assertThat(pkg.containsErrors()).isTrue();
- events.assertContainsError(
- "glob pattern '*.foo' didn't match anything, but allow_empty is set to False");
- }
-
- @Test
- public void testGlobNotBoolean() throws Exception {
- events.setFailFast(false);
-
- Path buildFile = scratch.file("/pkg/BUILD", "x = " + "glob(['*.foo'], allow_empty = 5)");
- Package pkg =
- packages.createPackage(
- "pkg",
- RootedPath.toRootedPath(root, buildFile),
- "--incompatible_disallow_empty_glob=true");
-
- assertThat(pkg.containsErrors()).isTrue();
- events.assertContainsError("expected boolean for argument `allow_empty`, got `5`");
- }
-
- @Test
public void testNativeModuleIsAvailable() throws Exception {
Path buildFile = scratch.file("/pkg/BUILD", "native.cc_library(name='bar')");
Package pkg =
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 3a2c36c..a8c1871 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
@@ -34,12 +34,14 @@
import com.google.devtools.build.lib.packages.ConstantRuleVisibility;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
+import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.StarlarkSemanticsOptions;
import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction;
import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
import com.google.devtools.build.lib.testutil.ManualClock;
+import com.google.devtools.build.lib.testutil.MoreAsserts;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.Dirent;
import com.google.devtools.build.lib.vfs.FileStatus;
@@ -81,6 +83,12 @@
private CustomInMemoryFs fs = new CustomInMemoryFs(new ManualClock());
private void preparePackageLoading(Path... roots) {
+ preparePackageLoadingWithCustomStarklarkSemanticsOptions(
+ Options.getDefaults(StarlarkSemanticsOptions.class), roots);
+ }
+
+ private void preparePackageLoadingWithCustomStarklarkSemanticsOptions(
+ StarlarkSemanticsOptions starlarkSemanticsOptions, Path... roots) {
PackageCacheOptions packageCacheOptions = Options.getDefaults(PackageCacheOptions.class);
packageCacheOptions.defaultVisibility = ConstantRuleVisibility.PUBLIC;
packageCacheOptions.showLoadingProgress = true;
@@ -92,7 +100,7 @@
Arrays.stream(roots).map(Root::fromPath).collect(ImmutableList.toImmutableList()),
BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY),
packageCacheOptions,
- Options.getDefaults(StarlarkSemanticsOptions.class),
+ starlarkSemanticsOptions,
UUID.randomUUID(),
ImmutableMap.<String, String>of(),
new TimestampGranularityMonitor(BlazeClock.instance()));
@@ -104,7 +112,16 @@
return fs;
}
- private PackageValue validPackage(SkyKey skyKey) throws InterruptedException {
+ private Package validPackageWithoutErrors(SkyKey skyKey) throws InterruptedException {
+ return validPackageInternal(skyKey, /*checkPackageError=*/ true);
+ }
+
+ private Package validPackage(SkyKey skyKey) throws InterruptedException {
+ return validPackageInternal(skyKey, /*checkPackageError=*/ false);
+ }
+
+ private Package validPackageInternal(SkyKey skyKey, boolean checkPackageError)
+ throws InterruptedException {
SkyframeExecutor skyframeExecutor = getSkyframeExecutor();
skyframeExecutor.injectExtraPrecomputedValues(
ImmutableList.of(
@@ -118,14 +135,16 @@
fail(result.getError(skyKey).getException().getMessage());
}
PackageValue value = result.get(skyKey);
- assertThat(value.getPackage().containsErrors()).isFalse();
- return value;
+ if (checkPackageError) {
+ assertThat(value.getPackage().containsErrors()).isFalse();
+ }
+ return value.getPackage();
}
@Test
public void testValidPackage() throws Exception {
scratch.file("pkg/BUILD");
- validPackage(PackageValue.key(PackageIdentifier.parse("@//pkg")));
+ validPackageWithoutErrors(PackageValue.key(PackageIdentifier.parse("@//pkg")));
}
@Test
@@ -271,15 +290,10 @@
scratch.file("foo/c/c.txt");
preparePackageLoading(rootDirectory);
SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
- PackageValue value = validPackage(skyKey);
+ Package pkg = validPackageWithoutErrors(skyKey);
assertThat(
(Iterable<Label>)
- value
- .getPackage()
- .getTarget("foo")
- .getAssociatedRule()
- .getAttributeContainer()
- .getAttr("srcs"))
+ pkg.getTarget("foo").getAssociatedRule().getAttributeContainer().getAttr("srcs"))
.containsExactly(
Label.parseAbsoluteUnchecked("//foo:b.txt"),
Label.parseAbsoluteUnchecked("//foo:c/c.txt"))
@@ -290,15 +304,10 @@
reporter,
ModifiedFileSet.builder().modify(PathFragment.create("foo/d.txt")).build(),
Root.fromPath(rootDirectory));
- value = validPackage(skyKey);
+ pkg = validPackageWithoutErrors(skyKey);
assertThat(
(Iterable<Label>)
- value
- .getPackage()
- .getTarget("foo")
- .getAssociatedRule()
- .getAttributeContainer()
- .getAttr("srcs"))
+ pkg.getTarget("foo").getAssociatedRule().getAttributeContainer().getAttr("srcs"))
.containsExactly(
Label.parseAbsoluteUnchecked("//foo:b.txt"),
Label.parseAbsoluteUnchecked("//foo:c/c.txt"),
@@ -313,7 +322,7 @@
scratch.file("foo/a.config");
preparePackageLoading(rootDirectory);
SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
- assertSrcs(validPackage(skyKey), "foo", "//foo:b.txt");
+ assertSrcs(validPackageWithoutErrors(skyKey), "foo", "//foo:b.txt");
scratch.overwriteFile(
"foo/BUILD", "sh_library(name = 'foo', srcs = glob(['*.txt', '*.config']))");
getSkyframeExecutor()
@@ -321,7 +330,7 @@
reporter,
ModifiedFileSet.builder().modify(PathFragment.create("foo/BUILD")).build(),
Root.fromPath(rootDirectory));
- assertSrcs(validPackage(skyKey), "foo", "//foo:a.config", "//foo:b.txt");
+ assertSrcs(validPackageWithoutErrors(skyKey), "foo", "//foo:a.config", "//foo:b.txt");
scratch.overwriteFile(
"foo/BUILD", "sh_library(name = 'foo', srcs = glob(['*.txt', '*.config'])) # comment");
getSkyframeExecutor()
@@ -329,7 +338,7 @@
reporter,
ModifiedFileSet.builder().modify(PathFragment.create("foo/BUILD")).build(),
Root.fromPath(rootDirectory));
- assertSrcs(validPackage(skyKey), "foo", "//foo:a.config", "//foo:b.txt");
+ assertSrcs(validPackageWithoutErrors(skyKey), "foo", "//foo:a.config", "//foo:b.txt");
getSkyframeExecutor().resetEvaluator();
PackageCacheOptions packageCacheOptions = Options.getDefaults(PackageCacheOptions.class);
packageCacheOptions.defaultVisibility = ConstantRuleVisibility.PUBLIC;
@@ -347,7 +356,7 @@
ImmutableMap.<String, String>of(),
tsgm);
getSkyframeExecutor().setActionEnv(ImmutableMap.<String, String>of());
- assertSrcs(validPackage(skyKey), "foo", "//foo:a.config", "//foo:b.txt");
+ assertSrcs(validPackageWithoutErrors(skyKey), "foo", "//foo:a.config", "//foo:b.txt");
}
/**
@@ -372,10 +381,10 @@
scratch.resolve("foo/subdir_link"), externalTarget.getParentDirectory());
preparePackageLoading(rootDirectory);
SkyKey fooKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
- PackageValue fooValue = validPackage(fooKey);
- assertSrcs(fooValue, "foo", "//foo:link.sh", "//foo:ordinary.sh");
- assertSrcs(fooValue, "bar", "//foo:link.sh");
- assertSrcs(fooValue, "baz", "//foo:subdir_link/target.txt");
+ Package fooPkg = validPackageWithoutErrors(fooKey);
+ assertSrcs(fooPkg, "foo", "//foo:link.sh", "//foo:ordinary.sh");
+ assertSrcs(fooPkg, "bar", "//foo:link.sh");
+ assertSrcs(fooPkg, "baz", "//foo:subdir_link/target.txt");
scratch.overwriteFile(
"foo/BUILD",
"sh_library(name = 'foo', srcs = glob(['*.sh'])) #comment",
@@ -386,32 +395,27 @@
reporter,
ModifiedFileSet.builder().modify(PathFragment.create("foo/BUILD")).build(),
Root.fromPath(rootDirectory));
- PackageValue fooValue2 = validPackage(fooKey);
- assertThat(fooValue2).isNotEqualTo(fooValue);
- assertSrcs(fooValue2, "foo", "//foo:link.sh", "//foo:ordinary.sh");
- assertSrcs(fooValue2, "bar", "//foo:link.sh");
- assertSrcs(fooValue2, "baz", "//foo:subdir_link/target.txt");
+ Package fooPkg2 = validPackageWithoutErrors(fooKey);
+ assertThat(fooPkg2).isNotEqualTo(fooPkg);
+ assertSrcs(fooPkg2, "foo", "//foo:link.sh", "//foo:ordinary.sh");
+ assertSrcs(fooPkg2, "bar", "//foo:link.sh");
+ assertSrcs(fooPkg2, "baz", "//foo:subdir_link/target.txt");
}
- private static void assertSrcs(PackageValue value, String targetName, String... expected)
+ private static void assertSrcs(Package pkg, String targetName, String... expected)
throws NoSuchTargetException {
List<Label> expectedLabels = new ArrayList<>();
for (String item : expected) {
expectedLabels.add(Label.parseAbsoluteUnchecked(item));
}
- assertThat(getSrcs(value, targetName)).containsExactlyElementsIn(expectedLabels).inOrder();
+ assertThat(getSrcs(pkg, targetName)).containsExactlyElementsIn(expectedLabels).inOrder();
}
@SuppressWarnings("unchecked")
- private static Iterable<Label> getSrcs(PackageValue packageValue, String targetName)
+ private static Iterable<Label> getSrcs(Package pkg, String targetName)
throws NoSuchTargetException {
return (Iterable<Label>)
- packageValue
- .getPackage()
- .getTarget(targetName)
- .getAssociatedRule()
- .getAttributeContainer()
- .getAttr("srcs");
+ pkg.getTarget(targetName).getAssociatedRule().getAttributeContainer().getAttr("srcs");
}
@Test
@@ -422,14 +426,14 @@
"sh_library(name = 'bar', srcs = glob(['*.sh', '*.txt']))");
preparePackageLoading(rootDirectory);
SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
- PackageValue value = validPackage(skyKey);
- scratch.file("foo/irrelevent");
+ Package pkg = validPackageWithoutErrors(skyKey);
+ scratch.file("foo/irrelevant");
getSkyframeExecutor()
.invalidateFilesUnderPathForTesting(
reporter,
ModifiedFileSet.builder().modify(PathFragment.create("foo/irrelevant")).build(),
Root.fromPath(rootDirectory));
- assertThat(validPackage(skyKey)).isSameInstanceAs(value);
+ assertThat(validPackageWithoutErrors(skyKey)).isSameInstanceAs(pkg);
}
@Test
@@ -440,14 +444,14 @@
"sh_library(name = 'bar', srcs = glob(['*.sh', '*.txt']))");
preparePackageLoading(rootDirectory);
SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
- PackageValue value = validPackage(skyKey);
- scratch.file("foo/irrelevent");
+ Package pkg = validPackageWithoutErrors(skyKey);
+ scratch.file("foo/irrelevant");
getSkyframeExecutor()
.invalidateFilesUnderPathForTesting(
reporter,
ModifiedFileSet.builder().modify(PathFragment.create("foo/irrelevant")).build(),
Root.fromPath(rootDirectory));
- assertThat(validPackage(skyKey)).isSameInstanceAs(value);
+ assertThat(validPackageWithoutErrors(skyKey)).isSameInstanceAs(pkg);
}
@Test
@@ -463,8 +467,8 @@
preparePackageLoading(rootDirectory);
SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
- PackageValue value = validPackage(skyKey);
- assertThat(value.getPackage().getSkylarkFileDependencies())
+ Package pkg = validPackageWithoutErrors(skyKey);
+ assertThat(pkg.getSkylarkFileDependencies())
.containsExactly(
Label.parseAbsolute("//bar:ext.bzl", ImmutableMap.of()),
Label.parseAbsolute("//baz:ext.bzl", ImmutableMap.of()));
@@ -476,8 +480,8 @@
ModifiedFileSet.builder().modify(PathFragment.create("bar/ext.bzl")).build(),
Root.fromPath(rootDirectory));
- value = validPackage(skyKey);
- assertThat(value.getPackage().getSkylarkFileDependencies())
+ pkg = validPackageWithoutErrors(skyKey);
+ assertThat(pkg.getSkylarkFileDependencies())
.containsExactly(
Label.parseAbsolute("//bar:ext.bzl", ImmutableMap.of()),
Label.parseAbsolute("//qux:ext.bzl", ImmutableMap.of()));
@@ -580,7 +584,7 @@
public void testLoadRelativePath() throws Exception {
scratch.file("pkg/BUILD", "load(':ext.bzl', 'a')");
scratch.file("pkg/ext.bzl", "a = 1");
- validPackage(PackageValue.key(PackageIdentifier.parse("@//pkg")));
+ validPackageWithoutErrors(PackageValue.key(PackageIdentifier.parse("@//pkg")));
}
@Test
@@ -588,7 +592,7 @@
scratch.file("pkg1/BUILD");
scratch.file("pkg2/BUILD", "load('//pkg1:ext.bzl', 'a')");
scratch.file("pkg1/ext.bzl", "a = 1");
- validPackage(PackageValue.key(PackageIdentifier.parse("@//pkg2")));
+ validPackageWithoutErrors(PackageValue.key(PackageIdentifier.parse("@//pkg2")));
}
@Test
@@ -623,10 +627,10 @@
preparePackageLoading(rootDirectory);
SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
- PackageValue value = validPackage(skyKey);
- assertThat(value.getPackage().containsErrors()).isFalse();
- assertThat(value.getPackage().getTarget("existing.txt").getName()).isEqualTo("existing.txt");
- assertThrows(NoSuchTargetException.class, () -> value.getPackage().getTarget("dangling.txt"));
+ Package pkg = validPackageWithoutErrors(skyKey);
+ assertThat(pkg.containsErrors()).isFalse();
+ assertThat(pkg.getTarget("existing.txt").getName()).isEqualTo("existing.txt");
+ assertThrows(NoSuchTargetException.class, () -> pkg.getTarget("dangling.txt"));
scratch.overwriteFile(
"foo/BUILD", "exports_files(glob(['*.txt']))", "#some-irrelevant-comment");
@@ -637,10 +641,10 @@
ModifiedFileSet.builder().modify(PathFragment.create("foo/BUILD")).build(),
Root.fromPath(rootDirectory));
- PackageValue value2 = validPackage(skyKey);
- assertThat(value2.getPackage().containsErrors()).isFalse();
- assertThat(value2.getPackage().getTarget("existing.txt").getName()).isEqualTo("existing.txt");
- assertThrows(NoSuchTargetException.class, () -> value2.getPackage().getTarget("dangling.txt"));
+ Package pkg2 = validPackageWithoutErrors(skyKey);
+ assertThat(pkg2.containsErrors()).isFalse();
+ assertThat(pkg2.getTarget("existing.txt").getName()).isEqualTo("existing.txt");
+ assertThrows(NoSuchTargetException.class, () -> pkg2.getTarget("dangling.txt"));
// One consequence of the bug was that dangling symlinks were matched by globs evaluated by
// Skyframe globbing, meaning there would incorrectly be corresponding targets in packages
// that had skyframe cache hits during skyframe hybrid globbing.
@@ -652,13 +656,13 @@
ModifiedFileSet.builder().modify(PathFragment.create("foo/nope")).build(),
Root.fromPath(rootDirectory));
- PackageValue newValue = validPackage(skyKey);
- assertThat(newValue.getPackage().containsErrors()).isFalse();
- assertThat(newValue.getPackage().getTarget("existing.txt").getName()).isEqualTo("existing.txt");
+ Package newPkg = validPackageWithoutErrors(skyKey);
+ assertThat(newPkg.containsErrors()).isFalse();
+ assertThat(newPkg.getTarget("existing.txt").getName()).isEqualTo("existing.txt");
// Another consequence of the bug is that change pruning would incorrectly cut off changes that
// caused a dangling symlink potentially matched by a glob to come into existence.
- assertThat(newValue.getPackage().getTarget("dangling.txt").getName()).isEqualTo("dangling.txt");
- assertThat(newValue.getPackage()).isNotSameInstanceAs(value.getPackage());
+ assertThat(newPkg.getTarget("dangling.txt").getName()).isEqualTo("dangling.txt");
+ assertThat(newPkg).isNotSameInstanceAs(pkg);
}
// Regression test for Skyframe globbing incorrectly matching the package's directory path on
@@ -674,10 +678,10 @@
preparePackageLoading(rootDirectory);
SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
- PackageValue value = validPackage(skyKey);
- assertThat(value.getPackage().containsErrors()).isFalse();
- assertThat(value.getPackage().getTarget("bar-matched").getName()).isEqualTo("bar-matched");
- assertThrows(NoSuchTargetException.class, () -> value.getPackage().getTarget("-matched"));
+ Package pkg = validPackageWithoutErrors(skyKey);
+ assertThat(pkg.containsErrors()).isFalse();
+ assertThat(pkg.getTarget("bar-matched").getName()).isEqualTo("bar-matched");
+ assertThrows(NoSuchTargetException.class, () -> pkg.getTarget("-matched"));
scratch.overwriteFile(
"foo/BUILD",
@@ -689,10 +693,10 @@
ModifiedFileSet.builder().modify(PathFragment.create("foo/BUILD")).build(),
Root.fromPath(rootDirectory));
- PackageValue value2 = validPackage(skyKey);
- assertThat(value2.getPackage().containsErrors()).isFalse();
- assertThat(value2.getPackage().getTarget("bar-matched").getName()).isEqualTo("bar-matched");
- assertThrows(NoSuchTargetException.class, () -> value2.getPackage().getTarget("-matched"));
+ Package pkg2 = validPackageWithoutErrors(skyKey);
+ assertThat(pkg2.containsErrors()).isFalse();
+ assertThat(pkg2.getTarget("bar-matched").getName()).isEqualTo("bar-matched");
+ assertThrows(NoSuchTargetException.class, () -> pkg2.getTarget("-matched"));
}
@Test
@@ -777,6 +781,298 @@
assertContainsEvent("circular symlinks detected");
}
+ @Test
+ public void testGlobAllowEmpty_ParamValueMustBeBoolean() throws Exception {
+ reporter.removeHandler(failFastHandler);
+
+ scratch.file("pkg/BUILD", "x = " + "glob(['*.foo'], allow_empty = 5)");
+ invalidatePackages();
+
+ SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//pkg"));
+ Package pkg = validPackage(skyKey);
+
+ String expectedEventString = "expected boolean for argument `allow_empty`, got `5`";
+ MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedEventString);
+ assertContainsEvent(expectedEventString);
+ }
+
+ @Test
+ public void testGlobAllowEmpty_FunctionParam() throws Exception {
+ scratch.file("pkg/BUILD", "x = " + "glob(['*.foo'], allow_empty=True)");
+ invalidatePackages();
+
+ SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//pkg"));
+ Package pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isFalse();
+ assertThat(pkg.getEvents()).isEmpty();
+ }
+
+ @Test
+ public void testGlobAllowEmpty_StarlarkOption() throws Exception {
+ preparePackageLoadingWithCustomStarklarkSemanticsOptions(
+ Options.parse(StarlarkSemanticsOptions.class, "--incompatible_disallow_empty_glob=false")
+ .getOptions(),
+ rootDirectory);
+
+ scratch.file("pkg/BUILD", "x = " + "glob(['*.foo'])");
+ invalidatePackages();
+
+ SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//pkg"));
+ Package pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isFalse();
+ assertThat(pkg.getEvents()).isEmpty();
+ }
+
+ @Test
+ public void testGlobDisallowEmpty_FunctionParam_WasNonEmptyAndBecomesEmpty() throws Exception {
+ scratch.file("pkg/BUILD", "x = " + "glob(['*.foo'], allow_empty=False)");
+ scratch.file("pkg/blah.foo");
+ invalidatePackages();
+
+ SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//pkg"));
+
+ Package pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isFalse();
+ assertThat(pkg.getEvents()).isEmpty();
+
+ scratch.deleteFile("pkg/blah.foo");
+ getSkyframeExecutor()
+ .invalidateFilesUnderPathForTesting(
+ reporter,
+ ModifiedFileSet.builder().modify(PathFragment.create("pkg/blah.foo")).build(),
+ Root.fromPath(rootDirectory));
+
+ reporter.removeHandler(failFastHandler);
+ pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isTrue();
+ String expectedEventString =
+ "glob pattern '*.foo' didn't match anything, but allow_empty is set to False";
+ MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedEventString);
+ assertContainsEvent(expectedEventString);
+ }
+
+ @Test
+ public void testGlobDisallowEmpty_StarlarkOption_WasNonEmptyAndBecomesEmpty() throws Exception {
+ preparePackageLoadingWithCustomStarklarkSemanticsOptions(
+ Options.parse(StarlarkSemanticsOptions.class, "--incompatible_disallow_empty_glob=true")
+ .getOptions(),
+ rootDirectory);
+
+ scratch.file("pkg/BUILD", "x = " + "glob(['*.foo'])");
+ scratch.file("pkg/blah.foo");
+ invalidatePackages();
+
+ SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//pkg"));
+
+ Package pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isFalse();
+ assertThat(pkg.getEvents()).isEmpty();
+
+ scratch.deleteFile("pkg/blah.foo");
+ getSkyframeExecutor()
+ .invalidateFilesUnderPathForTesting(
+ reporter,
+ ModifiedFileSet.builder().modify(PathFragment.create("pkg/blah.foo")).build(),
+ Root.fromPath(rootDirectory));
+
+ reporter.removeHandler(failFastHandler);
+ pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isTrue();
+ String expectedEventString =
+ "glob pattern '*.foo' didn't match anything, but allow_empty is set to False";
+ MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedEventString);
+ assertContainsEvent(expectedEventString);
+ }
+
+ @Test
+ public void testGlobDisallowEmpty_FunctionParam_WasEmptyAndStaysEmpty() throws Exception {
+ scratch.file("pkg/BUILD", "x = " + "glob(['*.foo'], allow_empty=False)");
+ invalidatePackages();
+
+ SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//pkg"));
+ reporter.removeHandler(failFastHandler);
+
+ Package pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isTrue();
+ String expectedEventString =
+ "glob pattern '*.foo' didn't match anything, but allow_empty is set to False";
+ MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedEventString);
+ assertContainsEvent(expectedEventString);
+
+ scratch.overwriteFile("pkg/BUILD", "x = " + "glob(['*.foo'], allow_empty=False) #comment");
+ getSkyframeExecutor()
+ .invalidateFilesUnderPathForTesting(
+ reporter,
+ ModifiedFileSet.builder().modify(PathFragment.create("pkg/BUILD")).build(),
+ Root.fromPath(rootDirectory));
+
+ pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isTrue();
+ MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedEventString);
+ assertContainsEvent(expectedEventString);
+ }
+
+ @Test
+ public void testGlobDisallowEmpty_StarlarkOption_WasEmptyAndStaysEmpty() throws Exception {
+ preparePackageLoadingWithCustomStarklarkSemanticsOptions(
+ Options.parse(StarlarkSemanticsOptions.class, "--incompatible_disallow_empty_glob=true")
+ .getOptions(),
+ rootDirectory);
+
+ scratch.file("pkg/BUILD", "x = " + "glob(['*.foo'])");
+ invalidatePackages();
+
+ SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//pkg"));
+ reporter.removeHandler(failFastHandler);
+
+ Package pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isTrue();
+ String expectedEventString =
+ "glob pattern '*.foo' didn't match anything, but allow_empty is set to False";
+ MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedEventString);
+ assertContainsEvent(expectedEventString);
+
+ scratch.overwriteFile("pkg/BUILD", "x = " + "glob(['*.foo']) #comment");
+ getSkyframeExecutor()
+ .invalidateFilesUnderPathForTesting(
+ reporter,
+ ModifiedFileSet.builder().modify(PathFragment.create("pkg/BUILD")).build(),
+ Root.fromPath(rootDirectory));
+
+ pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isTrue();
+ MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedEventString);
+ assertContainsEvent(expectedEventString);
+ }
+
+ @Test
+ public void testGlobDisallowEmpty_FunctionParam_WasEmptyDueToExcludeAndStaysEmpty()
+ throws Exception {
+ scratch.file("pkg/BUILD", "x = glob(include=['*.foo'], exclude=['blah.*'], allow_empty=False)");
+ scratch.file("pkg/blah.foo");
+ invalidatePackages();
+
+ SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//pkg"));
+ reporter.removeHandler(failFastHandler);
+
+ Package pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isTrue();
+ String expectedEventString =
+ "all files in the glob have been excluded, but allow_empty is set to False.";
+ MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedEventString);
+ assertContainsEvent(expectedEventString);
+
+ scratch.overwriteFile(
+ "pkg/BUILD",
+ "x = glob(include=['*.foo'], exclude=['blah.*'], allow_empty=False) # comment");
+ getSkyframeExecutor()
+ .invalidateFilesUnderPathForTesting(
+ reporter,
+ ModifiedFileSet.builder().modify(PathFragment.create("pkg/BUILD")).build(),
+ Root.fromPath(rootDirectory));
+
+ pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isTrue();
+ MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedEventString);
+ assertContainsEvent(expectedEventString);
+ }
+
+ @Test
+ public void testGlobDisallowEmpty_StarlarkOption_WasEmptyDueToExcludeAndStaysEmpty()
+ throws Exception {
+ preparePackageLoadingWithCustomStarklarkSemanticsOptions(
+ Options.parse(StarlarkSemanticsOptions.class, "--incompatible_disallow_empty_glob=true")
+ .getOptions(),
+ rootDirectory);
+
+ scratch.file("pkg/BUILD", "x = glob(include=['*.foo'], exclude=['blah.*'])");
+ scratch.file("pkg/blah.foo");
+ invalidatePackages();
+
+ SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//pkg"));
+ reporter.removeHandler(failFastHandler);
+
+ Package pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isTrue();
+ String expectedEventString =
+ "all files in the glob have been excluded, but allow_empty is set to False.";
+ MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedEventString);
+ assertContainsEvent(expectedEventString);
+
+ scratch.overwriteFile("pkg/BUILD", "x = glob(include=['*.foo'], exclude=['blah.*']) # comment");
+ getSkyframeExecutor()
+ .invalidateFilesUnderPathForTesting(
+ reporter,
+ ModifiedFileSet.builder().modify(PathFragment.create("pkg/BUILD")).build(),
+ Root.fromPath(rootDirectory));
+
+ pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isTrue();
+ MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedEventString);
+ assertContainsEvent(expectedEventString);
+ }
+
+ @Test
+ public void testGlobDisallowEmpty_FunctionParam_WasEmptyAndBecomesNonEmpty() throws Exception {
+ scratch.file("pkg/BUILD", "x = " + "glob(['*.foo'], allow_empty=False)");
+ invalidatePackages();
+
+ SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//pkg"));
+
+ reporter.removeHandler(failFastHandler);
+ Package pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isTrue();
+ String expectedEventString =
+ "glob pattern '*.foo' didn't match anything, but allow_empty is set to False";
+ MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedEventString);
+ assertContainsEvent(expectedEventString);
+
+ scratch.file("pkg/blah.foo");
+ getSkyframeExecutor()
+ .invalidateFilesUnderPathForTesting(
+ reporter,
+ ModifiedFileSet.builder().modify(PathFragment.create("pkg/blah.foo")).build(),
+ Root.fromPath(rootDirectory));
+
+ reporter.addHandler(failFastHandler);
+ pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isFalse();
+ assertThat(pkg.getEvents()).isEmpty();
+ }
+
+ @Test
+ public void testGlobDisallowEmpty_StarlarkOption_WasEmptyAndBecomesNonEmpty() throws Exception {
+ preparePackageLoadingWithCustomStarklarkSemanticsOptions(
+ Options.parse(StarlarkSemanticsOptions.class, "--incompatible_disallow_empty_glob=true")
+ .getOptions(),
+ rootDirectory);
+
+ scratch.file("pkg/BUILD", "x = " + "glob(['*.foo'])");
+ invalidatePackages();
+
+ SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//pkg"));
+
+ reporter.removeHandler(failFastHandler);
+ Package pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isTrue();
+ String expectedEventString =
+ "glob pattern '*.foo' didn't match anything, but allow_empty is set to False";
+ MoreAsserts.assertContainsEvent(pkg.getEvents(), expectedEventString);
+ assertContainsEvent(expectedEventString);
+
+ scratch.file("pkg/blah.foo");
+ getSkyframeExecutor()
+ .invalidateFilesUnderPathForTesting(
+ reporter,
+ ModifiedFileSet.builder().modify(PathFragment.create("pkg/blah.foo")).build(),
+ Root.fromPath(rootDirectory));
+
+ reporter.addHandler(failFastHandler);
+ pkg = validPackage(skyKey);
+ assertThat(pkg.containsErrors()).isFalse();
+ assertThat(pkg.getEvents()).isEmpty();
+ }
+
private static class CustomInMemoryFs extends InMemoryFileSystem {
private abstract static class FileStatusOrException {
abstract FileStatus get() throws IOException;
