Sample .bazelignore wildcard pr using isPackageIgnored #13807
Conversation
reset changes implemented lazy wildcard matching changed to use custom globbing function because UnixGlob Builder take a long time for some reason clean up code added logs and comment fixed comment and log clean up code remove Eventhandler because it will throw error changed Symlink and use concurrent hash set added early stop
|
@janakdr any comment on this change? According to #13756 (comment) this change will fail the test cases |
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return patterns.hashCode(); | ||
| return ignoredPrefixes.hashCode() ; |
There was a problem hiding this comment.
nit: probably want to incorporate the wildcardPatterns as well into the hash code?
| @@ -54,19 +71,33 @@ public static SkyKey key(RepositoryName repository) { | |||
| } | |||
|
|
|||
| public ImmutableSet<PathFragment> getPatterns() { | |||
| return patterns; | |||
| return ignoredPrefixes; | |||
There was a problem hiding this comment.
What is this method needed for now that #isPathFragmentIgnored is available? It seems very likely that all callers have to be updated. One way to approach this would be to not change any end-to-end behavior first, but just to replace all usages of #getPatterns with usages of #isPathFragmentIgnored. Then in a follow-up you can change the implementation of #isPathFragmentIgnored, which will be a 1- or 2-file change, and add the tests.
There was a problem hiding this comment.
Thanks for the suggestion! Any advice on changing the place that doesn't perform the check immediately but pass the set of ignored directory PathFragment to others? For example, in TargetPatternFunction, it passes the set to TargetPattern. Then TargetPattern use the set to produce an IgnoredPathFragmentsInScopeOrFilteringIgnorer (through getAllIgnoredSubdirectoriesToExclude) and pass to TargetPatternResolver, etc.
There was a problem hiding this comment.
I would pass a predicate instead of the set. The predicate can be the method reference ::isPathFragmentIgnored. Is there any place that actually needs to iterate over the set, versus checking for membership?
There was a problem hiding this comment.
I think there are some places. Examples:
Line 174 in EnvironmentBackedRecursivePackageProvider.java & Line 46 in RecursivePkgValueRootPackageExtractor.java
ImmutableSet<PathFragment> filteredIgnoredSubdirectories =
ImmutableSet.copyOf(
Iterables.filter(ignoredSubdirectories, path -> path.startsWith(directory)));
There was a problem hiding this comment.
Any suggestion @janakdr ? Thanks!
There was a problem hiding this comment.
Sorry for the late reply. It looks like this feature would require a fair amount of work to the rest of Bazel to actually work as intended, but I'm open to it. In particular, the fancy "filtering" code you pointed to would no longer work trivially. I think you could make it work by enriching the Predicate you're providing to have a method #restrictedTo(PathFragment fragment). That method would filter the prefixes, and leave the regexes unchanged, since it would be fairly hard to test them.
There was a problem hiding this comment.
Thanks for the suggestion @janakdr! I am stuck on modifing this function:
public IgnoredPathFragmentsInScopeOrFilteringIgnorer getAllIgnoredSubdirectoriesToExclude(
InterruptibleSupplier<ImmutableSet<PathFragment>> ignoredPackagePrefixes)
throws InterruptedException {
ImmutableSet.Builder<PathFragment> ignoredPathsBuilder =
ImmutableSet.builderWithExpectedSize(0);
for (PathFragment ignoredPackagePrefix : ignoredPackagePrefixes.get()) {
if (this.containedIn(ignoredPackagePrefix)) {
return new IgnoredPathFragmentsInScopeOrFilteringIgnorer.FilteringIgnorer(
ignoredPackagePrefix);
}
PackageIdentifier pkgIdForIgnoredDirectorPrefix =
PackageIdentifier.create(this.getDirectory().getRepository(), ignoredPackagePrefix);
if (this.containsAllTransitiveSubdirectories(pkgIdForIgnoredDirectorPrefix)) {
ignoredPathsBuilder.add(ignoredPackagePrefix);
}
}
return IgnoredPathFragmentsInScopeOrFilteringIgnorer.IgnoredPathFragments.of(
ignoredPathsBuilder.build());
}
https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java#L630-L648.
Any help will be great! Thank you!
There was a problem hiding this comment.
It looks like you can do the filtering of the ignored paths as before, and just propagate any regexes along unchanged (since it would be very difficult to see if a regex could apply to a subdirectory). You'd probably want to have special cases for the "totally ignored," "empty ignored paths and empty regexes," and "non-empty ignored paths and empty regexes." You might want to reverse the logic, so the "filterer" takes in a target pattern and has to produce a new filterer, rather than putting the target pattern in charge, but I could see either way.
|
Any update on this PR? Some of us are really looking forward to glob support in |
sgowroji
added
team-Core
|
Nudging this. It seems like it's ready to go? |
|
@janakdr @David-Lam any advice on how we can move forward here? |
janakdr
requested review from
haxorz and
janakdr
and removed request for
janakdr
|
I totally failed to remove myself from the reviewers list, but at least I managed to add @haxorz. I'm no longer working on Bazel, so @haxorz will decide what to do with this PR. My understanding of the current status is that I suggested some changes to @David-Lam and they haven't been applied yet. I would lean towards closing this PR and waiting for @David-Lam to update it and re-request. |
That was my understanding too when I checked this PR in April (in my sweep of all pending PRs). I also flagged this PR and the general feature request to @sventiffe back then. Sven, I'd be happy to chat about this! |
|
Hi there! Thank you for contributing to the Bazel repository. We appreciate your time and effort. We're doing a clean up of old PRs and will be closing this one since it seems to have stalled. Please feel free to reopen/let us know if you’re still interested in pursuing this or if you'd like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so. |
A sample pr with recommended implementation from maintainer in pr:#13756.