-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
False positives reported with --incompatible_disallow_empty_glob flag #8517
Comments
Thanks for the report. Can you do |
I added
|
Thanks a lot for the report! I can reproduce the problem on Bazel source code itself. Please ignore the flag |
There's a correctness issue.
It correctly reports the files from the current package. Then I modify the file (e.g. add a comment), and I get an error:
Looking at Bazel code, I see the error is raised by If I add the naive workaround: if (includes.isEmpty()) { return new ArrayList<>(); } Everything seems to work fine. cc @haxorz |
cursory analysis (i merely skimmed the source code and didn't e.g. repro in a debugger):
|
…ow_empty' param to the 'glob' function (1) Previously, the 'allow_empty=False' enforcement was solely in legacy globbing. So on incremental package evaluation where all the GlobValue nodes already existed in the Skyframe graph, we would not do any enforcement. This was fixed by having by Skyframe globbing and legacy globbing do enforcement. (2) On incremental package evaluation where all the GlobValue nodes already existed in the Skyframe graph, we would still call into legacy globbing with an empty list of glob patterns. Legacy globbing would then error out just as it does normally when a glob *include* pattern matches a bunch of stuff, but then all the matches are removed by an *exclude* pattern. This was fixed by having Skyframe hybrid globbing only call into legacy globbing if there is work to do. I added a bunch of unit tests for various incremental situations. I also added explicit unit tests for 'allow_empty=True'. We were previous missing all/some of this coverage, respectively. Fixes bazelbuild#8517 RELNOTES: None PiperOrigin-RevId: 253046516
…ow_empty' param to the 'glob' function (1) Previously, the 'allow_empty=False' enforcement was solely in legacy globbing. So on incremental package evaluation where all the GlobValue nodes already existed in the Skyframe graph, we would not do any enforcement. This was fixed by having by Skyframe globbing and legacy globbing do enforcement. (2) On incremental package evaluation where all the GlobValue nodes already existed in the Skyframe graph, we would still call into legacy globbing with an empty list of glob patterns. Legacy globbing would then error out just as it does normally when a glob *include* pattern matches a bunch of stuff, but then all the matches are removed by an *exclude* pattern. This was fixed by having Skyframe hybrid globbing only call into legacy globbing if there is work to do. I added a bunch of unit tests for various incremental situations. I also added explicit unit tests for 'allow_empty=True'. We were previous missing all/some of this coverage, respectively. Fixes bazelbuild#8517 RELNOTES: None PiperOrigin-RevId: 253046516
…ow_empty' param to the 'glob' function (1) Previously, the 'allow_empty=False' enforcement was solely in legacy globbing. So on incremental package evaluation where all the GlobValue nodes already existed in the Skyframe graph, we would not do any enforcement. This was fixed by having by Skyframe globbing and legacy globbing do enforcement. (2) On incremental package evaluation where all the GlobValue nodes already existed in the Skyframe graph, we would still call into legacy globbing with an empty list of glob patterns. Legacy globbing would then error out just as it does normally when a glob *include* pattern matches a bunch of stuff, but then all the matches are removed by an *exclude* pattern. This was fixed by having Skyframe hybrid globbing only call into legacy globbing if there is work to do. I added a bunch of unit tests for various incremental situations. I also added explicit unit tests for 'allow_empty=True'. We were previous missing all/some of this coverage, respectively. Fixes bazelbuild#8517 RELNOTES: None PiperOrigin-RevId: 253046516
On Bazel@HEAD (b2fac74) I'm seeing false positive reported on Gerrit Code Review repository (stable-2.14 branch):
This flag is tracked here.
The text was updated successfully, but these errors were encountered: