Flip disallow empty glob

[limdor]

What needs to be done before flipping this:

• [x] Merge https://github.com/protocolbuffers/upb/pull/584
• [x] Update upb to a version that has the change in https://github.com/protocolbuffers/upb/pull/584
• [x] Merge https://github.com/protocolbuffers/upb/pull/745
• [x] Bring the changes from https://github.com/protocolbuffers/upb/pull/745 to the 21.x branch
• [ ] Bring the changes from https://github.com/protocolbuffers/upb/commit/e5f26018368b11aab672e8e8bb76513f3620c579 to the 21.x branch (https://github.com/protocolbuffers/upb/pull/781)
• [ ] Update upb to the latest commit of the 21.x branch
• [ ] Merge https://github.com/bazelbuild/bazel/pull/15374
• [x] Merge https://github.com/bazelbuild/bazel/pull/15339
• [x] Merge https://github.com/bazelbuild/bazel/pull/15330

[sgowroji]

Hello @limdor, Can you please check the above buildkite presubmit failures and resubmit again. Thanks!

[limdor]

@sgowroji it is not straightforward and hard to flip and fix everything at once. I created a pr that all checks are passing that move some of the changes there. https://github.com/bazelbuild/bazel/pull/15330 This one here is not ready to review. I've marked it as draft but feel free to put any labels to indicate that. Creating the PR was the only way I knew to run the presubmit checks.

[limdor]

I created another PR that prepare some other parts https://github.com/bazelbuild/bazel/pull/15339

[limdor]

@sgowroji this is ready and the checks are passing. This could be taken as is or first the mentions PRs in the description could be merged.

[aiuto]

This is going to break a lot of end user rules. We may need more visibility about it, And different approvers

[limdor]

This is going to break a lot of end user rules. We may need more visibility about it, And different approvers

There was the discussion to fix first the downstream projects and then flip it. However the current problem is that there are regressions all the time. The only way would be to fix them but add the enforcement in the corresponding CI. Even Bazel itself has regressions regarding this topic. I this should be fixed with https://github.com/bazelbuild/bazel/pull/16670

[aiuto]

It was not mostly the downstream projects. Someone in the Rules SIG pointed out that user projects will break all over the place.

I still think we should do this, and do it sooner in the 7.x dev cycle rather than later. We should, however, have very clear messaging that this is happening.

On Tue, Jan 10, 2023 at 1:14 PM Xavier Bonaventura @.***> wrote:

This is going to break a lot of end user rules. We may need more visibility about it, And different approvers

There was the discussion to fix first the downstream projects and then flip it. However the current problem is that there are regressions all the time. The only way would be to fix them but add the enforcement in the corresponding CI. Even Bazel itself has regressions regarding this topic. I this should be fixed with #16670 https://github.com/bazelbuild/bazel/pull/16670

— Reply to this email directly, view it on GitHub https://github.com/bazelbuild/bazel/pull/15327#issuecomment-1377661708, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXHHHAGKAWTUND2MFSDFFDWRWRIRANCNFSM5UGG2O6Q . You are receiving this because you commented.Message ID: @.***>

[ted-xie]

Any other feedback needed regarding Android? I noticed @ahumesky and I are still added as reviewers.

[limdor]

Not from my side at least

[limdor]

We should, however, have very clear messaging that this is happening.

@aiuto Any proposals on how to make it clear? For the moment I started creating PRs to the downstream projects to make the flip in advance in the corresponding bazelrc files.

[alexeagle]

Discussed in the Rules Authors SIG meeting today, we all agree that it's time to flip this at HEAD so that Bazel 8 will have this change. There's plenty of time for the ecosystem to work through remaining violations before most users will encounter it.

[Wyverald]

Hey @limdor, we attempted internal merge but there were some test failures whose fixes were not immediately clear to me. I'm unfortunately quite swamped right now, so if you could find some time to sync this PR and fix the test failures, that'd be great! Otherwise, I'll probably get to this some time later.

[limdor]

@Wyverald then I will start rebasing this PR and seeing what is the CI saying about it

[limdor]

@Wyverald several tests seem to have an issue with rules_cc. They complain that https://github.com/bazelbuild/rules_cc/blob/main/cc/private/rules_impl/BUILD#L7 does not glob anything. I don't get why,

For another I added a separate commit to fix it https://github.com/bazelbuild/bazel/pull/15327/commits/5d2cea66f00bd8f45f612649f72b519868e9ac61

[fmeum]

@limdor That's because Bazel's Java tests don't see all of rules_cc, only the files listed here: https://cs.opensource.google/bazel/bazel/+/master:src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java;l=278?q=rules_impl&sq=&ss=bazel%2Fbazel

You could try adding at least one .bzl file to that list.

[Wyverald]

Nice, all checks are passing. I'll retry the import. Thanks!

[limdor]

Thanks for the comment @fmeum. With that information was easy to fix.

[Wyverald]

Status update: I spent some time trying to import this into Google, but it's surprisingly hard to flip the flag for Bazel only and not Blaze. (In slightly more detail: we have a .blazerc file in Google, which I have explicitly set to allow empty globs. But we have a plethora of integration test setups that all invoke Blaze in some sort of bespoke manner that does not include reading the .blazerc file. I quickly realized that it is not worth my time at all to make sure each of these invocations specify --noincompatible_disallow_empty_glob.)

So I'm probably going to do some hybrid thing, where I partially migrate Google off allow_empty_glob, at least the parts that are used by the Blaze integration tests. The rest we can do some other day.

[limdor]

@Wyverald do you have any updates on this?

[Wyverald]

sorry, no. I was whisked away by 7.1.0 work. Will have to come back to this after 7.1.0 is out (hopefully next week).

[brentleyjones]

Really looking forward to this being flipped.

[limdor]

@Wyverald do you have some update on this topic?

[Wyverald]

Not yet. Turns out I'm quite busy for 7.2.0, too 😅 but I promise I'll do this in time for 8.0!

[alexeagle]

@Wyverald I'm curious if your solution for this could be in the shape "ensure that all Blaze users are using the Blaze-team-provided <system>/.blazerc file" so that this is the last time we have such a painful flag flip. In the future it should always be "Bazel can have the reasonable default because Blaze easily chooses not to switch at that time"

[Wyverald]

@Wyverald I'm curious if your solution for this could be in the shape "ensure that all Blaze users are using the Blaze-team-provided <system>/.blazerc file" so that this is the last time we have such a painful flag flip. In the future it should always be "Bazel can have the reasonable default because Blaze easily chooses not to switch at that time"

It's less about the human users of Blaze, but more about the integration tests we have. It's probably not even accurate to call them "integration tests" as they don't actually invoke Blaze as a whole; they pull out parts of Skyframe, inject different random things, (crucially) uses source files pulled directly from Google's monorepo for certain @bazel_tools-y things. And because they're not really integration tests, they don't read any blazerc files (they don't even exercise the code paths that parse RC files).

[limdor]

Hahahaha I know what you are talking about, this is the type of issues I was hitting with the public repository and now you have the same with the internal only ones

[alexeagle]

Yeah the "public API" has leaked to the point that there's nowhere you can enforce that "usage of library code under this package assures that you also pick up the google3-wide configuration". Of course this is terrible news for Bazel as it means we'll continue to have dozens of critical flags with the wrong default value.

I have another proposal. You could (ab)use the copybara transform to simply have the flag default value differ between Piper and GitHub. This gives you a similar semantic "google opts for a different global setting" but since it lives in the source file where the flag is applied it's guaranteed to be seen by all the callsites you're hitting.