Flip disallow empty glob #15327
Flip disallow empty glob #15327
Conversation
limdor
force-pushed
the
patch-4
branch
3 times, most recently
from
cbba6ed to
8f44b3a
Compare
|
Hello @limdor, Can you please check the above buildkite presubmit failures and resubmit again. Thanks! |
|
@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. |
|
I created another PR that prepare some other parts #15339 |
limdor
force-pushed
the
patch-4
branch
2 times, most recently
from
0122821 to
171beb0
Compare
limdor
force-pushed
the
patch-4
branch
3 times, most recently
from
83844e1 to
59dd251
Compare
limdor
force-pushed
the
patch-4
branch
2 times, most recently
from
4e4231b to
c69629f
Compare
|
@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. |
gtech-bazel-bot
bot
added
the
awaiting-review
limdor
force-pushed
the
patch-4
branch
2 times, most recently
from
154dfa9 to
7b26587
Compare
Same BUILD file is used with data-1 and without data-1. In the first bazel build data-1 is not there and the glob is empty. In the second one, data-1 is there and it is not empty.
|
@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 |
|
Nice, all checks are passing. I'll retry the import. Thanks! |
|
Thanks for the comment @fmeum. With that information was easy to fix. |
|
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 So I'm probably going to do some hybrid thing, where I partially migrate Google off |
|
@Wyverald do you have any updates on this? |
|
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). |
|
Really looking forward to this being flipped. |
This bazel flag, that should be flipped in an upcoming release bazelbuild/bazel#15327, fails if globs have no matches. This helps find libraries where you are accidentally not including files because of typos. This change removes the various globs that were not matching anything, and uncovered some targets that were doing nothing because their source files were deleted. There are a few cases where globs were intentionally optional in the case of loops that expanded to different potential options, so those now use `allow_empty = True`. This allows downstream consumers to also flip this flags for their own builds, where previously this would fail in LLVM instead. The downside to this change is that if files are added in these relatively standard locations, manual work will have to be done to add this patterns back. If folks prefer we could instead add `allow_empty = True` to every glob.
This bazel flag, that should be flipped in an upcoming release bazelbuild/bazel#15327, fails if globs have no matches. This helps find libraries where you are accidentally not including files because of typos. This change removes the various globs that were not matching anything, and uncovered some targets that were doing nothing because their source files were deleted. There are a few cases where globs were intentionally optional in the case of loops that expanded to different potential options, so those now use `allow_empty = True`. This allows downstream consumers to also flip this flags for their own builds, where previously this would fail in LLVM instead. The downside to this change is that if files are added in these relatively standard locations, manual work will have to be done to add this patterns back. If folks prefer we could instead add `allow_empty = True` to every glob.
This bazel flag, that should be flipped in an upcoming release bazelbuild/bazel#15327, fails if globs have no matches. This helps find libraries where you are accidentally not including files because of typos. This change removes the various globs that were not matching anything, and uncovered some targets that were doing nothing because their source files were deleted. There are a few cases where globs were intentionally optional in the case of loops that expanded to different potential options, so those now use `allow_empty = True`. This allows downstream consumers to also flip this flags for their own builds, where previously this would fail in LLVM instead. The downside to this change is that if files are added in these relatively standard locations, manual work will have to be done to add this patterns back. If folks prefer we could instead add `allow_empty = True` to every glob.
This bazel flag, that should be flipped in an upcoming release bazelbuild/bazel#15327, fails if globs have no matches. This helps find libraries where you are accidentally not including files because of typos. This change removes the various globs that were not matching anything, and uncovered some targets that were doing nothing because their source files were deleted. There are a few cases where globs were intentionally optional in the case of loops that expanded to different potential options, so those now use `allow_empty = True`. This allows downstream consumers to also flip this flags for their own builds, where previously this would fail in LLVM instead. The downside to this change is that if files are added in these relatively standard locations, manual work will have to be done to add this patterns back. If folks prefer we could instead add `allow_empty = True` to every glob.
This bazel flag, that should be flipped in an upcoming release bazelbuild/bazel#15327, fails if globs have no matches. This helps find libraries where you are accidentally not including files because of typos. This change removes the various globs that were not matching anything, and uncovered some targets that were doing nothing because their source files were deleted. There are a few cases where globs were intentionally optional in the case of loops that expanded to different potential options, so those now use `allow_empty = True`. This allows downstream consumers to also flip this flags for their own builds, where previously this would fail in LLVM instead. The downside to this change is that if files are added in these relatively standard locations, manual work will have to be done to add this patterns back. If folks prefer we could instead add `allow_empty = True` to every glob.
|
@Wyverald do you have some update on this topic? |
|
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! |
|
@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 |
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 |
|
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 |
|
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". 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. |
In order to flip the flag, all downstream projects should be adapted. However, it is hard to fix them all if there are constant regressions. Adding it to the CI will ensure that once the project can build with incompatible_disallow_empty_glob it can keep building like that. See: bazelbuild/bazel#15327
|
@Wyverald do you have any updates on this topic? Were you able to make some progress? |
|
Sorry, I didn't end up having time... You know, I might just do the copybara transform thing today. Stay tuned... |
|
FYI -- I have the copybara CL ready, but rules_cc has slipped (https://github.com/bazelbuild/rules_cc/blob/main/cc/private/BUILD) which is preventing submit. Will try to get a new release of rules_cc after bazelbuild/rules_cc#250 is in. But this is looking ok to go in before 8.0 cut. |
github-actions
bot
removed
the
awaiting-PR-merge
|
Uou, it just happened! Thanks a lot @Wyverald for all your support from the Bazel team side! |
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:
Fix empty globs from Cartographer (https://github.com/cartographer-project/cartographer)(No longer maintained)