From 3f1c2878b6c2da44a001a9e1533c53afbf465fb8 Mon Sep 17 00:00:00 2001 From: "Lukacs T. Berki" Date: Fri, 18 Oct 2024 05:55:58 +0000 Subject: [PATCH 1/7] Implement ignoring directories based on wildcards. This is accomplished by a new directive in REPO.bazel, "ignore_directories()". It takes a single argument, a list of directories to ignore and it allows the same wildcards as glob(). This is done separately from .bazelignore to provide a migration path off of that weird single-purpose configuration file. Implementing this requires splitting RepoFileFunction into two: a part that parses the repository file and one that creates a PackageArgs instance. This was necessary to avoid a Skyframe dependency cycle: when a WORKSPACE file is present and it loads a .bzl file from a repository with a REPO.bazel file, the repo mapping for the main repository depends on the WORKSPACE file, which depends on the .bzl file, which depends on the IgnoredPackagePrefixesValue of its repository, which then depends on the repo mapping of the main repository and the one the .bzl file is in, which then depend on the WORKSPACE file. Fixes #7093. RELNOTES[NEW]: REPO.bazel now allows another directive, "ignore_directories()". It takes a list of directories to ignore just like .bazelignore does, but with glob semantics. --- .../google/devtools/build/lib/cmdline/BUILD | 1 + .../lib/cmdline/IgnoredSubdirectories.java | 112 +++++++++++-- .../build/lib/cmdline/TargetPattern.java | 8 +- .../build/lib/packages/RepoCallable.java | 32 ++-- .../build/lib/packages/RepoThreadContext.java | 31 +++- .../google/devtools/build/lib/skyframe/BUILD | 24 +++ .../BazelSkyframeExecutorConstants.java | 11 +- .../IgnoredPackagePrefixesFunction.java | 147 ++++++++++++------ .../skyframe/IgnoredPackagePrefixesValue.java | 8 +- .../build/lib/skyframe/PackageFunction.java | 43 +++-- .../build/lib/skyframe/RepoFileFunction.java | 36 ++--- .../build/lib/skyframe/RepoFileValue.java | 15 +- .../build/lib/skyframe/SkyFunctions.java | 1 + .../build/lib/skyframe/SkyframeExecutor.java | 1 + .../packages/AbstractPackageLoader.java | 5 +- .../build/lib/skyframe/packages/BUILD | 1 + .../devtools/build/lib/vfs/UnixGlob.java | 41 ++++- .../bzlmod/ModuleExtensionResolutionTest.java | 4 +- .../bazel/bzlmod/ModuleFileFunctionTest.java | 5 +- .../query2/testutil/SkyframeQueryHelper.java | 2 +- .../repository/RepositoryDelegatorTest.java | 5 +- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../ContainingPackageLookupFunctionTest.java | 3 +- .../skyframe/FilesetEntryFunctionTest.java | 3 +- .../build/lib/skyframe/GlobTestBase.java | 2 +- .../skyframe/PackageLookupFunctionTest.java | 2 +- ...psOfPatternsFunctionSmartNegationTest.java | 3 +- ...ursiveFilesystemTraversalFunctionTest.java | 3 +- .../IgnoredPackagePrefixesValueCodecTest.java | 21 ++- src/test/shell/bazel/bazelignore_test.sh | 58 +++++++ 30 files changed, 473 insertions(+), 156 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/BUILD b/src/main/java/com/google/devtools/build/lib/cmdline/BUILD index 00d4764187ff85..d9b981f1a72e9c 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/BUILD +++ b/src/main/java/com/google/devtools/build/lib/cmdline/BUILD @@ -51,6 +51,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", "//src/main/java/com/google/devtools/build/lib/util:hash_codes", "//src/main/java/com/google/devtools/build/lib/util:string", + "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:ospathpolicy", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectories.java b/src/main/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectories.java index 356251845662b2..e3c6e5ce250237 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectories.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectories.java @@ -14,12 +14,24 @@ package com.google.devtools.build.lib.cmdline; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; +import com.google.common.base.Splitter; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext; +import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec; +import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; +import com.google.devtools.build.lib.skyframe.serialization.SerializationException; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.UnixGlob; +import com.google.protobuf.CodedInputStream; +import com.google.protobuf.CodedOutputStream; +import java.io.IOException; import java.util.Objects; import javax.annotation.Nullable; @@ -29,42 +41,98 @@ *

