Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Flip default for --incompatible_disallow_empty_glob #37

Closed
alexeagle opened this issue Mar 22, 2022 · 16 comments
Closed

Flip default for --incompatible_disallow_empty_glob #37

alexeagle opened this issue Mar 22, 2022 · 16 comments
Assignees
@limdor
Labels
bazel-flags Flip default for Bazel flags bounty-1000USD A contributor who completes this will be rewarded $1000

Comments

@alexeagle
Copy link
Contributor

Help to finish bazelbuild/bazel#8195 by finding projects on Bazel CI which need an explicit setting turning this off, then land a PR in Bazel to change the default for new projects.
@alexeagle alexeagle added the bounty-1000USD A contributor who completes this will be rewarded $1000 label Apr 20, 2022
@limdor
Copy link

limdor commented Apr 24, 2022

The label took my attention and I decided to spend some hours to take a look how much is left. The interesting part is that indeed the first problem is to flip it in Bazel without even caring about the downstream projects. Bazel itself has quite a lot of empty globs.

When doing that, one of the dependencies that pop up is upb, it does not compiler with the incompatible flag
protocolbuffers/upb#584

Regarding bazelbuild/bazel, in this PR I tried to flip the flag bazelbuild/bazel#15327. The most interesting is that some issues appear only when flipping the flag, but not when running the tests wiht --incompatible_disallow_empty_glob.

Several could be fixed, just being explicit about allowing empty globs. I created a PR in bazelbuild/bazel#15330 but let's see what the Bazel team will say. In some it is not so clear if it should be allowed or the glob should be removed.

@cgrindel cgrindel added the bazel-flags Flip default for Bazel flags label Aug 9, 2022
@cgrindel cgrindel moved this to Needs Implementation in Rules SIG Tracker Aug 9, 2022
@limdor
Copy link

limdor commented Sep 28, 2022

Update: Bazel CI for the flip is green bazelbuild/bazel#15327

@limdor limdor mentioned this issue Sep 28, 2022
Open
@keith
Copy link
Member

keith commented Sep 28, 2022

Amazing work!

@cgrindel cgrindel moved this from Needs Assignment to In Progress in Rules SIG Tracker Sep 30, 2022
@cgrindel
Copy link
Member

cgrindel commented Oct 4, 2022

In rules SIG meeting, this was discussed. This may not be as green as it appears. Please see: https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1287.

May not be able to flip the flag until after Bazel 6.0.

@limdor
Copy link

limdor commented Oct 4, 2022

Yes, see discussion in bazelbuild/bazel#8195 (comment)

@limdor
Copy link

limdor commented Oct 20, 2022

Update: All PRs in Bazel have been merged except the flag flip itself. The remaining part is the downstream projects that are tracked here bazelbuild/bazel#15327

@alexeagle
Copy link
Contributor Author

@limdor from some discussion in the SIG meeting - is this going to mean that rule implementations and macros will now fail if they use native.glob and it returns no files? That's going to be painful to rollout - there's no buildozer command to edit macro code, and rulesets like TS (and Kotlin?) rely on using glob to detect whether files exist as part of their regular logic.

It seems to me this would work better if it were possible to apply only to the glob symbol that is in BUILD file scope, but not to the native.glob symbol that is used in .bzl files.

@limdor
Copy link

limdor commented Jan 10, 2023

It seems to me this would work better if it were possible to apply only to the glob symbol that is in BUILD file scope, but not to the native.glob symbol that is used in .bzl files.

Some Bazel developer would have to say how feasible that is and what it would require. I'm not aware if there is a way to differentiate between native.glob and a glob appearing in the BUILD file.

@limdor
Copy link

limdor commented Jan 10, 2023

and rulesets like TS (and Kotlin?) rely on using glob to detect whether files exist as part of their regular logic

For these cases allow_empty=True should be added, in the end is just about being explicit with the current behavior of Bazel

@alexeagle
Copy link
Contributor Author

@Wyverald says that it's theoretically possible to make two different glob symbols, though it's not that way today.

@limdor
Copy link

limdor commented Jan 10, 2023

Ok, assuming that it is possible. Why would we want to have a difference between glob and native.glob? AFAIK this is not happening with any other symbol that the native version is different. Do we have all relevant people in the Bazel team on board with this decision?

In the end the rules they would just have to adapt the code to have native.globa(...., allow_empty=True) instead of native.globa(....).

@alexeagle
Copy link
Contributor Author

