Make .bazelignore affect globs, as it should.
Fixes #7433.
RELNOTES: None.
PiperOrigin-RevId: 276023691
diff --git a/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java b/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java
index 8d1bb74..dbc48f9 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java
@@ -17,6 +17,7 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Predicate;
import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.util.concurrent.SettableFuture;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
@@ -24,6 +25,7 @@
import com.google.devtools.build.lib.packages.Globber.BadGlobException;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.UnixGlob;
import java.io.IOException;
import java.util.ArrayList;
@@ -95,6 +97,7 @@
public GlobCache(
final Path packageDirectory,
final PackageIdentifier packageId,
+ final ImmutableSet<PathFragment> blacklistedGlobPrefixes,
final CachingPackageLocator locator,
AtomicReference<? extends UnixGlob.FilesystemCalls> syscalls,
Executor globExecutor,
@@ -111,12 +114,18 @@
if (directory.equals(packageDirectory)) {
return true;
}
+
+ PathFragment subPackagePath =
+ packageId.getPackageFragment().getRelative(directory.relativeTo(packageDirectory));
+
+ for (PathFragment blacklistedPrefix : blacklistedGlobPrefixes) {
+ if (subPackagePath.startsWith(blacklistedPrefix)) {
+ return false;
+ }
+ }
+
PackageIdentifier subPackageId =
- PackageIdentifier.create(
- packageId.getRepository(),
- packageId
- .getPackageFragment()
- .getRelative(directory.relativeTo(packageDirectory)));
+ PackageIdentifier.create(packageId.getRepository(), subPackagePath);
return locator.getBuildFileForPackage(subPackageId) == null;
};
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 3ffeeac..82bcc56 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -19,6 +19,7 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
@@ -80,6 +81,7 @@
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.lib.vfs.UnixGlob;
import java.io.IOException;
@@ -1507,7 +1509,8 @@
}
Globber globber =
- createLegacyGlobber(buildFile.asPath().getParentDirectory(), packageId, locator);
+ createLegacyGlobber(
+ buildFile.asPath().getParentDirectory(), packageId, ImmutableSet.of(), locator);
ParserInput input =
ParserInput.create(
FileSystemUtils.convertFromLatin1(buildFileBytes), buildFile.asPath().asFragment());
@@ -1536,11 +1539,13 @@
public LegacyGlobber createLegacyGlobber(
Path packageDirectory,
PackageIdentifier packageId,
+ ImmutableSet<PathFragment> blacklistedGlobPrefixes,
CachingPackageLocator locator) {
return createLegacyGlobber(
new GlobCache(
packageDirectory,
packageId,
+ blacklistedGlobPrefixes,
locator,
syscalls,
executor,
@@ -1552,26 +1557,6 @@
return new LegacyGlobber(globCache, /*sort=*/ true);
}
- /**
- * Returns a new {@link LegacyGlobber}, the same as in {@link #createLegacyGlobber}, except that
- * the implementation of {@link Globber#fetch} intentionally breaks the contract and doesn't
- * return sorted results.
- */
- public LegacyGlobber createLegacyGlobberThatDoesntSort(
- Path packageDirectory,
- PackageIdentifier packageId,
- CachingPackageLocator locator) {
- return new LegacyGlobber(
- new GlobCache(
- packageDirectory,
- packageId,
- locator,
- syscalls,
- executor,
- maxDirectoriesToEagerlyVisitInGlobbing),
- /*sort=*/ false);
- }
-
@Nullable
private byte[] maybeGetBuildFileBytes(Path buildFile, ExtendedEventHandler eventHandler) {
try {
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 cabb54f..8f125a1 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
@@ -56,9 +56,23 @@
throws GlobFunctionException, InterruptedException {
GlobDescriptor glob = (GlobDescriptor) skyKey.argument();
+ BlacklistedPackagePrefixesValue blacklistedPackagePrefixes =
+ (BlacklistedPackagePrefixesValue) env.getValue(BlacklistedPackagePrefixesValue.key());
+ if (env.valuesMissing()) {
+ return null;
+ }
+
+ PathFragment globSubdir = glob.getSubdir();
+ PathFragment dirPathFragment = glob.getPackageId().getPackageFragment().getRelative(globSubdir);
+
+ for (PathFragment blacklistedPrefix : blacklistedPackagePrefixes.getPatterns()) {
+ if (dirPathFragment.startsWith(blacklistedPrefix)) {
+ return GlobValue.EMPTY;
+ }
+ }
+
// 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.
- PathFragment globSubdir = glob.getSubdir();
if (!globSubdir.equals(PathFragment.EMPTY_FRAGMENT)) {
PackageLookupValue globSubdirPkgLookupValue =
(PackageLookupValue)
@@ -99,7 +113,7 @@
boolean globMatchesBareFile = patternTail == null;
- PathFragment dirPathFragment = glob.getPackageId().getPackageFragment().getRelative(globSubdir);
+
RootedPath dirRootedPath = RootedPath.toRootedPath(glob.getPackageRoot(), dirPathFragment);
if (alwaysUseDirListing || containsGlobs(patternHead)) {
// Pattern contains globs, so a directory listing is required.
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 6a2593e..a738236 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
@@ -380,36 +380,24 @@
}
WorkspaceNameValue workspaceNameValue =
(WorkspaceNameValue) env.getValue(WorkspaceNameValue.key());
- if (workspaceNameValue == null) {
- return null;
- }
- String workspaceName = workspaceNameValue.getName();
RepositoryMappingValue repositoryMappingValue =
(RepositoryMappingValue)
env.getValue(RepositoryMappingValue.key(packageId.getRepository()));
- if (repositoryMappingValue == null) {
- return null;
- }
- ImmutableMap<RepositoryName, RepositoryName> repositoryMapping =
- repositoryMappingValue.getRepositoryMapping();
-
RootedPath buildFileRootedPath = packageLookupValue.getRootedPath(packageId);
FileValue buildFileValue = getBuildFileValue(env, buildFileRootedPath);
- if (buildFileValue == null) {
- return null;
- }
-
RuleVisibility defaultVisibility = PrecomputedValue.DEFAULT_VISIBILITY.get(env);
- if (defaultVisibility == null) {
+ StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
+ BlacklistedPackagePrefixesValue blacklistedPackagePrefixes =
+ (BlacklistedPackagePrefixesValue) env.getValue(BlacklistedPackagePrefixesValue.key());
+ if (env.valuesMissing()) {
return null;
}
- StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
- if (starlarkSemantics == null) {
- return null;
- }
+ String workspaceName = workspaceNameValue.getName();
+ ImmutableMap<RepositoryName, RepositoryName> repositoryMapping =
+ repositoryMappingValue.getRepositoryMapping();
// Load the prelude from the same repository as the package being loaded. Can't use
// Label.resolveRepositoryRelative because preludeLabel is in the main repository, not the
@@ -452,6 +440,7 @@
loadPackage(
workspaceName,
repositoryMapping,
+ blacklistedPackagePrefixes.getPatterns(),
packageId,
buildFileRootedPath,
buildFileValue,
@@ -1102,10 +1091,12 @@
private GlobberWithSkyframeGlobDeps makeGlobber(
Path buildFilePath,
PackageIdentifier packageId,
+ ImmutableSet<PathFragment> blacklistedGlobPrefixes,
Root packageRoot,
SkyFunction.Environment env) {
- LegacyGlobber legacyGlobber = packageFactory.createLegacyGlobber(
- buildFilePath.getParentDirectory(), packageId, packageLocator);
+ LegacyGlobber legacyGlobber =
+ packageFactory.createLegacyGlobber(
+ buildFilePath.getParentDirectory(), packageId, blacklistedGlobPrefixes, packageLocator);
switch (incrementalityIntent) {
case INCREMENTAL:
return new SkyframeHybridGlobber(packageId, packageRoot, env, legacyGlobber);
@@ -1133,6 +1124,7 @@
private LoadedPackageCacheEntry loadPackage(
String workspaceName,
ImmutableMap<RepositoryName, RepositoryName> repositoryMapping,
+ ImmutableSet<PathFragment> blacklistedGlobPrefixes,
PackageIdentifier packageId,
RootedPath buildFilePath,
@Nullable FileValue buildFileValue,
@@ -1212,7 +1204,7 @@
// Therefore, it is safe to invalidate the astCache entry for this packageId here.
astCache.invalidate(packageId);
GlobberWithSkyframeGlobDeps globberWithSkyframeGlobDeps =
- makeGlobber(inputFile, packageId, packageRoot, env);
+ makeGlobber(inputFile, packageId, blacklistedGlobPrefixes, packageRoot, env);
long startTimeNanos = BlazeClock.nanoTime();
Package.Builder pkgBuilder =
packageFactory.createPackageFromAst(
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 1ea1658c..f8eeb1f 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
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.packages.Globber.BadGlobException;
@@ -23,6 +24,7 @@
import com.google.devtools.build.lib.testutil.TestUtils;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
@@ -86,10 +88,15 @@
scratch.file("isolated/sub/sub.js",
"# this is sub/sub.js");
+ createCache();
+ }
+
+ private void createCache(PathFragment... blacklistedDirectories) {
cache =
new GlobCache(
packageDirectory,
PackageIdentifier.createInMainRepo("isolated"),
+ ImmutableSet.copyOf(blacklistedDirectories),
new CachingPackageLocator() {
@Override
public Path getBuildFileForPackage(PackageIdentifier packageId) {
@@ -114,6 +121,18 @@
}
@Test
+ public void testBlacklistedDirectory() throws Exception {
+ createCache(PathFragment.create("isolated/foo"));
+ List<Path> paths = cache.safeGlobUnsorted("**/*.js", true).get();
+ assertPathsAre(
+ paths,
+ "/workspace/isolated/first.js",
+ "/workspace/isolated/second.js",
+ "/workspace/isolated/bar/first.js",
+ "/workspace/isolated/bar/second.js");
+ }
+
+ @Test
public void testSafeGlob() throws Exception {
List<Path> paths = cache.safeGlobUnsorted("*.js", false).get();
assertPathsAre(paths,
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
index e1b2bdb..fd9a311 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
@@ -15,6 +15,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
@@ -152,6 +153,7 @@
new GlobCache(
buildFile.asPath().getParentDirectory(),
packageId,
+ ImmutableSet.of(),
getPackageLocator(),
null,
TestUtils.getPool(),
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java
index ec94250..c79eda7 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java
@@ -18,6 +18,7 @@
import static org.junit.Assert.fail;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.events.Event;
@@ -113,6 +114,7 @@
new GlobCache(
pkg.getFilename().asPath().getParentDirectory(),
pkg.getPackageIdentifier(),
+ ImmutableSet.of(),
PackageFactoryApparatus.createEmptyLocator(),
null,
TestUtils.getPool(),
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 3e5f330..f5ee322 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
@@ -170,10 +170,11 @@
deletedPackages,
CrossRepositoryLabelViolationStrategy.ERROR,
BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY));
- skyFunctions.put(SkyFunctions.BLACKLISTED_PACKAGE_PREFIXES,
+ skyFunctions.put(
+ SkyFunctions.BLACKLISTED_PACKAGE_PREFIXES,
new BlacklistedPackagePrefixesFunction(
/*hardcodedBlacklistedPackagePrefixes=*/ ImmutableSet.of(),
- /*additionalBlacklistedPackagePrefixesFile=*/ PathFragment.EMPTY_FRAGMENT));
+ BazelSkyframeExecutorConstants.ADDITIONAL_BLACKLISTED_PACKAGE_PREFIXES_FILE));
skyFunctions.put(
FileStateValue.FILE_STATE,
new FileStateFunction(
@@ -229,6 +230,27 @@
}
@Test
+ public void testBlacklist() throws Exception {
+ FileSystemUtils.writeContentAsLatin1(root.getRelative(".bazelignore"), "pkg/foo/bar");
+ assertGlobMatches("foo/**", "foo/barnacle/wiz", "foo/barnacle", "foo");
+ differencer.invalidate(
+ ImmutableList.of(
+ FileStateValue.key(
+ RootedPath.toRootedPath(
+ Root.fromPath(root), PathFragment.create(".bazelignore")))));
+
+ FileSystemUtils.createEmptyFile(root.getRelative(".bazelignore"));
+ assertGlobMatches(
+ "foo/**",
+ "foo/bar/wiz",
+ "foo/bar/wiz/file",
+ "foo/bar",
+ "foo/barnacle/wiz",
+ "foo/barnacle",
+ "foo");
+ }
+
+ @Test
public void testStartsWithStar() throws Exception {
assertGlobMatches("*oo", /* => */ "foo");
}
diff --git a/src/test/shell/bazel/bazelignore_test.sh b/src/test/shell/bazel/bazelignore_test.sh
index a294a38..e144ab9 100755
--- a/src/test/shell/bazel/bazelignore_test.sh
+++ b/src/test/shell/bazel/bazelignore_test.sh
@@ -25,6 +25,41 @@
set -e
+test_does_not_glob_into_ignored_directory() {
+ rm -rf work && mkdir work && cd work
+ create_workspace_with_default_repos WORKSPACE
+
+ echo 'filegroup(name="f", srcs=glob(["**"]))' > BUILD
+ echo 'ignored' > .bazelignore
+ mkdir -p ignored/pkg
+ echo 'filegroup(name="f", srcs=glob(["**"]))' > ignored/pkg/BUILD
+ touch ignored/pkg/a
+ touch ignored/file
+ bazel query //:all-targets > "$TEST_TMPDIR/targets"
+ assert_not_contains "//:ignored/file" "$TEST_TMPDIR/targets"
+ assert_not_contains "//:ignored/pkg/BUILD" "$TEST_TMPDIR/targets"
+ assert_not_contains "//:ignored/pkg/a" "$TEST_TMPDIR/targets"
+
+ # This weird line tests whether .bazelignore also stops the Skyframe-based
+ # glob. Globbing (as of 2019 Oct) is done in a hybrid fashion: we do the
+ # legacy globbing because it's faster and Skyframe globbing because it's
+ # more incremental. In the first run, we get the results of the legacy
+ # globbing, but if we invalidate the BUILD file, the result of the legacy
+ # glob is invalidated and but the better incrementality allows the result of
+ # the Skyframe glob to be cached.
+ echo "# change" >> BUILD
+ bazel query //:all-targets > "$TEST_TMPDIR/targets"
+ assert_not_contains "//:ignored/file" "$TEST_TMPDIR/targets"
+ assert_not_contains "//:ignored/pkg/BUILD" "$TEST_TMPDIR/targets"
+ assert_not_contains "//:ignored/pkg/a" "$TEST_TMPDIR/targets"
+
+ echo > .bazelignore
+ bazel query //:all-targets > "$TEST_TMPDIR/targets"
+ assert_contains "//:ignored/file" "$TEST_TMPDIR/targets"
+ bazel query //ignored/pkg:all-targets > "$TEST_TMPDIR/targets"
+ assert_contains "//ignored/pkg:a" "$TEST_TMPDIR/targets"
+}
+
test_broken_BUILD_files_ignored() {
rm -rf work && mkdir work && cd work
create_workspace_with_default_repos WORKSPACE
