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.

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

Flip default for --incompatible_disallow_empty_glob #37

Open
alexeagle opened this issue Mar 22, 2022 · 14 comments
Open

Flip default for --incompatible_disallow_empty_glob #37

alexeagle opened this issue Mar 22, 2022 · 14 comments
Assignees
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
@limdor
Copy link

limdor commented Sep 28, 2022

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

@keith
Copy link
Member

keith commented Sep 28, 2022

Amazing work!

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel-flags Flip default for Bazel flags bounty-1000USD A contributor who completes this will be rewarded $1000
Projects
Status: In Progress
Development

No branches or pull requests

4 participants