From b9e8174494f267798197f5e85f46fdb8f2405b5a Mon Sep 17 00:00:00 2001 From: Yun Peng Date: Mon, 25 Nov 2019 06:44:26 -0800 Subject: [PATCH 1/3] Support .bazelignore file for external repo RELNOTES[NEW]: Similar to the [.bazelignore](https://docs.bazel.build/versions/master/guide.html#.bazelignore) in the main repository, a `.bazelignore` file in external repository will cause the specified directories to be ignored by Bazel. Bazel won't try to identify any packages under the directories, but the files can still be referenced in other BUILD files. Fixes https://github.com/bazelbuild/bazel/issues/10234 Fixes https://github.com/bazelbuild/bazel/issues/9080 Closes #10261. PiperOrigin-RevId: 282347073 Change-Id: I6322054e8680acf7dffcfbf85104fb17c6481834 --- .../skylark/SkylarkRepositoryFunction.java | 5 +- .../build/lib/cmdline/TargetPattern.java | 17 +++-- .../BlacklistedPackagePrefixesFunction.java | 65 ++++++++++++++----- .../BlacklistedPackagePrefixesValue.java | 40 ++++++++++-- .../build/lib/skyframe/GlobFunction.java | 3 +- .../build/lib/skyframe/PackageFunction.java | 3 +- .../lib/skyframe/PackageLookupFunction.java | 34 ++++++++-- .../PrepareDepsOfPatternFunction.java | 18 +++-- .../lib/skyframe/TargetPatternFunction.java | 23 +++++-- .../bazel/bazel_external_repository_test.py | 54 +++++++++++++++ 10 files changed, 212 insertions(+), 50 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java index ec1c2fd3bf027c..65d74a504520c6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java @@ -112,7 +112,10 @@ public RepositoryDirectoryValue.Builder fetch( } BlacklistedPackagePrefixesValue blacklistedPackagesValue = - (BlacklistedPackagePrefixesValue) env.getValue(BlacklistedPackagePrefixesValue.key()); + (BlacklistedPackagePrefixesValue) + env.getValue( + BlacklistedPackagePrefixesValue.key( + rule.getPackage().getPackageIdentifier().getRepository())); if (env.valuesMissing()) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java index f6f92150905858..cd3ffdbfa42acb 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java @@ -65,6 +65,7 @@ public abstract class TargetPattern implements Serializable { private final Type type; private final String originalPattern; private final String offset; + private final RepositoryName repository; /** * Returns a parser with no offset. Note that the Parser class is immutable, so this method may @@ -117,11 +118,13 @@ static String normalize(String path) { return SLASH_JOINER.join(pieces); } - private TargetPattern(Type type, String originalPattern, String offset) { + private TargetPattern( + Type type, String originalPattern, String offset, RepositoryName repository) { // Don't allow inheritance outside this class. this.type = type; this.originalPattern = Preconditions.checkNotNull(originalPattern); this.offset = Preconditions.checkNotNull(offset); + this.repository = repository; } /** @@ -300,6 +303,10 @@ public PackageIdentifier getDirectoryForTargetOrTargetsInPackage() { throw new IllegalStateException(); } + public RepositoryName getRepository() { + return this.repository; + } + /** * Returns {@code true} iff this pattern has type {@code Type.TARGETS_BELOW_DIRECTORY} or * {@code Type.TARGETS_IN_PACKAGE} and the target pattern suffix specified it should match @@ -314,7 +321,7 @@ private static final class SingleTarget extends TargetPattern { private SingleTarget( String targetName, PackageIdentifier directory, String originalPattern, String offset) { - super(Type.SINGLE_TARGET, originalPattern, offset); + super(Type.SINGLE_TARGET, originalPattern, offset, directory.getRepository()); this.targetName = Preconditions.checkNotNull(targetName); this.directory = Preconditions.checkNotNull(directory); } @@ -372,7 +379,7 @@ private static final class InterpretPathAsTarget extends TargetPattern { private final String path; private InterpretPathAsTarget(String path, String originalPattern, String offset) { - super(Type.PATH_AS_TARGET, originalPattern, offset); + super(Type.PATH_AS_TARGET, originalPattern, offset, null); this.path = normalize(Preconditions.checkNotNull(path)); } @@ -452,7 +459,7 @@ private static final class TargetsInPackage extends TargetPattern { private TargetsInPackage(String originalPattern, String offset, PackageIdentifier packageIdentifier, String suffix, boolean wasOriginallyAbsolute, boolean rulesOnly, boolean checkWildcardConflict) { - super(Type.TARGETS_IN_PACKAGE, originalPattern, offset); + super(Type.TARGETS_IN_PACKAGE, originalPattern, offset, packageIdentifier.getRepository()); Preconditions.checkArgument(!packageIdentifier.getRepository().isDefault()); this.packageIdentifier = packageIdentifier; this.suffix = Preconditions.checkNotNull(suffix); @@ -562,7 +569,7 @@ private static final class TargetsBelowDirectory extends TargetPattern { private TargetsBelowDirectory( String originalPattern, String offset, PackageIdentifier directory, boolean rulesOnly) { - super(Type.TARGETS_BELOW_DIRECTORY, originalPattern, offset); + super(Type.TARGETS_BELOW_DIRECTORY, originalPattern, offset, directory.getRepository()); Preconditions.checkArgument(!directory.getRepository().isDefault()); this.directory = Preconditions.checkNotNull(directory); this.rulesOnly = rulesOnly; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesFunction.java index eb0f64598722b1..f2488d969dc7c7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesFunction.java @@ -18,7 +18,9 @@ import com.google.common.io.LineProcessor; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.actions.InconsistentFilesystemException; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; +import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.RootedPath; @@ -46,10 +48,30 @@ public BlacklistedPackagePrefixesFunction( this.additionalBlacklistedPackagePrefixesFile = additionalBlacklistedPackagePrefixesFile; } + private static void getBlacklistedPackagePrefixes( + RootedPath patternFile, ImmutableSet.Builder blacklistedPackagePrefixesBuilder) + throws BlacklistedPatternsFunctionException { + try (InputStreamReader reader = + new InputStreamReader(patternFile.asPath().getInputStream(), StandardCharsets.UTF_8)) { + blacklistedPackagePrefixesBuilder.addAll( + CharStreams.readLines(reader, new PathFragmentLineProcessor())); + } catch (IOException e) { + String errorMessage = e.getMessage() != null ? "error '" + e.getMessage() + "'" : "an error"; + throw new BlacklistedPatternsFunctionException( + new InconsistentFilesystemException( + patternFile.asPath() + + " is not readable because: " + + errorMessage + + ". Was it modified mid-build?")); + } + } + @Nullable @Override public SkyValue compute(SkyKey key, Environment env) throws SkyFunctionException, InterruptedException { + RepositoryName repositoryName = (RepositoryName) key.argument(); + ImmutableSet.Builder blacklistedPackagePrefixesBuilder = ImmutableSet.builder(); blacklistedPackagePrefixesBuilder.addAll(hardcodedBlacklistedPackagePrefixes); @@ -60,30 +82,37 @@ public SkyValue compute(SkyKey key, Environment env) return null; } - for (Root packagePathEntry : pkgLocator.getPathEntries()) { + if (repositoryName.isMain()) { + for (Root packagePathEntry : pkgLocator.getPathEntries()) { + RootedPath rootedPatternFile = + RootedPath.toRootedPath(packagePathEntry, additionalBlacklistedPackagePrefixesFile); + FileValue patternFileValue = (FileValue) env.getValue(FileValue.key(rootedPatternFile)); + if (patternFileValue == null) { + return null; + } + if (patternFileValue.isFile()) { + getBlacklistedPackagePrefixes(rootedPatternFile, blacklistedPackagePrefixesBuilder); + break; + } + } + } else { + // Make sure the repository is fetched. + RepositoryDirectoryValue repositoryValue = + (RepositoryDirectoryValue) env.getValue(RepositoryDirectoryValue.key(repositoryName)); + if (repositoryValue == null) { + return null; + } RootedPath rootedPatternFile = - RootedPath.toRootedPath(packagePathEntry, additionalBlacklistedPackagePrefixesFile); + RootedPath.toRootedPath( + Root.fromPath( + pkgLocator.getOutputBase().getRelative(repositoryName.getSourceRoot())), + additionalBlacklistedPackagePrefixesFile); FileValue patternFileValue = (FileValue) env.getValue(FileValue.key(rootedPatternFile)); if (patternFileValue == null) { return null; } if (patternFileValue.isFile()) { - try { - try (InputStreamReader reader = - new InputStreamReader(rootedPatternFile.asPath().getInputStream(), - StandardCharsets.UTF_8)) { - blacklistedPackagePrefixesBuilder.addAll( - CharStreams.readLines(reader, new PathFragmentLineProcessor())); - break; - } - } catch (IOException e) { - String errorMessage = e.getMessage() != null - ? "error '" + e.getMessage() + "'" : "an error"; - throw new BlacklistedPatternsFunctionException( - new InconsistentFilesystemException( - rootedPatternFile.asPath() + " is not readable because: " + errorMessage - + ". Was it modified mid-build?")); - } + getBlacklistedPackagePrefixes(rootedPatternFile, blacklistedPackagePrefixesBuilder); } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesValue.java index 13494812caf2d5..8de5c033d75dcd 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesValue.java @@ -15,9 +15,13 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Interner; +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.AbstractSkyKey; +import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyValue; /** An immutable set of package name prefixes that should be blacklisted. */ @@ -25,15 +29,18 @@ public class BlacklistedPackagePrefixesValue implements SkyValue { private final ImmutableSet patterns; - @AutoCodec.VisibleForSerialization @AutoCodec - static final SkyKey BLACKLIST_KEY = () -> SkyFunctions.BLACKLISTED_PACKAGE_PREFIXES; - public BlacklistedPackagePrefixesValue(ImmutableSet patterns) { this.patterns = Preconditions.checkNotNull(patterns); } - public static SkyKey key() { - return BLACKLIST_KEY; + /** Creates a key from the main repository. */ + public static Key key() { + return Key.create(RepositoryName.MAIN); + } + + /** Creates a key from the given repository name. */ + public static Key key(RepositoryName repository) { + return Key.create(repository); } public ImmutableSet getPatterns() { @@ -53,4 +60,25 @@ public boolean equals(Object obj) { } return false; } + + @AutoCodec.VisibleForSerialization + @AutoCodec + static class Key extends AbstractSkyKey { + private static final Interner interner = BlazeInterners.newWeakInterner(); + + private Key(RepositoryName arg) { + super(arg); + } + + @AutoCodec.VisibleForSerialization + @AutoCodec.Instantiator + static Key create(RepositoryName arg) { + return interner.intern(new Key(arg)); + } + + @Override + public SkyFunctionName functionName() { + return SkyFunctions.BLACKLISTED_PACKAGE_PREFIXES; + } + } } 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 8f125a19148a5a..0f82f3a673c060 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 @@ -57,7 +57,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) GlobDescriptor glob = (GlobDescriptor) skyKey.argument(); BlacklistedPackagePrefixesValue blacklistedPackagePrefixes = - (BlacklistedPackagePrefixesValue) env.getValue(BlacklistedPackagePrefixesValue.key()); + (BlacklistedPackagePrefixesValue) + env.getValue(BlacklistedPackagePrefixesValue.key(glob.getPackageId().getRepository())); if (env.valuesMissing()) { return null; } 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 34dd686895d98f..4ff10c577eba76 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 @@ -412,7 +412,8 @@ public SkyValue compute(SkyKey key, Environment env) throws PackageFunctionExcep RuleVisibility defaultVisibility = PrecomputedValue.DEFAULT_VISIBILITY.get(env); StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); BlacklistedPackagePrefixesValue blacklistedPackagePrefixes = - (BlacklistedPackagePrefixesValue) env.getValue(BlacklistedPackagePrefixesValue.key()); + (BlacklistedPackagePrefixesValue) + env.getValue(BlacklistedPackagePrefixesValue.key(packageId.getRepository())); if (env.valuesMissing()) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java index d1a20d33af527e..55054d6d11b66e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java @@ -90,17 +90,16 @@ public SkyValue compute(SkyKey skyKey, Environment env) return computeWorkspacePackageLookupValue(env, pkgLocator.getPathEntries()); } + // Check .bazelignore file under main repository. BlacklistedPackagePrefixesValue blacklistedPatternsValue = - (BlacklistedPackagePrefixesValue) env.getValue(BlacklistedPackagePrefixesValue.key()); + (BlacklistedPackagePrefixesValue) + env.getValue(BlacklistedPackagePrefixesValue.key(packageKey.getRepository())); if (blacklistedPatternsValue == null) { return null; } - PathFragment buildFileFragment = packageKey.getPackageFragment(); - for (PathFragment pattern : blacklistedPatternsValue.getPatterns()) { - if (buildFileFragment.startsWith(pattern)) { - return PackageLookupValue.DELETED_PACKAGE_VALUE; - } + if (isPackageIgnored(packageKey, blacklistedPatternsValue)) { + return PackageLookupValue.DELETED_PACKAGE_VALUE; } return findPackageByBuildFile(env, pkgLocator, packageKey); @@ -292,6 +291,17 @@ private PackageLookupValue getPackageLookupValue( return PackageLookupValue.NO_BUILD_FILE_VALUE; } + private static boolean isPackageIgnored( + PackageIdentifier id, BlacklistedPackagePrefixesValue blacklistedPatternsValue) { + PathFragment packageFragment = id.getPackageFragment(); + for (PathFragment pattern : blacklistedPatternsValue.getPatterns()) { + if (packageFragment.startsWith(pattern)) { + return true; + } + } + return false; + } + private PackageLookupValue computeWorkspacePackageLookupValue( Environment env, ImmutableList packagePathEntries) throws PackageLookupFunctionException, InterruptedException { @@ -369,6 +379,18 @@ private PackageLookupValue computeExternalPackageLookupValue( return new PackageLookupValue.NoRepositoryPackageLookupValue(id.getRepository().getName()); } + // Check .bazelignore file after fetching the external repository. + BlacklistedPackagePrefixesValue blacklistedPatternsValue = + (BlacklistedPackagePrefixesValue) + env.getValue(BlacklistedPackagePrefixesValue.key(id.getRepository())); + if (blacklistedPatternsValue == null) { + return null; + } + + if (isPackageIgnored(id, blacklistedPatternsValue)) { + return PackageLookupValue.DELETED_PACKAGE_VALUE; + } + // This checks for the build file names in the correct precedence order. for (BuildFileName buildFileName : buildFilesByPriority) { PathFragment buildFileFragment = diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java index 35d0a5e61bb9c0..f6f7e21b469809 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java @@ -83,18 +83,26 @@ public SkyValue compute(SkyKey key, Environment env) TargetPattern parsedPattern = patternKey.getParsedPattern(); - BlacklistedPackagePrefixesValue blacklist = - (BlacklistedPackagePrefixesValue) env.getValue(BlacklistedPackagePrefixesValue.key()); - if (blacklist == null) { - return null; + ImmutableSet blacklistedPatterns; + if (parsedPattern.getRepository() == null) { + blacklistedPatterns = ImmutableSet.of(); + } else { + BlacklistedPackagePrefixesValue blacklist = + (BlacklistedPackagePrefixesValue) + env.getValue(BlacklistedPackagePrefixesValue.key(parsedPattern.getRepository())); + if (blacklist == null) { + return null; + } + blacklistedPatterns = blacklist.getPatterns(); } + // This SkyFunction is used to load the universe, so we want both the blacklisted directories // from the global blacklist and the excluded directories from the TargetPatternKey itself to be // embedded in the SkyKeys created and used by the DepsOfPatternPreparer. The // DepsOfPatternPreparer ignores excludedSubdirectories and embeds blacklistedSubdirectories in // the SkyKeys it creates and uses. ImmutableSet blacklistedSubdirectories = - patternKey.getAllSubdirectoriesToExclude(blacklist.getPatterns()); + patternKey.getAllSubdirectoriesToExclude(blacklistedPatterns); ImmutableSet excludedSubdirectories = ImmutableSet.of(); DepsOfPatternPreparer preparer = diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java index ea21b36a07e42d..245fc9d17c625b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java @@ -44,13 +44,23 @@ public TargetPatternFunction() { @Override public SkyValue compute(SkyKey key, Environment env) throws TargetPatternFunctionException, InterruptedException { - BlacklistedPackagePrefixesValue blacklisted = - (BlacklistedPackagePrefixesValue) env.getValue(BlacklistedPackagePrefixesValue.key()); - if (env.valuesMissing()) { - return null; - } TargetPatternValue.TargetPatternKey patternKey = ((TargetPatternValue.TargetPatternKey) key.argument()); + TargetPattern parsedPattern = patternKey.getParsedPattern(); + + ImmutableSet blacklistedPatterns; + if (parsedPattern.getRepository() == null) { + blacklistedPatterns = ImmutableSet.of(); + } else { + BlacklistedPackagePrefixesValue blacklist = + (BlacklistedPackagePrefixesValue) + env.getValue(BlacklistedPackagePrefixesValue.key(parsedPattern.getRepository())); + if (blacklist == null) { + return null; + } + blacklistedPatterns = blacklist.getPatterns(); + } + ResolvedTargets resolvedTargets; try { EnvironmentBackedRecursivePackageProvider provider = @@ -61,7 +71,6 @@ public SkyValue compute(SkyKey key, Environment env) throws TargetPatternFunctio env.getListener(), patternKey.getPolicy(), MultisetSemaphore.unbounded()); - TargetPattern parsedPattern = patternKey.getParsedPattern(); ImmutableSet excludedSubdirectories = patternKey.getExcludedSubdirectories(); ResolvedTargets.Builder resolvedTargetsBuilder = ResolvedTargets.builder(); BatchCallback callback = @@ -75,7 +84,7 @@ public void process(Iterable partialResult) { }; parsedPattern.eval( resolver, - blacklisted.getPatterns(), + blacklistedPatterns, excludedSubdirectories, callback, // The exception type here has to match the one on the BatchCallback. Since the callback diff --git a/src/test/py/bazel/bazel_external_repository_test.py b/src/test/py/bazel/bazel_external_repository_test.py index f4b14f96f993e2..3232e0f177f6b4 100644 --- a/src/test/py/bazel/bazel_external_repository_test.py +++ b/src/test/py/bazel/bazel_external_repository_test.py @@ -217,6 +217,60 @@ def testDeletedPackagesOnExternalRepo(self): cwd=work_dir) self.AssertExitCode(exit_code, 0, stderr) + def testBazelignoreFileOnExternalRepo(self): + self.ScratchFile('other_repo/WORKSPACE') + self.ScratchFile('other_repo/pkg/BUILD', [ + 'filegroup(', + ' name = "file",', + ' srcs = ["ignore/file"],', + ')', + ]) + self.ScratchFile('other_repo/pkg/ignore/BUILD', [ + 'Bad BUILD file', + ]) + self.ScratchFile('other_repo/pkg/ignore/file') + work_dir = self.ScratchDir('my_repo') + self.ScratchFile('my_repo/WORKSPACE', [ + "local_repository(name = 'other_repo', path='../other_repo')", + ]) + + exit_code, _, stderr = self.RunBazel( + args=['build', '@other_repo//pkg:file'], cwd=work_dir) + self.AssertExitCode(exit_code, 1, stderr) + self.assertIn('\'@other_repo//pkg/ignore\' is a subpackage', + ''.join(stderr)) + + self.ScratchFile('other_repo/.bazelignore', [ + 'pkg/ignore', + ]) + + exit_code, _, stderr = self.RunBazel( + args=['build', '@other_repo//pkg:file'], cwd=work_dir) + self.AssertExitCode(exit_code, 0, stderr) + + def testBazelignoreFileFromMainRepoDoesNotAffectExternalRepos(self): + # Regression test for https://github.com/bazelbuild/bazel/issues/10234 + self.ScratchFile('other_repo/WORKSPACE') + self.ScratchFile('other_repo/foo/bar/BUILD', [ + 'filegroup(', + ' name = "file",', + ' srcs = ["file.txt"],', + ')', + ]) + self.ScratchFile('other_repo/foo/bar/file.txt') + + work_dir = self.ScratchDir('my_repo') + self.ScratchFile('my_repo/WORKSPACE', [ + "local_repository(name = 'other_repo', path='../other_repo')", + ]) + # This should not exclude @other_repo//foo/bar + self.ScratchFile('my_repo/.bazelignore', ['foo/bar']) + + exit_code, stdout, stderr = self.RunBazel( + args=['query', '@other_repo//foo/bar/...'], cwd=work_dir) + self.AssertExitCode(exit_code, 0, stderr) + self.assertIn('@other_repo//foo/bar:file', ''.join(stdout)) + if __name__ == '__main__': unittest.main() From ad26ea9700284b123629f854b62f3005c602c841 Mon Sep 17 00:00:00 2001 From: Yun Peng Date: Fri, 29 Nov 2019 11:10:45 +0100 Subject: [PATCH 2/3] Adrress reviewer comments --- .../skylark/SkylarkRepositoryFunction.java | 4 +- .../build/lib/cmdline/TargetPattern.java | 38 +++++++++++++------ .../BlacklistedPackagePrefixesFunction.java | 30 ++++++++------- .../BlacklistedPackagePrefixesValue.java | 16 ++++++-- .../build/lib/skyframe/GlobFunction.java | 6 ++- .../lib/skyframe/PackageLookupFunction.java | 3 +- .../PrepareDepsOfPatternFunction.java | 17 +++------ .../lib/skyframe/TargetPatternFunction.java | 17 +++------ 8 files changed, 75 insertions(+), 56 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java index 65d74a504520c6..38161afca59259 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java @@ -114,8 +114,8 @@ public RepositoryDirectoryValue.Builder fetch( BlacklistedPackagePrefixesValue blacklistedPackagesValue = (BlacklistedPackagePrefixesValue) env.getValue( - BlacklistedPackagePrefixesValue.key( - rule.getPackage().getPackageIdentifier().getRepository())); + BlacklistedPackagePrefixesValue.key()); + if (env.valuesMissing()) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java index cd3ffdbfa42acb..ed01bd80a4715a 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java @@ -65,7 +65,6 @@ public abstract class TargetPattern implements Serializable { private final Type type; private final String originalPattern; private final String offset; - private final RepositoryName repository; /** * Returns a parser with no offset. Note that the Parser class is immutable, so this method may @@ -118,13 +117,11 @@ static String normalize(String path) { return SLASH_JOINER.join(pieces); } - private TargetPattern( - Type type, String originalPattern, String offset, RepositoryName repository) { + private TargetPattern(Type type, String originalPattern, String offset) { // Don't allow inheritance outside this class. this.type = type; this.originalPattern = Preconditions.checkNotNull(originalPattern); this.offset = Preconditions.checkNotNull(offset); - this.repository = repository; } /** @@ -303,9 +300,8 @@ public PackageIdentifier getDirectoryForTargetOrTargetsInPackage() { throw new IllegalStateException(); } - public RepositoryName getRepository() { - return this.repository; - } + /** Returns the repository name of the target path. */ + public abstract RepositoryName getRepository(); /** * Returns {@code true} iff this pattern has type {@code Type.TARGETS_BELOW_DIRECTORY} or @@ -321,7 +317,7 @@ private static final class SingleTarget extends TargetPattern { private SingleTarget( String targetName, PackageIdentifier directory, String originalPattern, String offset) { - super(Type.SINGLE_TARGET, originalPattern, offset, directory.getRepository()); + super(Type.SINGLE_TARGET, originalPattern, offset); this.targetName = Preconditions.checkNotNull(targetName); this.directory = Preconditions.checkNotNull(directory); } @@ -346,6 +342,11 @@ public PackageIdentifier getDirectoryForTargetOrTargetsInPackage() { return directory; } + @Override + public RepositoryName getRepository() { + return directory.getRepository(); + } + @Override public boolean getRulesOnly() { return false; @@ -379,7 +380,7 @@ private static final class InterpretPathAsTarget extends TargetPattern { private final String path; private InterpretPathAsTarget(String path, String originalPattern, String offset) { - super(Type.PATH_AS_TARGET, originalPattern, offset, null); + super(Type.PATH_AS_TARGET, originalPattern, offset); this.path = normalize(Preconditions.checkNotNull(path)); } @@ -425,6 +426,11 @@ public String getPathForPathAsTarget() { return path; } + @Override + public RepositoryName getRepository() { + return RepositoryName.MAIN; + } + @Override public boolean getRulesOnly() { return false; @@ -459,7 +465,7 @@ private static final class TargetsInPackage extends TargetPattern { private TargetsInPackage(String originalPattern, String offset, PackageIdentifier packageIdentifier, String suffix, boolean wasOriginallyAbsolute, boolean rulesOnly, boolean checkWildcardConflict) { - super(Type.TARGETS_IN_PACKAGE, originalPattern, offset, packageIdentifier.getRepository()); + super(Type.TARGETS_IN_PACKAGE, originalPattern, offset); Preconditions.checkArgument(!packageIdentifier.getRepository().isDefault()); this.packageIdentifier = packageIdentifier; this.suffix = Preconditions.checkNotNull(suffix); @@ -497,6 +503,11 @@ public PackageIdentifier getDirectoryForTargetOrTargetsInPackage() { return packageIdentifier; } + @Override + public RepositoryName getRepository() { + return packageIdentifier.getRepository(); + } + @Override public boolean getRulesOnly() { return rulesOnly; @@ -569,7 +580,7 @@ private static final class TargetsBelowDirectory extends TargetPattern { private TargetsBelowDirectory( String originalPattern, String offset, PackageIdentifier directory, boolean rulesOnly) { - super(Type.TARGETS_BELOW_DIRECTORY, originalPattern, offset, directory.getRepository()); + super(Type.TARGETS_BELOW_DIRECTORY, originalPattern, offset); Preconditions.checkArgument(!directory.getRepository().isDefault()); this.directory = Preconditions.checkNotNull(directory); this.rulesOnly = rulesOnly; @@ -628,6 +639,11 @@ public PackageIdentifier getDirectoryForTargetsUnderDirectory() { return directory; } + @Override + public RepositoryName getRepository() { + return directory.getRepository(); + } + @Override public boolean getRulesOnly() { return rulesOnly; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesFunction.java index f2488d969dc7c7..9beff062a4511d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesFunction.java @@ -70,7 +70,10 @@ private static void getBlacklistedPackagePrefixes( @Override public SkyValue compute(SkyKey key, Environment env) throws SkyFunctionException, InterruptedException { - RepositoryName repositoryName = (RepositoryName) key.argument(); + RepositoryName repositoryName = null; + if (key.argument() != null && key.argument() instanceof RepositoryName) { + repositoryName = (RepositoryName) key.argument(); + } ImmutableSet.Builder blacklistedPackagePrefixesBuilder = ImmutableSet.builder(); @@ -82,7 +85,7 @@ public SkyValue compute(SkyKey key, Environment env) return null; } - if (repositoryName.isMain()) { + if (repositoryName == null || repositoryName.isMain()) { for (Root packagePathEntry : pkgLocator.getPathEntries()) { RootedPath rootedPatternFile = RootedPath.toRootedPath(packagePathEntry, additionalBlacklistedPackagePrefixesFile); @@ -102,17 +105,18 @@ public SkyValue compute(SkyKey key, Environment env) if (repositoryValue == null) { return null; } - RootedPath rootedPatternFile = - RootedPath.toRootedPath( - Root.fromPath( - pkgLocator.getOutputBase().getRelative(repositoryName.getSourceRoot())), - additionalBlacklistedPackagePrefixesFile); - FileValue patternFileValue = (FileValue) env.getValue(FileValue.key(rootedPatternFile)); - if (patternFileValue == null) { - return null; - } - if (patternFileValue.isFile()) { - getBlacklistedPackagePrefixes(rootedPatternFile, blacklistedPackagePrefixesBuilder); + if (repositoryValue.repositoryExists()) { + RootedPath rootedPatternFile = + RootedPath.toRootedPath( + Root.fromPath(repositoryValue.getPath()), + additionalBlacklistedPackagePrefixesFile); + FileValue patternFileValue = (FileValue) env.getValue(FileValue.key(rootedPatternFile)); + if (patternFileValue == null) { + return null; + } + if (patternFileValue.isFile()) { + getBlacklistedPackagePrefixes(rootedPatternFile, blacklistedPackagePrefixesBuilder); + } } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesValue.java index 8de5c033d75dcd..7febc7097c3508 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BlacklistedPackagePrefixesValue.java @@ -22,6 +22,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.AbstractSkyKey; import com.google.devtools.build.skyframe.SkyFunctionName; +import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; /** An immutable set of package name prefixes that should be blacklisted. */ @@ -29,18 +30,25 @@ public class BlacklistedPackagePrefixesValue implements SkyValue { private final ImmutableSet patterns; + @AutoCodec.VisibleForSerialization @AutoCodec + static final SkyKey BLACKLIST_KEY = () -> SkyFunctions.BLACKLISTED_PACKAGE_PREFIXES; + public BlacklistedPackagePrefixesValue(ImmutableSet patterns) { this.patterns = Preconditions.checkNotNull(patterns); } /** Creates a key from the main repository. */ - public static Key key() { - return Key.create(RepositoryName.MAIN); + public static SkyKey key() { + return BLACKLIST_KEY; } /** Creates a key from the given repository name. */ - public static Key key(RepositoryName repository) { - return Key.create(repository); + public static SkyKey key(RepositoryName repository) { + if (repository.isMain()) { + return BLACKLIST_KEY; + } else { + return Key.create(repository); + } } public ImmutableSet getPatterns() { 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 0f82f3a673c060..24a181300a198d 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 @@ -20,6 +20,7 @@ import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.actions.InconsistentFilesystemException; import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.vfs.Dirent; @@ -56,9 +57,10 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws GlobFunctionException, InterruptedException { GlobDescriptor glob = (GlobDescriptor) skyKey.argument(); + RepositoryName repositoryName = glob.getPackageId().getRepository(); BlacklistedPackagePrefixesValue blacklistedPackagePrefixes = (BlacklistedPackagePrefixesValue) - env.getValue(BlacklistedPackagePrefixesValue.key(glob.getPackageId().getRepository())); + env.getValue(BlacklistedPackagePrefixesValue.key(repositoryName)); if (env.valuesMissing()) { return null; } @@ -80,7 +82,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) env.getValue( PackageLookupValue.key( PackageIdentifier.create( - glob.getPackageId().getRepository(), + repositoryName, glob.getPackageId().getPackageFragment().getRelative(globSubdir)))); if (globSubdirPkgLookupValue == null) { return null; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java index 55054d6d11b66e..34708f90258fe0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java @@ -92,8 +92,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) // Check .bazelignore file under main repository. BlacklistedPackagePrefixesValue blacklistedPatternsValue = - (BlacklistedPackagePrefixesValue) - env.getValue(BlacklistedPackagePrefixesValue.key(packageKey.getRepository())); + (BlacklistedPackagePrefixesValue) env.getValue(BlacklistedPackagePrefixesValue.key()); if (blacklistedPatternsValue == null) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java index f6f7e21b469809..95c69c55130b0f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java @@ -83,18 +83,13 @@ public SkyValue compute(SkyKey key, Environment env) TargetPattern parsedPattern = patternKey.getParsedPattern(); - ImmutableSet blacklistedPatterns; - if (parsedPattern.getRepository() == null) { - blacklistedPatterns = ImmutableSet.of(); - } else { - BlacklistedPackagePrefixesValue blacklist = - (BlacklistedPackagePrefixesValue) - env.getValue(BlacklistedPackagePrefixesValue.key(parsedPattern.getRepository())); - if (blacklist == null) { - return null; - } - blacklistedPatterns = blacklist.getPatterns(); + BlacklistedPackagePrefixesValue blacklist = + (BlacklistedPackagePrefixesValue) + env.getValue(BlacklistedPackagePrefixesValue.key(parsedPattern.getRepository())); + if (blacklist == null) { + return null; } + ImmutableSet blacklistedPatterns = blacklist.getPatterns(); // This SkyFunction is used to load the universe, so we want both the blacklisted directories // from the global blacklist and the excluded directories from the TargetPatternKey itself to be diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java index 245fc9d17c625b..fd31d2ec025329 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java @@ -48,18 +48,13 @@ public SkyValue compute(SkyKey key, Environment env) throws TargetPatternFunctio ((TargetPatternValue.TargetPatternKey) key.argument()); TargetPattern parsedPattern = patternKey.getParsedPattern(); - ImmutableSet blacklistedPatterns; - if (parsedPattern.getRepository() == null) { - blacklistedPatterns = ImmutableSet.of(); - } else { - BlacklistedPackagePrefixesValue blacklist = - (BlacklistedPackagePrefixesValue) - env.getValue(BlacklistedPackagePrefixesValue.key(parsedPattern.getRepository())); - if (blacklist == null) { - return null; - } - blacklistedPatterns = blacklist.getPatterns(); + BlacklistedPackagePrefixesValue blacklist = + (BlacklistedPackagePrefixesValue) + env.getValue(BlacklistedPackagePrefixesValue.key(parsedPattern.getRepository())); + if (blacklist == null) { + return null; } + ImmutableSet blacklistedPatterns = blacklist.getPatterns(); ResolvedTargets resolvedTargets; try { From ee4f255424ac6599aeaf37dd7e4523e092a7e6aa Mon Sep 17 00:00:00 2001 From: Yun Peng Date: Fri, 29 Nov 2019 11:29:38 +0100 Subject: [PATCH 3/3] Refactor --- .../devtools/build/lib/skyframe/SkyframeExecutor.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index ccd2b8967c4548..0f1614ac23226c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -505,9 +505,7 @@ private ImmutableMap skyFunctions(PackageFactory p SkyFunctions.COLLECT_PACKAGES_UNDER_DIRECTORY, newCollectPackagesUnderDirectoryFunction(directories)); map.put( - SkyFunctions.BLACKLISTED_PACKAGE_PREFIXES, - new BlacklistedPackagePrefixesFunction( - hardcodedBlacklistedPackagePrefixes, additionalBlacklistedPackagePrefixesFile)); + SkyFunctions.BLACKLISTED_PACKAGE_PREFIXES, newBlacklistedPackagePrefixesFunction()); map.put(SkyFunctions.TESTS_IN_SUITE, new TestExpansionFunction()); map.put(SkyFunctions.TEST_SUITE_EXPANSION, new TestsForTargetPatternFunction()); map.put(SkyFunctions.TARGET_PATTERN_PHASE, new TargetPatternPhaseFunction()); @@ -636,6 +634,11 @@ protected SkyFunction newGlobFunction() { return new GlobFunction(/*alwaysUseDirListing=*/ false); } + protected SkyFunction newBlacklistedPackagePrefixesFunction() { + return new BlacklistedPackagePrefixesFunction( + hardcodedBlacklistedPackagePrefixes, additionalBlacklistedPackagePrefixesFile); + } + protected SkyFunction newCollectPackagesUnderDirectoryFunction(BlazeDirectories directories) { return new CollectPackagesUnderDirectoryFunction(directories); }