This is currently just a prefix, but will eventually support glob-style wildcards. */ public final class IgnoredSubdirectories { - public static final IgnoredSubdirectories EMPTY = new IgnoredSubdirectories(ImmutableSet.of()); + public static final IgnoredSubdirectories EMPTY = + new IgnoredSubdirectories(ImmutableSet.of(), ImmutableList.of()); + + private static final Splitter SLASH_SPLITTER = Splitter.on("/"); private final ImmutableSet prefixes; - private IgnoredSubdirectories(ImmutableSet prefixes) { - for (PathFragment prefix : prefixes) { - Preconditions.checkArgument(!prefix.isAbsolute()); + // String[] is mutable; we keep the split version because that's faster to match and the non-split + // one because that allows for simpler equality checking and then matchingEntry() doesn't need to + // allocate new objects. + private final ImmutableList patterns; + private final ImmutableList splitPatterns; + + private static class Codec implements ObjectCodec { + private static final Codec INSTANCE = new Codec(); + + @Override + public Class getEncodedClass() { + return IgnoredSubdirectories.class; + } + + @Override + public void serialize( + SerializationContext context, IgnoredSubdirectories obj, CodedOutputStream codedOut) + throws SerializationException, IOException { + context.serialize(obj.prefixes, codedOut); + context.serialize(obj.patterns, codedOut); } + + @Override + public IgnoredSubdirectories deserialize( + DeserializationContext context, CodedInputStream codedIn) + throws SerializationException, IOException { + ImmutableSet prefixes = context.deserialize(codedIn); + ImmutableList patterns = context.deserialize(codedIn); + + return new IgnoredSubdirectories(prefixes, patterns); + } + } + + private IgnoredSubdirectories( + ImmutableSet prefixes, ImmutableList patterns) { this.prefixes = prefixes; + this.patterns = patterns; + this.splitPatterns = + patterns.stream() + .map(p -> Iterables.toArray(SLASH_SPLITTER.split(p), String.class)) + .collect(toImmutableList()); } public static IgnoredSubdirectories of(ImmutableSet prefixes) { - if (prefixes.isEmpty()) { + return of(prefixes, ImmutableList.of()); + } + + public static IgnoredSubdirectories of( + ImmutableSet prefixes, ImmutableList patterns) { + if (prefixes.isEmpty() && patterns.isEmpty()) { return EMPTY; - } else { - return new IgnoredSubdirectories(prefixes); } + + for (PathFragment prefix : prefixes) { + Preconditions.checkArgument(!prefix.isAbsolute()); + } + + return new IgnoredSubdirectories(prefixes, patterns); } public IgnoredSubdirectories withPrefix(PathFragment prefix) { - ImmutableSet prefixed = + Preconditions.checkArgument(!prefix.isAbsolute()); + + ImmutableSet prefixedPrefixes = prefixes.stream().map(prefix::getRelative).collect(toImmutableSet()); - return new IgnoredSubdirectories(prefixed); + + ImmutableList prefixedPatterns = + patterns.stream().map(p -> prefix + "/" + p).collect(toImmutableList()); + + return new IgnoredSubdirectories(prefixedPrefixes, prefixedPatterns); } public IgnoredSubdirectories union(IgnoredSubdirectories other) { return new IgnoredSubdirectories( - ImmutableSet.builder().addAll(prefixes).addAll(other.prefixes).build()); + ImmutableSet.builder().addAll(prefixes).addAll(other.prefixes).build(), + ImmutableList.builder().addAll(patterns).addAll(other.patterns).build()); } /** Filters out entries that cannot match anything under {@code directory}. */ public IgnoredSubdirectories filterForDirectory(PathFragment directory) { ImmutableSet filteredPrefixes = prefixes.stream().filter(p -> p.startsWith(directory)).collect(toImmutableSet()); + ImmutableList.Builder filteredPatterns = ImmutableList.builder(); - return new IgnoredSubdirectories(filteredPrefixes); + return new IgnoredSubdirectories(filteredPrefixes, filteredPatterns.build()); } public ImmutableSet prefixes() { @@ -95,10 +163,17 @@ public boolean allPathsAreUnder(PathFragment directory) { /** Returns the entry that matches a given directory or {@code null} if none. */ @Nullable - public PathFragment matchingEntry(PathFragment directory) { + public String matchingEntry(PathFragment directory) { for (PathFragment prefix : prefixes) { if (directory.startsWith(prefix)) { - return prefix; + return prefix.getPathString(); + } + } + + String[] segmentArray = Iterables.toArray(directory.segments(), String.class); + for (int i = 0; i < patterns.size(); i++) { + if (UnixGlob.matchesPrefix(splitPatterns.get(i), segmentArray)) { + return patterns.get(i); } } @@ -111,17 +186,22 @@ public boolean equals(Object other) { return false; } + // splitPatterns is a function of patterns so it's enough to check if patterns is equal IgnoredSubdirectories that = (IgnoredSubdirectories) other; - return Objects.equals(this.prefixes, that.prefixes); + return Objects.equals(this.prefixes, that.prefixes) + && Objects.equals(this.patterns, that.patterns); } @Override public int hashCode() { - return prefixes.hashCode(); + return Objects.hash(prefixes, patterns); } @Override public String toString() { - return MoreObjects.toStringHelper("IgnoredSubdirectories").add("prefixes", prefixes).toString(); + return MoreObjects.toStringHelper("IgnoredSubdirectories") + .add("prefixes", prefixes) + .add("patterns", patterns) + .toString(); } } 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 e1ff3593701309..99687ba531e401 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 @@ -595,8 +595,7 @@ public void eval( this, excludedSubdirectories); IgnoredSubdirectories ignoredSubdirectories = ignoredSubdirectoriesSupplier.get(); - PathFragment matchingEntry = - ignoredSubdirectories.matchingEntry(directory.getPackageFragment()); + String matchingEntry = ignoredSubdirectories.matchingEntry(directory.getPackageFragment()); if (warnIfFiltered(matchingEntry, resolver)) { return; } @@ -632,8 +631,7 @@ ListenableFuture evalAsync( IgnoredSubdirectories filteredIgnoredSubdirectories; try { IgnoredSubdirectories ignoredSubdirectories = ignoredSubdirectoriesSupplier.get(); - PathFragment matchingEntry = - ignoredSubdirectories.matchingEntry(directory.getPackageFragment()); + String matchingEntry = ignoredSubdirectories.matchingEntry(directory.getPackageFragment()); if (warnIfFiltered(matchingEntry, resolver)) { return immediateVoidFuture(); } @@ -654,7 +652,7 @@ ListenableFuture evalAsync( executor); } - private boolean warnIfFiltered(PathFragment matchingEntry, TargetPatternResolver resolver) { + private boolean warnIfFiltered(String matchingEntry, TargetPatternResolver resolver) { if (matchingEntry != null) { resolver.warn( "Pattern '" diff --git a/src/main/java/com/google/devtools/build/lib/packages/RepoCallable.java b/src/main/java/com/google/devtools/build/lib/packages/RepoCallable.java index a78514be8f5fa5..7c0c7aa06ea9d8 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RepoCallable.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RepoCallable.java @@ -16,8 +16,10 @@ import java.util.Map; import net.starlark.java.annot.Param; +import net.starlark.java.annot.ParamType; import net.starlark.java.annot.StarlarkMethod; import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.Sequence; import net.starlark.java.eval.Starlark; import net.starlark.java.eval.StarlarkThread; @@ -27,6 +29,25 @@ private RepoCallable() {} public static final RepoCallable INSTANCE = new RepoCallable(); + @StarlarkMethod( + name = "ignore_directories", + useStarlarkThread = true, + documented = false, // TODO + parameters = { + @Param( + name = "dirs", + allowedTypes = { + @ParamType(type = Sequence.class, generic1 = String.class), + }) + }) + public Object ignoreDirectories(Iterable dirsUnchecked, StarlarkThread thread) + throws EvalException { + Sequence dirs = Sequence.cast(dirsUnchecked, String.class, "dirs"); + RepoThreadContext context = RepoThreadContext.fromOrFail(thread, "repo()"); + context.setIgnoredDirectories(dirs); + return Starlark.NONE; + } + @StarlarkMethod( name = "repo", documented = false, // documented separately @@ -44,16 +65,7 @@ public Object repoCallable(Map kwargs, StarlarkThread thread) throw Starlark.errorf("at least one argument must be given to the 'repo' function"); } - PackageArgs.Builder pkgArgsBuilder = PackageArgs.builder(); - for (Map.Entry kwarg : kwargs.entrySet()) { - PackageArgs.processParam( - kwarg.getKey(), - kwarg.getValue(), - "repo() argument '" + kwarg.getKey() + "'", - context.getLabelConverter(), - pkgArgsBuilder); - } - context.setPackageArgs(pkgArgsBuilder.build()); + context.setPackageArgsMap(kwargs); return Starlark.NONE; } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/RepoThreadContext.java b/src/main/java/com/google/devtools/build/lib/packages/RepoThreadContext.java index 88c0f4fc8445e5..afd092295ba0d2 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RepoThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RepoThreadContext.java @@ -14,8 +14,12 @@ package com.google.devtools.build.lib.packages; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.cmdline.StarlarkThreadContext; +import java.util.Collection; +import java.util.Map; import net.starlark.java.eval.EvalException; import net.starlark.java.eval.Starlark; import net.starlark.java.eval.StarlarkThread; @@ -23,9 +27,13 @@ /** Context object for a Starlark thread evaluating the REPO.bazel file. */ public class RepoThreadContext extends StarlarkThreadContext { private final LabelConverter labelConverter; - private PackageArgs packageArgs = PackageArgs.EMPTY; + + private ImmutableMap packageArgsMap = ImmutableMap.of(); private boolean repoFunctionCalled = false; + private ImmutableList ignoredDirectories = ImmutableList.of(); + private boolean ignoredDirectoriesSet = false; + public static RepoThreadContext fromOrFail(StarlarkThread thread, String what) throws EvalException { StarlarkThreadContext context = thread.getThreadLocal(StarlarkThreadContext.class); @@ -52,11 +60,24 @@ public void setRepoFunctionCalled() { repoFunctionCalled = true; } - public void setPackageArgs(PackageArgs packageArgs) { - this.packageArgs = packageArgs; + public void setPackageArgsMap(Map kwargs) { + this.packageArgsMap = ImmutableMap.copyOf(kwargs); + } + + public ImmutableMap getPackageArgsMap() { + return packageArgsMap; + } + + public void setIgnoredDirectories(Collection ignoredDirectories) throws EvalException { + if (ignoredDirectoriesSet) { + throw new EvalException("'ignored_directories()' can only be called once"); + } + + ignoredDirectoriesSet = true; + this.ignoredDirectories = ImmutableList.copyOf(ignoredDirectories); } - public PackageArgs getPackageArgs() { - return packageArgs; + public ImmutableList getIgnoredDirectories() { + return ignoredDirectories; } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 1ff8ba45c0fb65..253f494bd0d337 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -151,6 +151,7 @@ java_library( ":map_as_package_roots", ":metadata_consumer_for_metrics", ":node_dropping_inconsistency_receiver", + ":package_args_function", ":package_error_function", ":package_error_message_function", ":package_identifier_batching_callback", @@ -832,6 +833,8 @@ java_library( deps = [ ":ignored_package_prefixes_value", ":precomputed_value", + ":repo_file_function", + ":repo_file_value", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception", @@ -2330,6 +2333,25 @@ java_library( ], ) +java_library( + name = "package_args_function", + srcs = ["PackageArgsFunction.java"], + deps = [ + ":repo_file_function", + ":repo_file_value", + ":repository_mapping_value", + ":sky_functions", + "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//src/main/java/net/starlark/java/eval", + "//third_party:guava", + "//third_party:jsr305", + ], +) + java_library( name = "repo_file_function", srcs = ["RepoFileFunction.java"], @@ -2347,6 +2369,7 @@ java_library( "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//src/main/java/net/starlark/java/eval", "//src/main/java/net/starlark/java/syntax", + "//third_party:guava", "//third_party:jsr305", ], ) @@ -2360,6 +2383,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", "//third_party:auto_value", + "//third_party:guava", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java b/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java index 77ae054c29195f..bf6de69371c4fe 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java @@ -30,6 +30,8 @@ private BazelSkyframeExecutorConstants() {} public static final ImmutableSet HARDCODED_IGNORED_PACKAGE_PREFIXES = ImmutableSet.of(); + public static final PathFragment BAZELIGNORE_PATH = PathFragment.create(".bazelignore"); + /** * The file .bazelignore can be used to specify directories to be ignored by bazel * @@ -41,7 +43,14 @@ private BazelSkyframeExecutorConstants() {} * therefore fails the build, this ignore functionality currently has no chance to kick in. */ public static final SkyFunction IGNORED_PACKAGE_PREFIXES_FUNCTION = - new IgnoredPackagePrefixesFunction(PathFragment.create(".bazelignore")); + new IgnoredPackagePrefixesFunction(BAZELIGNORE_PATH, true); + + /** + * IGNORED_PACKAGE_PREFIXES_FUNCTION, except always returns the empty value. Used for tests where + * the extra complications incurred by evaluating the function are undesired. + */ + public static final SkyFunction NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION = + new IgnoredPackagePrefixesFunction(PathFragment.EMPTY_FRAGMENT, false); public static final CrossRepositoryLabelViolationStrategy CROSS_REPOSITORY_LABEL_VIOLATION_STRATEGY = CrossRepositoryLabelViolationStrategy.ERROR; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesFunction.java index c935316d34a8c8..ef6f1ce23cf0a0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesFunction.java @@ -15,6 +15,7 @@ import static com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction.VENDOR_DIRECTORY; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.io.CharStreams; import com.google.common.io.LineProcessor; @@ -23,6 +24,7 @@ import com.google.devtools.build.lib.io.InconsistentFilesystemException; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue; +import com.google.devtools.build.lib.skyframe.RepoFileFunction.BadRepoFileException; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.RootedPath; @@ -44,9 +46,12 @@ */ public class IgnoredPackagePrefixesFunction implements SkyFunction { private final PathFragment ignoredPackagePrefixesFile; + private final boolean useRepoFile; - public IgnoredPackagePrefixesFunction(PathFragment ignoredPackagePrefixesFile) { + public IgnoredPackagePrefixesFunction( + PathFragment ignoredPackagePrefixesFile, boolean useRepoFile) { this.ignoredPackagePrefixesFile = ignoredPackagePrefixesFile; + this.useRepoFile = useRepoFile; } public static void getIgnoredPackagePrefixes( @@ -71,64 +76,110 @@ public static void getIgnoredPackagePrefixes( } @Nullable - @Override - public SkyValue compute(SkyKey key, Environment env) + private ImmutableList computeIgnoredPatterns( + Environment env, RepositoryName repositoryName) throws IgnoredPatternsFunctionException, InterruptedException { - RepositoryName repositoryName = (RepositoryName) key.argument(); - ImmutableSet.Builder ignoredPackagePrefixesBuilder = ImmutableSet.builder(); - if (!ignoredPackagePrefixesFile.equals(PathFragment.EMPTY_FRAGMENT)) { - PathPackageLocator pkgLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env); + if (!useRepoFile) { + return ImmutableList.of(); + } + + try { + RepoFileValue repoFileValue = + (RepoFileValue) + env.getValueOrThrow( + RepoFileValue.key(repositoryName), IOException.class, BadRepoFileException.class); + if (env.valuesMissing()) { return null; } - if (repositoryName.isMain()) { - PathFragment vendorDir = null; - if (VENDOR_DIRECTORY.get(env).isPresent()) { - vendorDir = VENDOR_DIRECTORY.get(env).get().asFragment(); + return repoFileValue.ignoredDirectories(); + } catch (IOException e) { + throw new IgnoredPatternsFunctionException(e); + } catch (BadRepoFileException e) { + throw new IgnoredPatternsFunctionException(e); + } + } + + @Nullable + private ImmutableSet computeIgnoredPrefixes( + Environment env, RepositoryName repositoryName) + throws IgnoredPatternsFunctionException, InterruptedException { + if (ignoredPackagePrefixesFile.equals(PathFragment.EMPTY_FRAGMENT)) { + return ImmutableSet.of(); + } + + ImmutableSet.Builder ignoredPackagePrefixesBuilder = ImmutableSet.builder(); + PathPackageLocator pkgLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env); + if (env.valuesMissing()) { + return null; + } + + if (repositoryName.isMain()) { + PathFragment vendorDir = null; + if (VENDOR_DIRECTORY.get(env).isPresent()) { + vendorDir = VENDOR_DIRECTORY.get(env).get().asFragment(); + } + + for (Root packagePathEntry : pkgLocator.getPathEntries()) { + PathFragment workspaceRoot = packagePathEntry.asPath().asFragment(); + if (vendorDir != null && vendorDir.startsWith(workspaceRoot)) { + ignoredPackagePrefixesBuilder.add(vendorDir.relativeTo(workspaceRoot)); } - for (Root packagePathEntry : pkgLocator.getPathEntries()) { - PathFragment workspaceRoot = packagePathEntry.asPath().asFragment(); - if (vendorDir != null && vendorDir.startsWith(workspaceRoot)) { - ignoredPackagePrefixesBuilder.add(vendorDir.relativeTo(workspaceRoot)); - } - - RootedPath rootedPatternFile = - RootedPath.toRootedPath(packagePathEntry, ignoredPackagePrefixesFile); - FileValue patternFileValue = (FileValue) env.getValue(FileValue.key(rootedPatternFile)); - if (patternFileValue == null) { - return null; - } - if (patternFileValue.isFile()) { - getIgnoredPackagePrefixes(rootedPatternFile, ignoredPackagePrefixesBuilder); - break; - } + RootedPath rootedPatternFile = + RootedPath.toRootedPath(packagePathEntry, ignoredPackagePrefixesFile); + FileValue patternFileValue = (FileValue) env.getValue(FileValue.key(rootedPatternFile)); + if (patternFileValue == null) { + return null; + } + if (patternFileValue.isFile()) { + getIgnoredPackagePrefixes(rootedPatternFile, ignoredPackagePrefixesBuilder); + break; } - } else { - // Make sure the repository is fetched. - RepositoryDirectoryValue repositoryValue = - (RepositoryDirectoryValue) env.getValue(RepositoryDirectoryValue.key(repositoryName)); - if (repositoryValue == null) { + } + } else { + // Make sure the repository is fetched. + RepositoryDirectoryValue repositoryValue = + (RepositoryDirectoryValue) env.getValue(RepositoryDirectoryValue.key(repositoryName)); + if (repositoryValue == null) { + return null; + } + if (repositoryValue.repositoryExists()) { + RootedPath rootedPatternFile = + RootedPath.toRootedPath( + Root.fromPath(repositoryValue.getPath()), ignoredPackagePrefixesFile); + FileValue patternFileValue = (FileValue) env.getValue(FileValue.key(rootedPatternFile)); + if (patternFileValue == null) { return null; } - if (repositoryValue.repositoryExists()) { - RootedPath rootedPatternFile = - RootedPath.toRootedPath( - Root.fromPath(repositoryValue.getPath()), ignoredPackagePrefixesFile); - FileValue patternFileValue = (FileValue) env.getValue(FileValue.key(rootedPatternFile)); - if (patternFileValue == null) { - return null; - } - if (patternFileValue.isFile()) { - getIgnoredPackagePrefixes(rootedPatternFile, ignoredPackagePrefixesBuilder); - } + if (patternFileValue.isFile()) { + getIgnoredPackagePrefixes(rootedPatternFile, ignoredPackagePrefixesBuilder); } } } - return IgnoredPackagePrefixesValue.of(ignoredPackagePrefixesBuilder.build()); + return ignoredPackagePrefixesBuilder.build(); + } + + @Nullable + @Override + public SkyValue compute(SkyKey key, Environment env) + throws IgnoredPatternsFunctionException, InterruptedException { + RepositoryName repositoryName = (RepositoryName) key.argument(); + + ImmutableList ignoredPatterns = computeIgnoredPatterns(env, repositoryName); + if (env.valuesMissing()) { + return null; + } + + ImmutableSet ignoredPrefixes = computeIgnoredPrefixes(env, repositoryName); + if (env.valuesMissing()) { + return null; + } + + return IgnoredPackagePrefixesValue.of(ignoredPrefixes, ignoredPatterns); } private static final class PathFragmentLineProcessor @@ -172,5 +223,13 @@ public IgnoredPatternsFunctionException(InconsistentFilesystemException e) { public IgnoredPatternsFunctionException(InvalidIgnorePathException e) { super(e, Transience.PERSISTENT); } + + public IgnoredPatternsFunctionException(IOException e) { + super(e, Transience.TRANSIENT); + } + + public IgnoredPatternsFunctionException(BadRepoFileException e) { + super(e, Transience.PERSISTENT); + } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesValue.java index 237bfd0a3148a8..d286920754a718 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesValue.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.cmdline.IgnoredSubdirectories; import com.google.devtools.build.lib.cmdline.RepositoryName; @@ -37,10 +38,11 @@ private IgnoredPackagePrefixesValue(IgnoredSubdirectories ignoredSubdirectories) this.ignoredSubdirectories = ignoredSubdirectories; } - public static IgnoredPackagePrefixesValue of(ImmutableSet patterns) { - return patterns.isEmpty() + public static IgnoredPackagePrefixesValue of( + ImmutableSet prefixes, ImmutableList patterns) { + return prefixes.isEmpty() && patterns.isEmpty() ? EMPTY - : new IgnoredPackagePrefixesValue(IgnoredSubdirectories.of(patterns)); + : new IgnoredPackagePrefixesValue(IgnoredSubdirectories.of(prefixes, patterns)); } public static IgnoredPackagePrefixesValue of(IgnoredSubdirectories ignoredSubdirectories) { 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 2e6fc752d50684..5b0316a8838d08 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 @@ -57,6 +57,7 @@ import com.google.devtools.build.lib.server.FailureDetails.Filesystem; import com.google.devtools.build.lib.server.FailureDetails.PackageLoading; import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code; +import com.google.devtools.build.lib.skyframe.PackageArgsFunction.PackageArgsValue; import com.google.devtools.build.lib.skyframe.PackageFunctionWithMultipleGlobDeps.SkyframeGlobbingIOException; import com.google.devtools.build.lib.skyframe.RepoFileFunction.BadRepoFileException; import com.google.devtools.build.lib.skyframe.StarlarkBuiltinsFunction.BuiltinsFailedException; @@ -429,8 +430,11 @@ public SkyValue compute(SkyKey key, Environment env) (PackageLookupValue) env.getValueOrThrow( packageLookupKey, + BadRepoFileException.class, BuildFileNotFoundException.class, InconsistentFilesystemException.class); + } catch (BadRepoFileException e) { + throw badRepoFileException(e, packageId); } catch (BuildFileNotFoundException e) { throw new PackageFunctionException(e, Transience.PERSISTENT); } catch (InconsistentFilesystemException e) { @@ -941,6 +945,18 @@ private static boolean maybeAddEventAboutLabelCrossingSubpackage( } } + private static PackageFunctionException badRepoFileException( + Exception cause, PackageIdentifier packageId) { + return PackageFunctionException.builder() + .setType(PackageFunctionException.Type.BUILD_FILE_CONTAINS_ERRORS) + .setPackageIdentifier(packageId) + .setTransience(Transience.PERSISTENT) + .setException(cause) + .setMessage("bad REPO.bazel file") + .setPackageLoadingCode(PackageLoading.Code.BAD_REPO_FILE) + .build(); + } + @ForOverride protected abstract Globber makeGlobber( NonSkyframeGlobber nonSkyframeGlobber, @@ -979,31 +995,26 @@ private LoadedPackage loadPackage( IgnoredPackagePrefixesValue repositoryIgnoredPackagePrefixes = (IgnoredPackagePrefixesValue) env.getValue(IgnoredPackagePrefixesValue.key(packageId.getRepository())); - RepoFileValue repoFileValue; + PackageArgsValue packageArgsValue; if (shouldUseRepoDotBazel) { try { - repoFileValue = - (RepoFileValue) + packageArgsValue = + (PackageArgsValue) env.getValueOrThrow( - RepoFileValue.key(packageId.getRepository()), + PackageArgsFunction.key(packageId.getRepository()), IOException.class, BadRepoFileException.class); } catch (IOException | BadRepoFileException e) { - throw PackageFunctionException.builder() - .setType(PackageFunctionException.Type.BUILD_FILE_CONTAINS_ERRORS) - .setPackageIdentifier(packageId) - .setTransience(Transience.PERSISTENT) - .setException(e) - .setMessage("bad REPO.bazel file") - .setPackageLoadingCode(PackageLoading.Code.BAD_REPO_FILE) - .build(); + throw badRepoFileException(e, packageId); } } else { - repoFileValue = RepoFileValue.EMPTY; + packageArgsValue = PackageArgsValue.EMPTY; } + if (env.valuesMissing()) { return null; } + String workspaceName = workspaceNameValue.getName(); RepositoryMapping repositoryMapping = repositoryMappingValue.getRepositoryMapping(); RepositoryMapping mainRepositoryMapping = mainRepositoryMappingValue.getRepositoryMapping(); @@ -1163,7 +1174,7 @@ private LoadedPackage loadPackage( pkgBuilder.mergePackageArgsFrom( PackageArgs.builder().setDefaultVisibility(defaultVisibility)); - pkgBuilder.mergePackageArgsFrom(repoFileValue.packageArgs()); + pkgBuilder.mergePackageArgsFrom(packageArgsValue.getPackageArgs()); if (compiled.ok()) { packageFactory.executeBuildFile( @@ -1519,6 +1530,10 @@ public PackageFunctionException(NoSuchPackageException e, Transience transience) super(e, transience); } + public PackageFunctionException(BadRepoFileException e, Transience transience) { + super(e, transience); + } + static Builder builder() { return new Builder(); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java index 8cfa75e5ddbc77..f77463e2c3e090 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java @@ -14,6 +14,8 @@ package com.google.devtools.build.lib.skyframe; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.cmdline.PackageIdentifier; @@ -87,29 +89,23 @@ public SkyValue compute(SkyKey skyKey, Environment env) } if (!repoFileValue.exists()) { // It's okay to not have a REPO.bazel file. - return RepoFileValue.of(PackageArgs.EMPTY); + return RepoFileValue.of(ImmutableMap.of(), ImmutableList.of()); } // Now we can actually evaluate the file. StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); - RepositoryMappingValue repoMapping = - (RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(repoName)); - RepositoryMappingValue mainRepoMapping = - (RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(RepositoryName.MAIN)); if (env.valuesMissing()) { return null; } - StarlarkFile repoFile = readAndParseRepoFile(repoFilePath.asPath(), env); - PackageArgs packageArgs = - evalRepoFile( - repoFile, - repoName, - repoMapping.getRepositoryMapping(), - mainRepoMapping.getRepositoryMapping(), - starlarkSemantics, - env.getListener()); - return RepoFileValue.of(packageArgs); + StarlarkFile repoFile = readAndParseRepoFile(repoFilePath.asPath(), env); + return evalRepoFile( + repoFile, + repoName, + RepositoryMapping.ALWAYS_FALLBACK, + null, + starlarkSemantics, + env.getListener()); } private static StarlarkFile readAndParseRepoFile(Path path, Environment env) @@ -131,7 +127,7 @@ private static StarlarkFile readAndParseRepoFile(Path path, Environment env) return starlarkFile; } - private static String getDisplayNameForRepo( + public static String getDisplayNameForRepo( RepositoryName repoName, RepositoryMapping mainRepoMapping) { String displayName = repoName.getDisplayForm(mainRepoMapping); if (displayName.isEmpty()) { @@ -140,7 +136,7 @@ private static String getDisplayNameForRepo( return displayName; } - private PackageArgs evalRepoFile( + private RepoFileValue evalRepoFile( StarlarkFile starlarkFile, RepositoryName repoName, RepositoryMapping repoMapping, @@ -168,7 +164,7 @@ private PackageArgs evalRepoFile( mainRepoMapping); context.storeInThread(thread); Starlark.execFileProgram(program, predeclared, thread); - return context.getPackageArgs(); + return RepoFileValue.of(context.getPackageArgsMap(), context.getIgnoredDirectories()); } catch (SyntaxError.Exception e) { Event.replayEventsOn(handler, e.errors()); throw new RepoFileFunctionException( @@ -192,11 +188,11 @@ public BadRepoFileException(String message, Exception cause) { } static class RepoFileFunctionException extends SkyFunctionException { - private RepoFileFunctionException(IOException e, Transience transience) { + public RepoFileFunctionException(IOException e, Transience transience) { super(e, transience); } - private RepoFileFunctionException(BadRepoFileException e) { + public RepoFileFunctionException(BadRepoFileException e) { super(e, Transience.PERSISTENT); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileValue.java index 34763fee4f2c50..20ca0aa373b636 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileValue.java @@ -15,6 +15,8 @@ package com.google.devtools.build.lib.skyframe; import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.PackageArgs; import com.google.devtools.build.skyframe.AbstractSkyKey; @@ -24,12 +26,17 @@ /** Contains information about the REPO.bazel file at the root of a repo. */ @AutoValue public abstract class RepoFileValue implements SkyValue { - public static final RepoFileValue EMPTY = of(PackageArgs.EMPTY); + public static final RepoFileValue EMPTY = + of(ImmutableMap.of(), ImmutableList.of()); - public abstract PackageArgs packageArgs(); + public abstract ImmutableMap packageArgsMap(); - public static RepoFileValue of(PackageArgs packageArgs) { - return new AutoValue_RepoFileValue(packageArgs); + public abstract ImmutableList ignoredDirectories(); + + public static RepoFileValue of( + ImmutableMap packageArgsMap, + ImmutableList ignoredDirectories) { + return new AutoValue_RepoFileValue(packageArgsMap, ignoredDirectories); } public static Key key(RepositoryName repoName) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java index beb0883c74f35a..338c2740fc3113 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java @@ -147,6 +147,7 @@ public final class SkyFunctions { SkyFunctionName.createHermetic("RESOLVED_FILE"); public static final SkyFunctionName MODULE_FILE = SkyFunctionName.createNonHermetic("MODULE_FILE"); + public static final SkyFunctionName PACKAGE_ARGS = SkyFunctionName.createHermetic("PACKAGE_ARGS"); public static final SkyFunctionName REPO_FILE = SkyFunctionName.createHermetic("REPO_FILE"); public static final SkyFunctionName BUILD_DRIVER = SkyFunctionName.createNonHermetic("BUILD_DRIVER"); 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 e8ceaa20bf043c..6a4a9b6a072ab7 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 @@ -765,6 +765,7 @@ private ImmutableMap skyFunctions() { : (k, env) -> { throw new IllegalStateException("supposed to be unused"); }); + map.put(SkyFunctions.PACKAGE_ARGS, PackageArgsFunction.INSTANCE); map.put(SkyFunctions.EXTERNAL_PACKAGE, new ExternalPackageFunction(externalPackageHelper)); // Inject an empty default BAZEL_DEP_GRAPH SkyFunction for unit tests. map.put( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java index d1664ac461b50f..6c91ebd303c3d6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java @@ -69,6 +69,7 @@ import com.google.devtools.build.lib.skyframe.FileFunction; import com.google.devtools.build.lib.skyframe.FileStateFunction; import com.google.devtools.build.lib.skyframe.IgnoredPackagePrefixesFunction; +import com.google.devtools.build.lib.skyframe.PackageArgsFunction; import com.google.devtools.build.lib.skyframe.PackageFunction; import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile; import com.google.devtools.build.lib.skyframe.PackageFunction.GlobbingStrategy; @@ -561,8 +562,7 @@ public String getBaseNameForLoadedPackage(PackageIdentifier packageName) { getExternalPackageHelper())) .put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - new IgnoredPackagePrefixesFunction( - /* ignoredPackagePrefixesFile= */ PathFragment.EMPTY_FRAGMENT)) + new IgnoredPackagePrefixesFunction(PathFragment.EMPTY_FRAGMENT, false)) .put(SkyFunctions.CONTAINING_PACKAGE_LOOKUP, new ContainingPackageLookupFunction()) .put( SkyFunctions.BZL_COMPILE, @@ -583,6 +583,7 @@ public String getBaseNameForLoadedPackage(PackageIdentifier packageName) { SkyFunctions.REPO_FILE, new RepoFileFunction( ruleClassProvider.getBazelStarlarkEnvironment(), directories.getWorkspace())) + .put(SkyFunctions.PACKAGE_ARGS, PackageArgsFunction.INSTANCE) .put(SkyFunctions.EXTERNAL_PACKAGE, new ExternalPackageFunction(getExternalPackageHelper())) .put( BzlmodRepoRuleValue.BZLMOD_REPO_RULE, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD index bc67b392f0b7e8..18795f41c6740c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD @@ -58,6 +58,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:default_syscall_cache", "//src/main/java/com/google/devtools/build/lib/skyframe:file_function", "//src/main/java/com/google/devtools/build/lib/skyframe:ignored_package_prefixes_function", + "//src/main/java/com/google/devtools/build/lib/skyframe:package_args_function", "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function", "//src/main/java/com/google/devtools/build/lib/skyframe:package_value", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_function", diff --git a/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java b/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java index d822a0f007ba57..e9259911e36a84 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java @@ -901,7 +901,7 @@ public static void removeExcludes(Set paths, Collection excludes path -> { String[] segments = Iterables.toArray(Splitter.on('/').split(path), String.class); for (String[] splitPattern : splitPatterns) { - if (matchesPattern(splitPattern, segments, 0, 0, patternCache)) { + if (matchesPattern(splitPattern, segments, 0, 0, patternCache, MatchMode.EXACT)) { return true; } } @@ -909,21 +909,48 @@ public static void removeExcludes(Set paths, Collection excludes }); } + /** Returns whether any path under {@code path} can match {@code pattern}. */ + public static boolean canMatchChild(String[] pattern, String[] path) { + return matchesPattern(pattern, path, 0, 0, null, MatchMode.CAN_MATCH_CHILD); + } + + /** Returns whether {@code pattern} matches a prefix of {@code path}. */ + public static boolean matchesPrefix(String[] pattern, String[] path) { + return matchesPattern(pattern, path, 0, 0, null, MatchMode.PREFIX); + } + + /** Returns whether {@code pattern} matches {@code path}. */ + public static boolean matches(String[] pattern, String[] path) { + return matchesPattern(pattern, path, 0, 0, null, MatchMode.EXACT); + } + + /** How {@code #matchesPattern()} should work */ + private enum MatchMode { + EXACT, // The path should exactly match the pattern + PREFIX, // The pattern should match a prefix of the path + CAN_MATCH_CHILD, // Whether there can be any path under the prefix that matches the pattern + } + /** Returns true if {@code pattern} matches {@code path} starting from the given segments. */ private static boolean matchesPattern( - String[] pattern, String[] path, int i, int j, Map patternCache) { + String[] pattern, + String[] path, + int i, + int j, + Map patternCache, + MatchMode matchMode) { if (i == pattern.length) { - return j == path.length; + return matchMode == MatchMode.PREFIX ? true : j == path.length; } if (pattern[i].equals("**")) { - return matchesPattern(pattern, path, i + 1, j, patternCache) - || (j < path.length && matchesPattern(pattern, path, i, j + 1, patternCache)); + return matchesPattern(pattern, path, i + 1, j, patternCache, matchMode) + || (j < path.length && matchesPattern(pattern, path, i, j + 1, patternCache, matchMode)); } if (j == path.length) { - return false; + return matchMode == MatchMode.CAN_MATCH_CHILD ? true : false; } if (matches(pattern[i], path[j], patternCache)) { - return matchesPattern(pattern, path, i + 1, j + 1, patternCache); + return matchesPattern(pattern, path, i + 1, j + 1, patternCache, matchMode); } return false; } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index bd51a28f58a19d..43c0d1edeca032 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -61,7 +61,6 @@ import com.google.devtools.build.lib.skyframe.ExternalPackageFunction; import com.google.devtools.build.lib.skyframe.FileFunction; import com.google.devtools.build.lib.skyframe.FileStateFunction; -import com.google.devtools.build.lib.skyframe.IgnoredPackagePrefixesFunction; import com.google.devtools.build.lib.skyframe.LocalRepositoryLookupFunction; import com.google.devtools.build.lib.skyframe.PackageFunction; import com.google.devtools.build.lib.skyframe.PackageLookupFunction; @@ -210,8 +209,7 @@ public void setup() throws Exception { BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER)) .put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - new IgnoredPackagePrefixesFunction( - /* ignoredPackagePrefixesFile= */ PathFragment.EMPTY_FRAGMENT)) + BazelSkyframeExecutorConstants.NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION) .put( SkyFunctions.REPOSITORY_MAPPING, new RepositoryMappingFunction(ruleClassProvider)) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java index d1650d50e61faf..ec7e94735d4d07 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java @@ -51,7 +51,6 @@ import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction; import com.google.devtools.build.lib.skyframe.FileFunction; import com.google.devtools.build.lib.skyframe.FileStateFunction; -import com.google.devtools.build.lib.skyframe.IgnoredPackagePrefixesFunction; import com.google.devtools.build.lib.skyframe.LocalRepositoryLookupFunction; import com.google.devtools.build.lib.skyframe.PackageLookupFunction; import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy; @@ -65,7 +64,6 @@ import com.google.devtools.build.lib.vfs.FileStateKey; 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.Root; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.lib.vfs.SyscallCache; @@ -164,8 +162,7 @@ private void setUpWithBuiltinModules(ImmutableMap b BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER)) .put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - new IgnoredPackagePrefixesFunction( - /* ignoredPackagePrefixesFile= */ PathFragment.EMPTY_FRAGMENT)) + BazelSkyframeExecutorConstants.NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION) .put( SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction( diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java index 19ff63f1f44efc..fd7722ac9d0117 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java +++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java @@ -425,7 +425,7 @@ protected SkyframeExecutor createSkyframeExecutor(ConfiguredRuleClassProvider ru .setDirectories(directories) .setActionKeyContext(actionKeyContext) .setIgnoredPackagePrefixesFunction( - new IgnoredPackagePrefixesFunction(ignoredPackagePrefixesFile)) + new IgnoredPackagePrefixesFunction(ignoredPackagePrefixesFile, true)) .setExtraSkyFunctions(analysisMock.getSkyFunctions(directories)) .setSyscallCache(delegatingSyscallCache) .build(); diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java index cf2a064ac2977a..f26d8e6e4ea904 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java @@ -69,7 +69,6 @@ import com.google.devtools.build.lib.skyframe.ExternalPackageFunction; import com.google.devtools.build.lib.skyframe.FileFunction; import com.google.devtools.build.lib.skyframe.FileStateFunction; -import com.google.devtools.build.lib.skyframe.IgnoredPackagePrefixesFunction; import com.google.devtools.build.lib.skyframe.LocalRepositoryLookupFunction; import com.google.devtools.build.lib.skyframe.PackageFunction; import com.google.devtools.build.lib.skyframe.PackageLookupFunction; @@ -88,7 +87,6 @@ import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.FileStateKey; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.build.skyframe.EvaluationContext; @@ -232,8 +230,7 @@ public void setupDelegator() throws Exception { .put(SkyFunctions.CONTAINING_PACKAGE_LOOKUP, new ContainingPackageLookupFunction()) .put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - new IgnoredPackagePrefixesFunction( - /* ignoredPackagePrefixesFile= */ PathFragment.EMPTY_FRAGMENT)) + BazelSkyframeExecutorConstants.NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION) .put( SkyFunctions.REPOSITORY_MAPPING, new RepositoryMappingFunction(ruleClassProvider)) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD index 8229001a838f7e..1fe436742a4c24 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD @@ -556,6 +556,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:directory_listing_state_value", "//src/main/java/com/google/devtools/build/lib/skyframe:file_function", "//src/main/java/com/google/devtools/build/lib/skyframe:glob_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:ignored_package_prefixes_function", "//src/main/java/com/google/devtools/build/lib/skyframe:invalid_glob_pattern_exception", "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java index bd967b0138fe3b..a60c480e039040 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java @@ -112,8 +112,7 @@ public final void setUp() throws Exception { skyFunctions.put(SkyFunctions.PACKAGE, PackageFunction.newBuilder().build()); skyFunctions.put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - new IgnoredPackagePrefixesFunction( - /*ignoredPackagePrefixesFile=*/ PathFragment.EMPTY_FRAGMENT)); + BazelSkyframeExecutorConstants.NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION); skyFunctions.put( FileStateKey.FILE_STATE, new FileStateFunction( diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java index af6531c7e56700..f55565660bb37d 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java @@ -121,8 +121,7 @@ public void setUp() throws Exception { BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER)); skyFunctions.put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - new IgnoredPackagePrefixesFunction( - /*ignoredPackagePrefixesFile=*/ PathFragment.EMPTY_FRAGMENT)); + BazelSkyframeExecutorConstants.NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION); skyFunctions.put( SkyFunctions.FILESET_ENTRY, new FilesetEntryFunction((unused) -> rootDirectory)); skyFunctions.put(SkyFunctions.WORKSPACE_NAME, new TestWorkspaceNameFunction()); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/GlobTestBase.java b/src/test/java/com/google/devtools/build/lib/skyframe/GlobTestBase.java index 6ff1459e66dd01..1aedeead90957e 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/GlobTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/GlobTestBase.java @@ -160,7 +160,7 @@ private Map createFunctionMap() { BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER)); skyFunctions.put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - BazelSkyframeExecutorConstants.IGNORED_PACKAGE_PREFIXES_FUNCTION); + new IgnoredPackagePrefixesFunction(BazelSkyframeExecutorConstants.BAZELIGNORE_PATH, false)); skyFunctions.put( FileStateKey.FILE_STATE, new FileStateFunction( diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java index 84400bb12c2efd..d984f744148922 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java @@ -135,7 +135,7 @@ public final void setUp() throws Exception { skyFunctions.put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, new IgnoredPackagePrefixesFunction( - PathFragment.create(IGNORED_PACKAGE_PREFIXES_FILE_PATH_STRING))); + PathFragment.create(IGNORED_PACKAGE_PREFIXES_FILE_PATH_STRING), false)); RuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider(); skyFunctions.put( WorkspaceFileValue.WORKSPACE_FILE, diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionSmartNegationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionSmartNegationTest.java index 913bd8f8318fbd..3b92433df54888 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionSmartNegationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionSmartNegationTest.java @@ -97,7 +97,8 @@ public void setUp() throws Exception { .setSyscallCache(SyscallCache.NO_CACHE) .setIgnoredPackagePrefixesFunction( new IgnoredPackagePrefixesFunction( - PathFragment.create(ADDITIONAL_IGNORED_PACKAGE_PREFIXES_FILE_PATH_STRING))) + PathFragment.create(ADDITIONAL_IGNORED_PACKAGE_PREFIXES_FILE_PATH_STRING), + false)) .build(); SkyframeExecutorTestHelper.process(skyframeExecutor); OptionsParser optionsParser = diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java index 1f8a1dcc5ecd4c..9f7e131e2d47bd 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java @@ -179,8 +179,7 @@ public void setUp() { BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER)); skyFunctions.put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - new IgnoredPackagePrefixesFunction( - /*ignoredPackagePrefixesFile=*/ PathFragment.EMPTY_FRAGMENT)); + BazelSkyframeExecutorConstants.NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION); skyFunctions.put(SkyFunctions.PACKAGE, PackageFunction.newBuilder().build()); skyFunctions.put( WorkspaceFileValue.WORKSPACE_FILE, diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/IgnoredPackagePrefixesValueCodecTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/IgnoredPackagePrefixesValueCodecTest.java index dc04ba9babd67c..66decb938da0a9 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/IgnoredPackagePrefixesValueCodecTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/IgnoredPackagePrefixesValueCodecTest.java @@ -13,10 +13,12 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe.serialization; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.skyframe.IgnoredPackagePrefixesValue; import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester; import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.Arrays; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -25,13 +27,24 @@ @RunWith(JUnit4.class) public class IgnoredPackagePrefixesValueCodecTest { + private static ImmutableSet prefixes(String... prefixes) { + return Arrays.stream(prefixes).map(PathFragment::create).collect(ImmutableSet.toImmutableSet()); + } + + private static ImmutableList patterns(String... patterns) { + return ImmutableList.copyOf(patterns); + } + @Test public void testCodec() throws Exception { new SerializationTester( - IgnoredPackagePrefixesValue.of(ImmutableSet.of()), - IgnoredPackagePrefixesValue.of(ImmutableSet.of(PathFragment.create("foo"))), - IgnoredPackagePrefixesValue.of( - ImmutableSet.of(PathFragment.create("foo"), PathFragment.create("bar/moo")))) + IgnoredPackagePrefixesValue.of(prefixes(), patterns()), + IgnoredPackagePrefixesValue.of(prefixes("foo"), patterns()), + IgnoredPackagePrefixesValue.of(prefixes("foo", "bar/moo"), patterns()), + IgnoredPackagePrefixesValue.of(prefixes(), patterns("foo")), + IgnoredPackagePrefixesValue.of(prefixes(), patterns("foo")), + IgnoredPackagePrefixesValue.of(prefixes(), patterns("foo/**")), + IgnoredPackagePrefixesValue.of(prefixes("foo"), patterns("foo/**"))) .runTests(); } } diff --git a/src/test/shell/bazel/bazelignore_test.sh b/src/test/shell/bazel/bazelignore_test.sh index dfd3a831a5d50f..74fa3d9a9dcbfe 100755 --- a/src/test/shell/bazel/bazelignore_test.sh +++ b/src/test/shell/bazel/bazelignore_test.sh @@ -217,4 +217,62 @@ test_invalid_path() { expect_log "java.nio.file.InvalidPathException: Nul character not allowed" } +test_target_patterns_with_wildcards_in_repo_bazel() { + rm -rf work && mkdir work && cd work + setup_module_dot_bazel + cat >REPO.bazel <<'EOF' +ignore_directories(["**/sub", "foo/bar/.dot/*"]) +EOF + + for pkg in foo foo/sub foo/sub/subsub foo/bar/.dot/pkg foo/notsub foo/bar/.dot; do + mkdir -p "$pkg" + echo 'filegroup(name="fg")' > "$pkg/BUILD.bazel" + done + + bazel query //foo/... > "$TEST_TMPDIR/targets" + assert_not_contains "//foo/sub:fg" "$TEST_TMPDIR/targets" + assert_not_contains "//foo/sub/subsub:fg" "$TEST_TMPDIR/targets" + assert_not_contains "//foo/bar/.dot/pkg:fg" "$TEST_TMPDIR/targets" + assert_contains "//foo/notsub:fg" "$TEST_TMPDIR/targets" +} + +test_globs_with_wildcards_in_repo_bazel() { + rm -rf work && mkdir work && cd work + setup_module_dot_bazel + cat >REPO.bazel <<'EOF' +ignore_directories(["**/sub", "foo/.hidden*"]) +EOF + + mkdir -p foo foo/bar sub bar/sub foo/.hidden_excluded foo/.included + touch foo/foofile foo/bar/barfile sub/subfile bar/sub/subfile + touch foo/.hidden_excluded/file foo/.included/file + + cat > BUILD <<'EOF' +filegroup(name="fg", srcs=glob(["**"])) + +EOF + bazel query //:all-targets > "$TEST_TMPDIR/targets" + assert_contains ":foo/foofile" "$TEST_TMPDIR/targets" + assert_contains ":foo/bar/barfile" "$TEST_TMPDIR/targets" + assert_contains ":foo/.included/file" "$TEST_TMPDIR/targets" + assert_not_contains ":sub/subfile" "$TEST_TMPDIR/targets" + assert_not_contains ":bar/sub/subfile" "$TEST_TMPDIR/targets" + assert_not_contains ":foo/.hidden_excluded/file" "$TEST_TMPDIR/targets" +} + + +test_syntax_error_in_repo_bazel() { + rm -rf work && mkdir work && cd work + setup_module_dot_bazel + cat >REPO.bazel <<'EOF' +SYNTAX ERROR +EOF + + touch BUILD.bazel + bazel query //:all && fail "failure expected" + if [[ $? != 7 ]]; then + fail "expected an analysis failure" + fi +} + run_suite "Integration tests for .bazelignore" From e93e711fb0b27709ae5b9822981a7fecd05cfb76 Mon Sep 17 00:00:00 2001 From: "Lukacs T. Berki" Date: Fri, 18 Oct 2024 06:29:12 +0000 Subject: [PATCH 2/7] add missing file --- .../lib/skyframe/PackageArgsFunction.java | 124 ++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 src/main/java/com/google/devtools/build/lib/skyframe/PackageArgsFunction.java diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageArgsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageArgsFunction.java new file mode 100644 index 00000000000000..28f359478e2d2d --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageArgsFunction.java @@ -0,0 +1,124 @@ +// Copyright 2024 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.skyframe; + +import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.packages.LabelConverter; +import com.google.devtools.build.lib.packages.PackageArgs; +import com.google.devtools.build.lib.skyframe.RepoFileFunction.BadRepoFileException; +import com.google.devtools.build.lib.skyframe.RepoFileFunction.RepoFileFunctionException; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.skyframe.AbstractSkyKey; +import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunctionException; +import com.google.devtools.build.skyframe.SkyFunctionName; +import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.SkyValue; +import java.util.Map; +import javax.annotation.Nullable; +import net.starlark.java.eval.EvalException; + +/** A @{link SkyFunction} that returns the {@link PackageArgs} for a given repository. */ +public class PackageArgsFunction implements SkyFunction { + public static final PackageArgsFunction INSTANCE = new PackageArgsFunction(); + + /** {@link SkyValue} wrapping a PackageArgs. */ + public static final class PackageArgsValue implements SkyValue { + public static final PackageArgsValue EMPTY = new PackageArgsValue(PackageArgs.EMPTY); + + private final PackageArgs packageArgs; + + public PackageArgsValue(PackageArgs packageArgs) { + this.packageArgs = packageArgs; + } + + public PackageArgs getPackageArgs() { + return packageArgs; + } + + @Override + public int hashCode() { + return packageArgs.hashCode(); + } + + @Override + public boolean equals(Object other) { + if (other instanceof PackageArgsValue that) { + return that.packageArgs.equals(packageArgs); + } else { + return false; + } + } + } + + public static Key key(RepositoryName repoName) { + return new Key(repoName); + } + + /** Key type for {@link RepoFileValue}. */ + public static class Key extends AbstractSkyKey { + + private Key(RepositoryName repoName) { + super(repoName); + } + + @Override + public SkyFunctionName functionName() { + return SkyFunctions.PACKAGE_ARGS; + } + } + + private PackageArgsFunction() {} + + @Nullable + @Override + public SkyValue compute(SkyKey skyKey, Environment env) + throws SkyFunctionException, InterruptedException { + RepositoryName repositoryName = (RepositoryName) skyKey.argument(); + + RepoFileValue repoFileValue = (RepoFileValue) env.getValue(RepoFileValue.key(repositoryName)); + RepositoryMappingValue repositoryMappingValue = + (RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(repositoryName)); + + if (env.valuesMissing()) { + return null; + } + String repoDisplayName = RepoFileFunction.getDisplayNameForRepo(repositoryName, null); + + PackageArgs.Builder pkgArgsBuilder = PackageArgs.builder(); + LabelConverter labelConverter = + new LabelConverter( + PackageIdentifier.create(repositoryName, PathFragment.EMPTY_FRAGMENT), + repositoryMappingValue.getRepositoryMapping()); + try { + for (Map.Entry kwarg : repoFileValue.packageArgsMap().entrySet()) { + PackageArgs.processParam( + kwarg.getKey(), + kwarg.getValue(), + "repo() argument '" + kwarg.getKey() + "'", + labelConverter, + pkgArgsBuilder); + } + } catch (EvalException e) { + env.getListener().handle(Event.error(e.getMessageWithStack())); + throw new RepoFileFunctionException( + new BadRepoFileException("error evaluating REPO.bazel file for " + repoDisplayName, e)); + } + + return new PackageArgsValue(pkgArgsBuilder.build()); + } +} From 9c09e329784797ca5926c535cdd3def5d72f9fa6 Mon Sep 17 00:00:00 2001 From: "Lukacs T. Berki" Date: Mon, 28 Oct 2024 07:46:59 +0000 Subject: [PATCH 3/7] address review comments --- .../starlark/StarlarkGlobalsImpl.java | 4 ++-- .../lib/cmdline/IgnoredSubdirectories.java | 2 -- ...RepoCallable.java => RepoFileGlobals.java} | 22 ++++++++++++------- .../build/lib/packages/RepoThreadContext.java | 13 +++++------ .../devtools/build/lib/vfs/UnixGlob.java | 2 +- src/test/shell/bazel/bazelignore_test.sh | 13 +++++++++++ 6 files changed, 35 insertions(+), 21 deletions(-) rename src/main/java/com/google/devtools/build/lib/packages/{RepoCallable.java => RepoFileGlobals.java} (78%) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkGlobalsImpl.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkGlobalsImpl.java index bae72e298463c5..5005259efa5446 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkGlobalsImpl.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkGlobalsImpl.java @@ -23,7 +23,7 @@ import com.google.devtools.build.lib.collect.nestedset.Depset; import com.google.devtools.build.lib.packages.BuildGlobals; import com.google.devtools.build.lib.packages.Proto; -import com.google.devtools.build.lib.packages.RepoCallable; +import com.google.devtools.build.lib.packages.RepoFileGlobals; import com.google.devtools.build.lib.packages.SelectorList; import com.google.devtools.build.lib.packages.StarlarkGlobals; import com.google.devtools.build.lib.packages.StarlarkNativeModule; @@ -133,7 +133,7 @@ public ImmutableMap getModuleToplevels() { @Override public ImmutableMap getRepoToplevels() { ImmutableMap.Builder env = ImmutableMap.builder(); - Starlark.addMethods(env, RepoCallable.INSTANCE); + Starlark.addMethods(env, RepoFileGlobals.INSTANCE); return env.buildOrThrow(); } diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectories.java b/src/main/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectories.java index e3c6e5ce250237..ec5ef9eb3efdf8 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectories.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectories.java @@ -37,8 +37,6 @@ /** * A set of subdirectories to ignore during target pattern matching or globbing. - * - *

This is currently just a prefix, but will eventually support glob-style wildcards. */ public final class IgnoredSubdirectories { public static final IgnoredSubdirectories EMPTY = diff --git a/src/main/java/com/google/devtools/build/lib/packages/RepoCallable.java b/src/main/java/com/google/devtools/build/lib/packages/RepoFileGlobals.java similarity index 78% rename from src/main/java/com/google/devtools/build/lib/packages/RepoCallable.java rename to src/main/java/com/google/devtools/build/lib/packages/RepoFileGlobals.java index 7c0c7aa06ea9d8..b381c257469e39 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RepoCallable.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RepoFileGlobals.java @@ -24,10 +24,10 @@ import net.starlark.java.eval.StarlarkThread; /** Definition of the {@code repo()} function used in REPO.bazel files. */ -public final class RepoCallable { - private RepoCallable() {} +public final class RepoFileGlobals { + private RepoFileGlobals() {} - public static final RepoCallable INSTANCE = new RepoCallable(); + public static final RepoFileGlobals INSTANCE = new RepoFileGlobals(); @StarlarkMethod( name = "ignore_directories", @@ -40,12 +40,16 @@ private RepoCallable() {} @ParamType(type = Sequence.class, generic1 = String.class), }) }) - public Object ignoreDirectories(Iterable dirsUnchecked, StarlarkThread thread) + public void ignoreDirectories(Iterable dirsUnchecked, StarlarkThread thread) throws EvalException { Sequence dirs = Sequence.cast(dirsUnchecked, String.class, "dirs"); RepoThreadContext context = RepoThreadContext.fromOrFail(thread, "repo()"); + + if (context.isIgnoredDirectoriesSet()) { + throw new EvalException("'ignored_directories()' can only be called once"); + } + context.setIgnoredDirectories(dirs); - return Starlark.NONE; } @StarlarkMethod( @@ -53,19 +57,21 @@ public Object ignoreDirectories(Iterable dirsUnchecked, StarlarkThread thread documented = false, // documented separately extraKeywords = @Param(name = "kwargs"), useStarlarkThread = true) - public Object repoCallable(Map kwargs, StarlarkThread thread) + public void repoCallable(Map kwargs, StarlarkThread thread) throws EvalException { RepoThreadContext context = RepoThreadContext.fromOrFail(thread, "repo()"); if (context.isRepoFunctionCalled()) { throw Starlark.errorf("'repo' can only be called once in the REPO.bazel file"); } - context.setRepoFunctionCalled(); + + if (context.isIgnoredDirectoriesSet()) { + throw Starlark.errorf("if repo() is called, it must be called before any other functions"); + } if (kwargs.isEmpty()) { throw Starlark.errorf("at least one argument must be given to the 'repo' function"); } context.setPackageArgsMap(kwargs); - return Starlark.NONE; } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/RepoThreadContext.java b/src/main/java/com/google/devtools/build/lib/packages/RepoThreadContext.java index afd092295ba0d2..92fc8c0428c2f5 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RepoThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RepoThreadContext.java @@ -56,11 +56,8 @@ public boolean isRepoFunctionCalled() { return repoFunctionCalled; } - public void setRepoFunctionCalled() { - repoFunctionCalled = true; - } - public void setPackageArgsMap(Map kwargs) { + repoFunctionCalled = true; this.packageArgsMap = ImmutableMap.copyOf(kwargs); } @@ -69,14 +66,14 @@ public ImmutableMap getPackageArgsMap() { } public void setIgnoredDirectories(Collection ignoredDirectories) throws EvalException { - if (ignoredDirectoriesSet) { - throw new EvalException("'ignored_directories()' can only be called once"); - } - ignoredDirectoriesSet = true; this.ignoredDirectories = ImmutableList.copyOf(ignoredDirectories); } + public boolean isIgnoredDirectoriesSet() { + return ignoredDirectoriesSet; + } + public ImmutableList getIgnoredDirectories() { return ignoredDirectories; } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java b/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java index e9259911e36a84..60ad786b7d6f82 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/UnixGlob.java @@ -947,7 +947,7 @@ private static boolean matchesPattern( || (j < path.length && matchesPattern(pattern, path, i, j + 1, patternCache, matchMode)); } if (j == path.length) { - return matchMode == MatchMode.CAN_MATCH_CHILD ? true : false; + return matchMode == MatchMode.CAN_MATCH_CHILD; } if (matches(pattern[i], path[j], patternCache)) { return matchesPattern(pattern, path, i + 1, j + 1, patternCache, matchMode); diff --git a/src/test/shell/bazel/bazelignore_test.sh b/src/test/shell/bazel/bazelignore_test.sh index 74fa3d9a9dcbfe..1b51fd37b64854 100755 --- a/src/test/shell/bazel/bazelignore_test.sh +++ b/src/test/shell/bazel/bazelignore_test.sh @@ -275,4 +275,17 @@ EOF fi } +test_ignore_directories_after_repo_in_repo_bazel() { + rm -rf work && mkdir work && cd work + setup_module_dot_bazel + cat >REPO.bazel <<'EOF' +ignore_directories(["**/sub", "foo/.hidden*"]) +repo(default_visibility=["//visibility:public"]) +EOF + + touch BUILD.bazel + bazel query //:all >& "$TEST_log" && fail "failure expected" + expect_log "it must be called before any other functions" +} + run_suite "Integration tests for .bazelignore" From 407a171b21296c5e9146f2d6e0021bb1ac0db4b5 Mon Sep 17 00:00:00 2001 From: "Lukacs T. Berki" Date: Thu, 31 Oct 2024 08:16:40 +0000 Subject: [PATCH 4/7] Address code review comments --- .../lib/cmdline/IgnoredSubdirectories.java | 11 ++- .../build/lib/packages/RepoFileGlobals.java | 6 +- .../google/devtools/build/lib/skyframe/BUILD | 2 +- .../BazelSkyframeExecutorConstants.java | 9 +-- .../IgnoredPackagePrefixesFunction.java | 24 +++--- .../build/lib/skyframe/PackageFunction.java | 14 ++-- .../build/lib/skyframe/RepoFileFunction.java | 10 +-- ...tion.java => RepoPackageArgsFunction.java} | 50 +++++++++---- .../build/lib/skyframe/SkyFunctions.java | 2 +- .../build/lib/skyframe/SkyframeExecutor.java | 2 +- .../packages/AbstractPackageLoader.java | 7 +- .../bzlmod/ModuleExtensionResolutionTest.java | 3 +- .../bazel/bzlmod/ModuleFileFunctionTest.java | 3 +- .../cmdline/IgnoredSubdirectoriesTest.java | 73 +++++++++++++++++++ .../query2/testutil/SkyframeQueryHelper.java | 2 +- .../repository/RepositoryDelegatorTest.java | 3 +- .../google/devtools/build/lib/skyframe/BUILD | 2 + .../ContainingPackageLookupFunctionTest.java | 2 +- .../skyframe/FilesetEntryFunctionTest.java | 2 +- .../build/lib/skyframe/GlobTestBase.java | 10 ++- .../skyframe/PackageLookupFunctionTest.java | 8 +- ...psOfPatternsFunctionSmartNegationTest.java | 4 +- ...ursiveFilesystemTraversalFunctionTest.java | 2 +- 23 files changed, 180 insertions(+), 71 deletions(-) rename src/main/java/com/google/devtools/build/lib/skyframe/{PackageArgsFunction.java => RepoPackageArgsFunction.java} (68%) create mode 100644 src/test/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectoriesTest.java diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectories.java b/src/main/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectories.java index ec5ef9eb3efdf8..0efc379353e0d6 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectories.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectories.java @@ -121,14 +121,23 @@ public IgnoredSubdirectories withPrefix(PathFragment prefix) { public IgnoredSubdirectories union(IgnoredSubdirectories other) { return new IgnoredSubdirectories( ImmutableSet.builder().addAll(prefixes).addAll(other.prefixes).build(), - ImmutableList.builder().addAll(patterns).addAll(other.patterns).build()); + ImmutableList.copyOf( + ImmutableSet.builder().addAll(patterns).addAll(other.patterns).build())); } /** Filters out entries that cannot match anything under {@code directory}. */ public IgnoredSubdirectories filterForDirectory(PathFragment directory) { ImmutableSet filteredPrefixes = prefixes.stream().filter(p -> p.startsWith(directory)).collect(toImmutableSet()); + + String[] splitDirectory = Iterables.toArray( + SLASH_SPLITTER.split(directory.getPathString()), String.class); ImmutableList.Builder filteredPatterns = ImmutableList.builder(); + for (int i = 0; i < patterns.size(); i++) { + if (UnixGlob.canMatchChild(splitPatterns.get(i), splitDirectory)) { + filteredPatterns.add(patterns.get(i)); + } + } return new IgnoredSubdirectories(filteredPrefixes, filteredPatterns.build()); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/RepoFileGlobals.java b/src/main/java/com/google/devtools/build/lib/packages/RepoFileGlobals.java index b381c257469e39..f3155bd00e9982 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RepoFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RepoFileGlobals.java @@ -31,8 +31,12 @@ private RepoFileGlobals() {} @StarlarkMethod( name = "ignore_directories", + doc = "The list of directories to ignore in this repository.

This function takes a list" + + " of strings and a directory is ignored if any of the given strings matches its" + + " repository-relative path according to the semantics of the glob()" + + " function. This function can be used to ignore directories that are implementation" + + " details of source control systems, output files of other build systems, etc.", useStarlarkThread = true, - documented = false, // TODO parameters = { @Param( name = "dirs", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 253f494bd0d337..e28015968128ac 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -2335,7 +2335,7 @@ java_library( java_library( name = "package_args_function", - srcs = ["PackageArgsFunction.java"], + srcs = ["RepoPackageArgsFunction.java"], deps = [ ":repo_file_function", ":repo_file_value", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java b/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java index bf6de69371c4fe..b7fd5cc48901ed 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java @@ -43,14 +43,7 @@ private BazelSkyframeExecutorConstants() {} * therefore fails the build, this ignore functionality currently has no chance to kick in. */ public static final SkyFunction IGNORED_PACKAGE_PREFIXES_FUNCTION = - new IgnoredPackagePrefixesFunction(BAZELIGNORE_PATH, true); - - /** - * IGNORED_PACKAGE_PREFIXES_FUNCTION, except always returns the empty value. Used for tests where - * the extra complications incurred by evaluating the function are undesired. - */ - public static final SkyFunction NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION = - new IgnoredPackagePrefixesFunction(PathFragment.EMPTY_FRAGMENT, false); + new IgnoredPackagePrefixesFunction(BAZELIGNORE_PATH); public static final CrossRepositoryLabelViolationStrategy CROSS_REPOSITORY_LABEL_VIOLATION_STRATEGY = CrossRepositoryLabelViolationStrategy.ERROR; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesFunction.java index ef6f1ce23cf0a0..481785f2bad6a6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/IgnoredPackagePrefixesFunction.java @@ -42,16 +42,24 @@ /** * A {@link SkyFunction} for {@link IgnoredPackagePrefixesValue}. * - *

It is used to implement the `.bazelignore` feature. + *

It is used to compute which directories should be ignored in a package. These either come + * from the {@code .bazelignore} file or from the {@code ignored_directories()} function in + * {@code REPO.bazel}. */ public class IgnoredPackagePrefixesFunction implements SkyFunction { + /** + * A version of {@link IgnoredPackagePrefixesFunction} that always returns the empty value. + * + *

Used for tests where the extra complications incurred by evaluating the function are + * undesired. + */ + public static final SkyFunction NOOP = (skyKey, env) -> IgnoredPackagePrefixesValue.EMPTY; + private final PathFragment ignoredPackagePrefixesFile; - private final boolean useRepoFile; public IgnoredPackagePrefixesFunction( - PathFragment ignoredPackagePrefixesFile, boolean useRepoFile) { + PathFragment ignoredPackagePrefixesFile) { this.ignoredPackagePrefixesFile = ignoredPackagePrefixesFile; - this.useRepoFile = useRepoFile; } public static void getIgnoredPackagePrefixes( @@ -80,10 +88,6 @@ private ImmutableList computeIgnoredPatterns( Environment env, RepositoryName repositoryName) throws IgnoredPatternsFunctionException, InterruptedException { - if (!useRepoFile) { - return ImmutableList.of(); - } - try { RepoFileValue repoFileValue = (RepoFileValue) @@ -106,10 +110,6 @@ private ImmutableList computeIgnoredPatterns( private ImmutableSet computeIgnoredPrefixes( Environment env, RepositoryName repositoryName) throws IgnoredPatternsFunctionException, InterruptedException { - if (ignoredPackagePrefixesFile.equals(PathFragment.EMPTY_FRAGMENT)) { - return ImmutableSet.of(); - } - ImmutableSet.Builder ignoredPackagePrefixesBuilder = ImmutableSet.builder(); PathPackageLocator pkgLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env); if (env.valuesMissing()) { 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 5b0316a8838d08..29ee962532b840 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 @@ -57,7 +57,7 @@ import com.google.devtools.build.lib.server.FailureDetails.Filesystem; import com.google.devtools.build.lib.server.FailureDetails.PackageLoading; import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code; -import com.google.devtools.build.lib.skyframe.PackageArgsFunction.PackageArgsValue; +import com.google.devtools.build.lib.skyframe.RepoPackageArgsFunction.RepoPackageArgsValue; import com.google.devtools.build.lib.skyframe.PackageFunctionWithMultipleGlobDeps.SkyframeGlobbingIOException; import com.google.devtools.build.lib.skyframe.RepoFileFunction.BadRepoFileException; import com.google.devtools.build.lib.skyframe.StarlarkBuiltinsFunction.BuiltinsFailedException; @@ -995,20 +995,20 @@ private LoadedPackage loadPackage( IgnoredPackagePrefixesValue repositoryIgnoredPackagePrefixes = (IgnoredPackagePrefixesValue) env.getValue(IgnoredPackagePrefixesValue.key(packageId.getRepository())); - PackageArgsValue packageArgsValue; + RepoPackageArgsValue repoPackageArgsValue; if (shouldUseRepoDotBazel) { try { - packageArgsValue = - (PackageArgsValue) + repoPackageArgsValue = + (RepoPackageArgsValue) env.getValueOrThrow( - PackageArgsFunction.key(packageId.getRepository()), + RepoPackageArgsFunction.key(packageId.getRepository()), IOException.class, BadRepoFileException.class); } catch (IOException | BadRepoFileException e) { throw badRepoFileException(e, packageId); } } else { - packageArgsValue = PackageArgsValue.EMPTY; + repoPackageArgsValue = RepoPackageArgsValue.EMPTY; } if (env.valuesMissing()) { @@ -1174,7 +1174,7 @@ private LoadedPackage loadPackage( pkgBuilder.mergePackageArgsFrom( PackageArgs.builder().setDefaultVisibility(defaultVisibility)); - pkgBuilder.mergePackageArgsFrom(packageArgsValue.getPackageArgs()); + pkgBuilder.mergePackageArgsFrom(repoPackageArgsValue.getPackageArgs()); if (compiled.ok()) { packageFactory.executeBuildFile( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java index f77463e2c3e090..ce7a4b7ccbd153 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java @@ -103,7 +103,6 @@ public SkyValue compute(SkyKey skyKey, Environment env) repoFile, repoName, RepositoryMapping.ALWAYS_FALLBACK, - null, starlarkSemantics, env.getListener()); } @@ -140,11 +139,10 @@ private RepoFileValue evalRepoFile( StarlarkFile starlarkFile, RepositoryName repoName, RepositoryMapping repoMapping, - RepositoryMapping mainRepoMapping, StarlarkSemantics starlarkSemantics, ExtendedEventHandler handler) throws RepoFileFunctionException, InterruptedException { - String repoDisplayName = getDisplayNameForRepo(repoName, mainRepoMapping); + String repoDisplayName = getDisplayNameForRepo(repoName, null); try (Mutability mu = Mutability.create("repo file", repoName)) { new DotBazelFileSyntaxChecker("REPO.bazel files", /* canLoadBzl= */ false) .check(starlarkFile); @@ -161,7 +159,7 @@ private RepoFileValue evalRepoFile( new RepoThreadContext( new LabelConverter( PackageIdentifier.create(repoName, PathFragment.EMPTY_FRAGMENT), repoMapping), - mainRepoMapping); + null); context.storeInThread(thread); Starlark.execFileProgram(program, predeclared, thread); return RepoFileValue.of(context.getPackageArgsMap(), context.getIgnoredDirectories()); @@ -188,11 +186,11 @@ public BadRepoFileException(String message, Exception cause) { } static class RepoFileFunctionException extends SkyFunctionException { - public RepoFileFunctionException(IOException e, Transience transience) { + private RepoFileFunctionException(IOException e, Transience transience) { super(e, transience); } - public RepoFileFunctionException(BadRepoFileException e) { + private RepoFileFunctionException(BadRepoFileException e) { super(e, Transience.PERSISTENT); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageArgsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepoPackageArgsFunction.java similarity index 68% rename from src/main/java/com/google/devtools/build/lib/skyframe/PackageArgsFunction.java rename to src/main/java/com/google/devtools/build/lib/skyframe/RepoPackageArgsFunction.java index 28f359478e2d2d..2e640c48500151 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageArgsFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepoPackageArgsFunction.java @@ -19,8 +19,6 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.LabelConverter; import com.google.devtools.build.lib.packages.PackageArgs; -import com.google.devtools.build.lib.skyframe.RepoFileFunction.BadRepoFileException; -import com.google.devtools.build.lib.skyframe.RepoFileFunction.RepoFileFunctionException; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.skyframe.AbstractSkyKey; import com.google.devtools.build.skyframe.SkyFunction; @@ -33,16 +31,16 @@ import net.starlark.java.eval.EvalException; /** A @{link SkyFunction} that returns the {@link PackageArgs} for a given repository. */ -public class PackageArgsFunction implements SkyFunction { - public static final PackageArgsFunction INSTANCE = new PackageArgsFunction(); +public class RepoPackageArgsFunction implements SkyFunction { + public static final RepoPackageArgsFunction INSTANCE = new RepoPackageArgsFunction(); /** {@link SkyValue} wrapping a PackageArgs. */ - public static final class PackageArgsValue implements SkyValue { - public static final PackageArgsValue EMPTY = new PackageArgsValue(PackageArgs.EMPTY); + public static final class RepoPackageArgsValue implements SkyValue { + public static final RepoPackageArgsValue EMPTY = new RepoPackageArgsValue(PackageArgs.EMPTY); private final PackageArgs packageArgs; - public PackageArgsValue(PackageArgs packageArgs) { + public RepoPackageArgsValue(PackageArgs packageArgs) { this.packageArgs = packageArgs; } @@ -57,7 +55,7 @@ public int hashCode() { @Override public boolean equals(Object other) { - if (other instanceof PackageArgsValue that) { + if (other instanceof RepoPackageArgsValue that) { return that.packageArgs.equals(packageArgs); } else { return false; @@ -69,7 +67,25 @@ public static Key key(RepositoryName repoName) { return new Key(repoName); } - /** Key type for {@link RepoFileValue}. */ + /** Thrown when there is something wrong with the arguments of the {@code repo()} function. */ + private static class BadPackageArgsException extends Exception { + public BadPackageArgsException(String message, Exception cause) { + super(message, cause); + } + } + + /** A {@link SkyFunctionException} for @{link RepoPackageArgsFunction}.*/ + private static class RepoPackageArgsFunctionException extends SkyFunctionException { + public RepoPackageArgsFunctionException(Exception cause, Transience transience) { + super(cause, transience); + } + + private RepoPackageArgsFunctionException(BadPackageArgsException e) { + super(e, Transience.PERSISTENT); + } + } + + /** Key type for {@link RepoPackageArgsValue}. */ public static class Key extends AbstractSkyKey { private Key(RepositoryName repoName) { @@ -78,11 +94,11 @@ private Key(RepositoryName repoName) { @Override public SkyFunctionName functionName() { - return SkyFunctions.PACKAGE_ARGS; + return SkyFunctions.REPO_PACKAGE_ARGS; } } - private PackageArgsFunction() {} + private RepoPackageArgsFunction() {} @Nullable @Override @@ -93,11 +109,15 @@ public SkyValue compute(SkyKey skyKey, Environment env) RepoFileValue repoFileValue = (RepoFileValue) env.getValue(RepoFileValue.key(repositoryName)); RepositoryMappingValue repositoryMappingValue = (RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(repositoryName)); + RepositoryMappingValue mainRepoMapping = + (RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(RepositoryName.MAIN)); if (env.valuesMissing()) { return null; } - String repoDisplayName = RepoFileFunction.getDisplayNameForRepo(repositoryName, null); + + String repoDisplayName = RepoFileFunction.getDisplayNameForRepo( + repositoryName, mainRepoMapping.getRepositoryMapping()); PackageArgs.Builder pkgArgsBuilder = PackageArgs.builder(); LabelConverter labelConverter = @@ -115,10 +135,10 @@ public SkyValue compute(SkyKey skyKey, Environment env) } } catch (EvalException e) { env.getListener().handle(Event.error(e.getMessageWithStack())); - throw new RepoFileFunctionException( - new BadRepoFileException("error evaluating REPO.bazel file for " + repoDisplayName, e)); + throw new RepoPackageArgsFunctionException( + new BadPackageArgsException("error evaluating REPO.bazel file for " + repoDisplayName, e)); } - return new PackageArgsValue(pkgArgsBuilder.build()); + return new RepoPackageArgsValue(pkgArgsBuilder.build()); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java index 338c2740fc3113..d2e21828ab77c3 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java @@ -147,7 +147,7 @@ public final class SkyFunctions { SkyFunctionName.createHermetic("RESOLVED_FILE"); public static final SkyFunctionName MODULE_FILE = SkyFunctionName.createNonHermetic("MODULE_FILE"); - public static final SkyFunctionName PACKAGE_ARGS = SkyFunctionName.createHermetic("PACKAGE_ARGS"); + public static final SkyFunctionName REPO_PACKAGE_ARGS = SkyFunctionName.createHermetic("REPO_PACKAGE_ARGS"); public static final SkyFunctionName REPO_FILE = SkyFunctionName.createHermetic("REPO_FILE"); public static final SkyFunctionName BUILD_DRIVER = SkyFunctionName.createNonHermetic("BUILD_DRIVER"); 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 6a4a9b6a072ab7..92f266d35a0b1b 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 @@ -765,7 +765,7 @@ private ImmutableMap skyFunctions() { : (k, env) -> { throw new IllegalStateException("supposed to be unused"); }); - map.put(SkyFunctions.PACKAGE_ARGS, PackageArgsFunction.INSTANCE); + map.put(SkyFunctions.REPO_PACKAGE_ARGS, RepoPackageArgsFunction.INSTANCE); map.put(SkyFunctions.EXTERNAL_PACKAGE, new ExternalPackageFunction(externalPackageHelper)); // Inject an empty default BAZEL_DEP_GRAPH SkyFunction for unit tests. map.put( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java index 6c91ebd303c3d6..75cceb9efda341 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java @@ -69,7 +69,7 @@ import com.google.devtools.build.lib.skyframe.FileFunction; import com.google.devtools.build.lib.skyframe.FileStateFunction; import com.google.devtools.build.lib.skyframe.IgnoredPackagePrefixesFunction; -import com.google.devtools.build.lib.skyframe.PackageArgsFunction; +import com.google.devtools.build.lib.skyframe.RepoPackageArgsFunction; import com.google.devtools.build.lib.skyframe.PackageFunction; import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile; import com.google.devtools.build.lib.skyframe.PackageFunction.GlobbingStrategy; @@ -89,7 +89,6 @@ import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.FileStateKey; import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.skyframe.Differencer; import com.google.devtools.build.skyframe.Differencer.DiffWithDelta.Delta; @@ -562,7 +561,7 @@ public String getBaseNameForLoadedPackage(PackageIdentifier packageName) { getExternalPackageHelper())) .put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - new IgnoredPackagePrefixesFunction(PathFragment.EMPTY_FRAGMENT, false)) + IgnoredPackagePrefixesFunction.NOOP) .put(SkyFunctions.CONTAINING_PACKAGE_LOOKUP, new ContainingPackageLookupFunction()) .put( SkyFunctions.BZL_COMPILE, @@ -583,7 +582,7 @@ public String getBaseNameForLoadedPackage(PackageIdentifier packageName) { SkyFunctions.REPO_FILE, new RepoFileFunction( ruleClassProvider.getBazelStarlarkEnvironment(), directories.getWorkspace())) - .put(SkyFunctions.PACKAGE_ARGS, PackageArgsFunction.INSTANCE) + .put(SkyFunctions.REPO_PACKAGE_ARGS, RepoPackageArgsFunction.INSTANCE) .put(SkyFunctions.EXTERNAL_PACKAGE, new ExternalPackageFunction(getExternalPackageHelper())) .put( BzlmodRepoRuleValue.BZLMOD_REPO_RULE, diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index 43c0d1edeca032..54aaca79f19fb4 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -61,6 +61,7 @@ import com.google.devtools.build.lib.skyframe.ExternalPackageFunction; import com.google.devtools.build.lib.skyframe.FileFunction; import com.google.devtools.build.lib.skyframe.FileStateFunction; +import com.google.devtools.build.lib.skyframe.IgnoredPackagePrefixesFunction; import com.google.devtools.build.lib.skyframe.LocalRepositoryLookupFunction; import com.google.devtools.build.lib.skyframe.PackageFunction; import com.google.devtools.build.lib.skyframe.PackageLookupFunction; @@ -209,7 +210,7 @@ public void setup() throws Exception { BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER)) .put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - BazelSkyframeExecutorConstants.NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION) + IgnoredPackagePrefixesFunction.NOOP) .put( SkyFunctions.REPOSITORY_MAPPING, new RepositoryMappingFunction(ruleClassProvider)) diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java index ec7e94735d4d07..1a6aaa9ca260f8 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java @@ -51,6 +51,7 @@ import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction; import com.google.devtools.build.lib.skyframe.FileFunction; import com.google.devtools.build.lib.skyframe.FileStateFunction; +import com.google.devtools.build.lib.skyframe.IgnoredPackagePrefixesFunction; import com.google.devtools.build.lib.skyframe.LocalRepositoryLookupFunction; import com.google.devtools.build.lib.skyframe.PackageLookupFunction; import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy; @@ -162,7 +163,7 @@ private void setUpWithBuiltinModules(ImmutableMap b BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER)) .put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - BazelSkyframeExecutorConstants.NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION) + IgnoredPackagePrefixesFunction.NOOP) .put( SkyFunctions.LOCAL_REPOSITORY_LOOKUP, new LocalRepositoryLookupFunction( diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectoriesTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectoriesTest.java new file mode 100644 index 00000000000000..755283eae9bd2c --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/cmdline/IgnoredSubdirectoriesTest.java @@ -0,0 +1,73 @@ +package com.google.devtools.build.lib.cmdline; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.Arrays; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link IgnoredSubdirectories}. */ +@RunWith(JUnit4.class) +public class IgnoredSubdirectoriesTest { + private static ImmutableSet prefixes(String... prefixes) { + return Arrays.stream(prefixes).map(PathFragment::create).collect(ImmutableSet.toImmutableSet()); + } + + private static ImmutableList patterns(String... patterns) { + return ImmutableList.copyOf(patterns); + } + + @Test + public void testSimpleUnion() throws Exception { + IgnoredSubdirectories one = IgnoredSubdirectories.of( + prefixes("prefix1"), patterns("pattern1")); + IgnoredSubdirectories two = IgnoredSubdirectories.of( + prefixes("prefix2"), patterns("pattern2")); + IgnoredSubdirectories union = one.union(two); + assertThat(union).isEqualTo(IgnoredSubdirectories.of( + prefixes("prefix1", "prefix2"), patterns("pattern1", "pattern2"))); + } + + @Test + public void testSelfUnionNoop() throws Exception { + IgnoredSubdirectories ignored = IgnoredSubdirectories.of(prefixes("pre"), patterns("pat")); + assertThat(ignored.union(ignored)).isEqualTo(ignored); + } + + @Test + public void filterPrefixes() throws Exception { + IgnoredSubdirectories original = IgnoredSubdirectories.of( + prefixes("foo", "bar", "barbaz", "bar/qux")); + IgnoredSubdirectories filtered = original.filterForDirectory(PathFragment.create("bar")); + assertThat(filtered).isEqualTo(IgnoredSubdirectories.of(prefixes("bar", "bar/qux"))); + } + + @Test + public void filterPatterns() throws Exception { + IgnoredSubdirectories original = IgnoredSubdirectories.of( + prefixes(), + patterns("**/sub", "foo", "bar/*/onesub", "bar/qux/**", "bar/ba*", "bar/not/*/twosub", + "bar/**/barsub", "bar/sub/subsub")); + IgnoredSubdirectories filtered = original.filterForDirectory(PathFragment.create("bar/sub")); + assertThat(filtered).isEqualTo(IgnoredSubdirectories.of( + prefixes(), + patterns("**/sub", "bar/*/onesub", "bar/**/barsub", "bar/sub/subsub"))); + } + + @Test + public void filterPatternsForHiddenFiles() throws Exception { + IgnoredSubdirectories original = IgnoredSubdirectories.of( + prefixes(), + patterns("not/sub", "*dden/sub", ".hidden/**/sub", ".hi*/*/sub", "*/sub", "**/sub")); + IgnoredSubdirectories filtered = original.filterForDirectory(PathFragment.create(".hidden")); + // Glob semantics say that "*dden" is not supposed to match ".hidden". + // "**" and "*" and ".hi*" should, though. + assertThat(filtered).isEqualTo(IgnoredSubdirectories.of( + prefixes(), + patterns(".hidden/**/sub", ".hi*/*/sub", "*/sub", "**/sub"))); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java index fd7722ac9d0117..19ff63f1f44efc 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java +++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java @@ -425,7 +425,7 @@ protected SkyframeExecutor createSkyframeExecutor(ConfiguredRuleClassProvider ru .setDirectories(directories) .setActionKeyContext(actionKeyContext) .setIgnoredPackagePrefixesFunction( - new IgnoredPackagePrefixesFunction(ignoredPackagePrefixesFile, true)) + new IgnoredPackagePrefixesFunction(ignoredPackagePrefixesFile)) .setExtraSkyFunctions(analysisMock.getSkyFunctions(directories)) .setSyscallCache(delegatingSyscallCache) .build(); diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java index f26d8e6e4ea904..a5fac27bd90cc8 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java @@ -69,6 +69,7 @@ import com.google.devtools.build.lib.skyframe.ExternalPackageFunction; import com.google.devtools.build.lib.skyframe.FileFunction; import com.google.devtools.build.lib.skyframe.FileStateFunction; +import com.google.devtools.build.lib.skyframe.IgnoredPackagePrefixesFunction; import com.google.devtools.build.lib.skyframe.LocalRepositoryLookupFunction; import com.google.devtools.build.lib.skyframe.PackageFunction; import com.google.devtools.build.lib.skyframe.PackageLookupFunction; @@ -230,7 +231,7 @@ public void setupDelegator() throws Exception { .put(SkyFunctions.CONTAINING_PACKAGE_LOOKUP, new ContainingPackageLookupFunction()) .put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - BazelSkyframeExecutorConstants.NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION) + IgnoredPackagePrefixesFunction.NOOP) .put( SkyFunctions.REPOSITORY_MAPPING, new RepositoryMappingFunction(ruleClassProvider)) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD index 1fe436742a4c24..4d9fa8c6da9ee6 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD @@ -560,6 +560,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:invalid_glob_pattern_exception", "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:repo_file_function", "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value", "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster", @@ -1443,6 +1444,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function", "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_value", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", + "//src/main/java/com/google/devtools/build/lib/skyframe:repo_file_function", "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value", "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java index a60c480e039040..e4494eb1ddee8d 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java @@ -112,7 +112,7 @@ public final void setUp() throws Exception { skyFunctions.put(SkyFunctions.PACKAGE, PackageFunction.newBuilder().build()); skyFunctions.put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - BazelSkyframeExecutorConstants.NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION); + IgnoredPackagePrefixesFunction.NOOP); skyFunctions.put( FileStateKey.FILE_STATE, new FileStateFunction( diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java index f55565660bb37d..92cc9bf7bff58e 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesetEntryFunctionTest.java @@ -121,7 +121,7 @@ public void setUp() throws Exception { BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER)); skyFunctions.put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - BazelSkyframeExecutorConstants.NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION); + IgnoredPackagePrefixesFunction.NOOP); skyFunctions.put( SkyFunctions.FILESET_ENTRY, new FilesetEntryFunction((unused) -> rootDirectory)); skyFunctions.put(SkyFunctions.WORKSPACE_NAME, new TestWorkspaceNameFunction()); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/GlobTestBase.java b/src/test/java/com/google/devtools/build/lib/skyframe/GlobTestBase.java index 1aedeead90957e..80e1fe4be980ad 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/GlobTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/GlobTestBase.java @@ -145,6 +145,8 @@ private Map createFunctionMap() { ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS, directories); + AnalysisMock analysisMock = AnalysisMock.get(); + RuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider(); Map skyFunctions = new HashMap<>(); createGlobSkyFunction(skyFunctions); skyFunctions.put( @@ -158,9 +160,13 @@ private Map createFunctionMap() { CrossRepositoryLabelViolationStrategy.ERROR, BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY, BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER)); + skyFunctions.put( + SkyFunctions.REPO_FILE, + new RepoFileFunction( + ruleClassProvider.getBazelStarlarkEnvironment(), directories.getWorkspace())); skyFunctions.put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - new IgnoredPackagePrefixesFunction(BazelSkyframeExecutorConstants.BAZELIGNORE_PATH, false)); + new IgnoredPackagePrefixesFunction(BazelSkyframeExecutorConstants.BAZELIGNORE_PATH)); skyFunctions.put( FileStateKey.FILE_STATE, new FileStateFunction( @@ -173,8 +179,6 @@ private Map createFunctionMap() { skyFunctions.put(SkyFunctions.FILE, new FileFunction(pkgLocator, directories)); skyFunctions.put( FileSymlinkCycleUniquenessFunction.NAME, new FileSymlinkCycleUniquenessFunction()); - AnalysisMock analysisMock = AnalysisMock.get(); - RuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider(); skyFunctions.put( WorkspaceFileValue.WORKSPACE_FILE, new WorkspaceFileFunction( diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java index d984f744148922..ceadca14725028 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java @@ -112,6 +112,7 @@ public final void setUp() throws Exception { ExternalFilesHelper externalFilesHelper = ExternalFilesHelper.createForTesting( pkgLocator, ExternalFileAction.DEPEND_ON_EXTERNAL_PKG_FOR_EXTERNAL_REPO_PATHS, directories); + RuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider(); Map skyFunctions = new HashMap<>(); skyFunctions.put( SkyFunctions.PACKAGE_LOOKUP, @@ -132,11 +133,14 @@ public final void setUp() throws Exception { skyFunctions.put( SkyFunctions.DIRECTORY_LISTING_STATE, new DirectoryListingStateFunction(externalFilesHelper, SyscallCache.NO_CACHE)); + skyFunctions.put( + SkyFunctions.REPO_FILE, + new RepoFileFunction(ruleClassProvider.getBazelStarlarkEnvironment(), + directories.getWorkspace())); skyFunctions.put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, new IgnoredPackagePrefixesFunction( - PathFragment.create(IGNORED_PACKAGE_PREFIXES_FILE_PATH_STRING), false)); - RuleClassProvider ruleClassProvider = analysisMock.createRuleClassProvider(); + PathFragment.create(IGNORED_PACKAGE_PREFIXES_FILE_PATH_STRING))); skyFunctions.put( WorkspaceFileValue.WORKSPACE_FILE, new WorkspaceFileFunction( diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionSmartNegationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionSmartNegationTest.java index 3b92433df54888..9a3c18ec2c8c88 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionSmartNegationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternsFunctionSmartNegationTest.java @@ -97,8 +97,8 @@ public void setUp() throws Exception { .setSyscallCache(SyscallCache.NO_CACHE) .setIgnoredPackagePrefixesFunction( new IgnoredPackagePrefixesFunction( - PathFragment.create(ADDITIONAL_IGNORED_PACKAGE_PREFIXES_FILE_PATH_STRING), - false)) + PathFragment.create(ADDITIONAL_IGNORED_PACKAGE_PREFIXES_FILE_PATH_STRING) + )) .build(); SkyframeExecutorTestHelper.process(skyframeExecutor); OptionsParser optionsParser = diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java index 9f7e131e2d47bd..ae21187a6e4bbb 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java @@ -179,7 +179,7 @@ public void setUp() { BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER)); skyFunctions.put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - BazelSkyframeExecutorConstants.NOOP_IGNORED_PACKAGE_PREFIXES_FUNCTION); + IgnoredPackagePrefixesFunction.NOOP); skyFunctions.put(SkyFunctions.PACKAGE, PackageFunction.newBuilder().build()); skyFunctions.put( WorkspaceFileValue.WORKSPACE_FILE, From 2a0d045ca75a12aded666d7f25eaad36df65e048 Mon Sep 17 00:00:00 2001 From: "Lukacs T. Berki" Date: Thu, 31 Oct 2024 15:30:58 +0000 Subject: [PATCH 5/7] remove useless ctor arguments of RepoThreadContext --- .../build/lib/packages/RepoThreadContext.java | 11 ++--------- .../devtools/build/lib/skyframe/RepoFileFunction.java | 6 +----- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/RepoThreadContext.java b/src/main/java/com/google/devtools/build/lib/packages/RepoThreadContext.java index 92fc8c0428c2f5..b1481bbdad440f 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RepoThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RepoThreadContext.java @@ -26,8 +26,6 @@ /** Context object for a Starlark thread evaluating the REPO.bazel file. */ public class RepoThreadContext extends StarlarkThreadContext { - private final LabelConverter labelConverter; - private ImmutableMap packageArgsMap = ImmutableMap.of(); private boolean repoFunctionCalled = false; @@ -43,13 +41,8 @@ public static RepoThreadContext fromOrFail(StarlarkThread thread, String what) throw Starlark.errorf("%s can only be called from REPO.bazel", what); } - public RepoThreadContext(LabelConverter labelConverter, RepositoryMapping mainRepoMapping) { - super(() -> mainRepoMapping); - this.labelConverter = labelConverter; - } - - public LabelConverter getLabelConverter() { - return labelConverter; + public RepoThreadContext() { + super(() -> null); } public boolean isRepoFunctionCalled() { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java index ce7a4b7ccbd153..86e4de26a0522b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java @@ -155,11 +155,7 @@ private RepoFileValue evalRepoFile( /* contextDescription= */ "", SymbolGenerator.create(repoName)); thread.setPrintHandler(Event.makeDebugPrintHandler(handler)); - RepoThreadContext context = - new RepoThreadContext( - new LabelConverter( - PackageIdentifier.create(repoName, PathFragment.EMPTY_FRAGMENT), repoMapping), - null); + RepoThreadContext context = new RepoThreadContext(); context.storeInThread(thread); Starlark.execFileProgram(program, predeclared, thread); return RepoFileValue.of(context.getPackageArgsMap(), context.getIgnoredDirectories()); From 17961e5a6157c056bb67bf103e123507ea5dd29f Mon Sep 17 00:00:00 2001 From: "Lukacs T. Berki" Date: Thu, 31 Oct 2024 19:18:41 +0000 Subject: [PATCH 6/7] Address more review comments --- src/main/java/com/google/devtools/build/lib/skyframe/BUILD | 4 ++-- .../build/lib/skyframe/BazelSkyframeExecutorConstants.java | 7 +------ .../com/google/devtools/build/lib/skyframe/packages/BUILD | 2 +- .../google/devtools/build/lib/skyframe/GlobTestBase.java | 2 +- 4 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index e28015968128ac..1a2128c5370127 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -151,7 +151,6 @@ java_library( ":map_as_package_roots", ":metadata_consumer_for_metrics", ":node_dropping_inconsistency_receiver", - ":package_args_function", ":package_error_function", ":package_error_message_function", ":package_identifier_batching_callback", @@ -177,6 +176,7 @@ java_library( ":recursive_pkg_value", ":repo_file_function", ":repo_file_value", + ":repo_package_args_function", ":repository_mapping_function", ":repository_mapping_value", ":rule_configured_target_value", @@ -2334,7 +2334,7 @@ java_library( ) java_library( - name = "package_args_function", + name = "repo_package_args_function", srcs = ["RepoPackageArgsFunction.java"], deps = [ ":repo_file_function", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java b/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java index b7fd5cc48901ed..bd2539113cbd16 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BazelSkyframeExecutorConstants.java @@ -27,11 +27,6 @@ public class BazelSkyframeExecutorConstants { private BazelSkyframeExecutorConstants() {} - public static final ImmutableSet HARDCODED_IGNORED_PACKAGE_PREFIXES = - ImmutableSet.of(); - - public static final PathFragment BAZELIGNORE_PATH = PathFragment.create(".bazelignore"); - /** * The file .bazelignore can be used to specify directories to be ignored by bazel * @@ -43,7 +38,7 @@ private BazelSkyframeExecutorConstants() {} * therefore fails the build, this ignore functionality currently has no chance to kick in. */ public static final SkyFunction IGNORED_PACKAGE_PREFIXES_FUNCTION = - new IgnoredPackagePrefixesFunction(BAZELIGNORE_PATH); + new IgnoredPackagePrefixesFunction(PathFragment.create(".bazelignore")); public static final CrossRepositoryLabelViolationStrategy CROSS_REPOSITORY_LABEL_VIOLATION_STRATEGY = CrossRepositoryLabelViolationStrategy.ERROR; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD index 18795f41c6740c..e3823e63a0ee44 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BUILD @@ -58,12 +58,12 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:default_syscall_cache", "//src/main/java/com/google/devtools/build/lib/skyframe:file_function", "//src/main/java/com/google/devtools/build/lib/skyframe:ignored_package_prefixes_function", - "//src/main/java/com/google/devtools/build/lib/skyframe:package_args_function", "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function", "//src/main/java/com/google/devtools/build/lib/skyframe:package_value", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_function", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", "//src/main/java/com/google/devtools/build/lib/skyframe:repo_file_function", + "//src/main/java/com/google/devtools/build/lib/skyframe:repo_package_args_function", "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_function", "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value", "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/GlobTestBase.java b/src/test/java/com/google/devtools/build/lib/skyframe/GlobTestBase.java index 80e1fe4be980ad..ddc8900e827067 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/GlobTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/GlobTestBase.java @@ -166,7 +166,7 @@ private Map createFunctionMap() { ruleClassProvider.getBazelStarlarkEnvironment(), directories.getWorkspace())); skyFunctions.put( SkyFunctions.IGNORED_PACKAGE_PREFIXES, - new IgnoredPackagePrefixesFunction(BazelSkyframeExecutorConstants.BAZELIGNORE_PATH)); + BazelSkyframeExecutorConstants.IGNORED_PACKAGE_PREFIXES_FUNCTION); skyFunctions.put( FileStateKey.FILE_STATE, new FileStateFunction( From f7fa1582780d18490192fd110f3f54afaaecdc85 Mon Sep 17 00:00:00 2001 From: "Lukacs T. Berki" Date: Mon, 4 Nov 2024 07:10:43 +0000 Subject: [PATCH 7/7] fix last nits --- .../google/devtools/build/lib/skyframe/RepoFileFunction.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java index 86e4de26a0522b..9410a817af72f1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepoFileFunction.java @@ -102,7 +102,6 @@ public SkyValue compute(SkyKey skyKey, Environment env) return evalRepoFile( repoFile, repoName, - RepositoryMapping.ALWAYS_FALLBACK, starlarkSemantics, env.getListener()); } @@ -138,7 +137,6 @@ public static String getDisplayNameForRepo( private RepoFileValue evalRepoFile( StarlarkFile starlarkFile, RepositoryName repoName, - RepositoryMapping repoMapping, StarlarkSemantics starlarkSemantics, ExtendedEventHandler handler) throws RepoFileFunctionException, InterruptedException {