Yeah our discussion was just that it's going to be a long, painful slog to identify all the locations where the allow_empty=True needs to be added across the whole ecosystem, and there's no tooling we can imagine to make it more trivial (e.g. the error message can't suggest the fix)

@limdor
Copy link

limdor commented Jan 10, 2023

You are totally right with that and I would like to change the approach. I would propose to all downstream projects to create a PR flipping the flag in the CI and fixing it until the CI is green. With this way we prevent regressions and at least we move forward instead of 2 steps forwards and 1 backwards.

@limdor
Copy link

limdor commented Jan 5, 2024

@alexeagle I would like to come to a SIG rules author meeting to discuss how to proceed. This flag has been there for a long time and as a community we should take a decision if we want to flip it or we just give up and decide that will never be flipped.

@limdor limdor mentioned this issue Jan 5, 2024
Closed
56 tasks
copybara-service bot pushed a commit to bazelbuild/bazel that referenced this issue Oct 7, 2024
@limdor @copybara-github
Flip disallow empty glob
538b119 
This PR is to flip the default value of `incompatible_disallow_empty_glob`. This flag is tracked in #8195 and it is available since Bazel 0.29 from August 2019.
Once this is merged, users still can allow empty globs without being explicit setting `--incompatible_disallow_empty_glob=false`.

The motivation of the flip is bazel-contrib/SIG-rules-authors#37 and the fact of not keeping an incompatible flag for 4 years without flipping it.

What needs to be done before flipping this:

- [x] Merge protocolbuffers/upb#584
- [x] Update upb to a version that has the change in protocolbuffers/upb#584
- [x] Merge protocolbuffers/upb#745
- [x] Bring the changes from protocolbuffers/upb#745 to the 21.x branch
- [x] Bring the changes from protocolbuffers/upb@e5f2601 to the 21.x branch (protocolbuffers/upb#781)
- [x] Update upb to the latest commit of the 21.x branch (#16343)
- [x] Merge #15374
- [x] Merge #15339
- [x] Merge #15330
- [x] Merge #16431
- [x] Merge #16468
- [x] Get green checks for this PR
- [ ] Fix failures in downstream projects: https://buildkite.com/bazel/bazelisk-plus-incompatible-flags
  - [ ] Fix empty globs from IntellIJ Plugin (https://github.com/bazelbuild/intellij)
    - [ ] Issue: bazelbuild/intellij#4040
    - [ ] Android Studio Plugin
    - [ ] Android Studio Plugin Google
    - [ ] CLion Plugin
    - [ ] CLion Plugin Google
    - [ ] IntelliJ Plugin
    - [ ] IntelliJ Plugin Aspect
    - [ ] IntelliJ Plugin Aspect Google
    - [ ] IntelliJ Plugin Google
    - [ ] IntelliJ UE Plugin
    - [ ] IntelliJ UE Plugin Google
    - Opened PRs
      - [x] bazelbuild/intellij#4025
      - [x] bazelbuild/intellij#4038
  - [x] Fix empty globs from Bazel Bench (https://github.com/bazelbuild/bazel-bench)
    - [ ] bazelbuild/bazel-bench#174
    - [x] bazelbuild/bazel-bench#149
  - [x] Fix empty globs from Buildfarm (https://github.com/bazelbuild/bazel-buildfarm)
  - [x] Fix empty globs from Cargo
  - [x] ~~Fix empty globs from Cartographer (https://github.com/cartographer-project/cartographer)~~ (No longer maintained)
    - Issue: cartographer-project/cartographer#1908
    - PR to enforce it cartographer-project/cartographer#1944
  - [x] Fix empty globs from Cloud Robotics Core (https://github.com/googlecloudrobotics/core/)
    - [x] PR to prevent regresions googlecloudrobotics/core#279
  - [ ] Fix empty globs from Envoy
  - [x] Fix empty globs from FlatBuffers
  - [x] Fix empty globs from Flogger
  - [ ] Fix empty globs from Gerrit (https://gerrit.googlesource.com/gerrit.git)
    - Issue: https://issues.gerritcodereview.com/issues/40011754
  - [x] Fix empty globs from :bazel:Protobuf
  - [x] Fix empty globs from rules_android_ndk
  - [ ] Fix empty globs from rules_dotnet
  - [x] Fix empty globs from :bazel:rules_foreign_cc bazel-contrib/rules_foreign_cc#974
  - [x] Fix empty globs from rules_go
  - [x] Fix empty globs from rules_haskell tweag/rules_haskell#1827
  - [x] Fix empty globs from :bazel:rules_jvm_external
  - [x] Fix empty globs from rules_kotlin
  - [x] Fix empty globs from rules_nodejs
  - [x] Fix empty globs from :bazel:rules_python
  - [ ] Fix empty globs from rules_rust
  - [x] Fix empty globs from rules_swift
  - [x] Fix empty globs from rules_webtesting
  - [x] Fix empty globs from TensorFlow  tensorflow/tensorflow#58155
    - [x] tensorflow/tensorflow#58008
    - [x] tensorflow/tensorflow#58154
  - [x] Fix empty globs from upb

RELNOTES[INC]: The flag `--incompatible_disallow_empty_glob` now defaults to true.

Closes #15327.

PiperOrigin-RevId: 683316885
Change-Id: Id34ffd32a9e979fada9207e475b826a508100ba5
@limdor
Copy link

limdor commented Oct 23, 2024

The flag has been flipped in master and it will be in Bazel 8!

@alexeagle
Copy link
Contributor Author

Amazing

@github-project-automation github-project-automation bot moved this from In Progress to Done in Rules SIG Tracker Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@limdor limdor

Labels
bazel-flags Flip default for Bazel flags bounty-1000USD A contributor who completes this will be rewarded $1000
Projects
Rules SIG Tracker
Status: Done
Milestone
No milestone
Development

No branches or pull requests
4 participants
@alexeagle @cgrindel @keith @limdor