-
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
incompatible_config_setting_private_default_visibility #12933
Comments
This was rolled back in 36d228b because of depot breakages. This version adds incompatible flags to safely introduce enforcement. See #12932 and #12933 for flag settings. *** Original change description *** Roll back #12877. *** Fixes #12669. RELNOTES: enforce config_setting visibility. See #12932 for details. PiperOrigin-RevId: 354930807
This was rolled back in bazelbuild@36d228b because of depot breakages. This version adds incompatible flags to safely introduce enforcement. See bazelbuild#12932 and bazelbuild#12933 for flag settings. *** Original change description *** Roll back bazelbuild#12877. *** Fixes bazelbuild#12669. RELNOTES: enforce config_setting visibility. See bazelbuild#12932 for details. PiperOrigin-RevId: 354930807
This was rolled back in 36d228b because of depot breakages. This version adds incompatible flags to safely introduce enforcement. See #12932 and #12933 for flag settings. *** Original change description *** Roll back #12877. *** Fixes #12669. RELNOTES: enforce config_setting visibility. See #12932 for details. PiperOrigin-RevId: 354930807
I assume this flag is migration ready? Should we flip it before 6.0 cut? |
What was the risk calculation for flipping? I can't guarantee repos won't break. If that's okay, I support pushing this forward. More specifically, if we're not worried about initial breakages, I'd flip both this and #12932. If we want to be more careful I'd just flip #12932. If we don't want any change of any breakages I wouldn't flip. I'm hoping the answer is "flip both and adjust whatever breakages we see." The reason this isn't trivial is repos can already set visibility on So it's definitely a strict improvement but we have to get mislabeled repo code fixed up for it to finally stick. |
We should do it in the reverse order to make sure downstream is green, otherwise we'll lose the ability to catch other CI breakages. I recently updated our incompatible change process: https://bazel.build/contribute/breaking-changes#update-repos With the incompatible flags pipeline, we can detect those breakages before flipping the flag, check this doc I just added the "migration-ready" label for both flag, fixed the issue name and triggered a rerun, you can see the result here https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1256 |
The result looks good. You can ignore the rules_nodejs failure. But except that, --incompatible_config_setting_private_default_visibility didn't break any downstream project, so you can flip it. --incompatible_enforce_config_setting_visibility is breaking Envoy and TensorFlow, if you can file issue or send PR to fix them before flipping the flag, it will be great! |
Great! What's my deadline for fixing Envoy and TensorFlow and flipping |
6.0 is currently scheduled to be cut on Sep 26, see #16159 But there are still many release blockers, see https://github.com/bazelbuild/bazel/issues?q=is%3Aopen+is%3Aissue+milestone%3A%226.0.0+release+blockers%22, and it's likely the cut day will be postponed again. We can add the milestone to both tracking issues, so we won't forget ;) |
Bumping to P1 since it's blocking 6.0 release |
…bazelrc to prevent regression Imported from GitHub PR tensorflow/tensorflow#58832 Related: bazelbuild/bazel#12932 and bazelbuild/bazel#12933 Copybara import of the project: -- b4edfd47bd9a0c0363a9feb1eb1b48d067e4644e by Yun Peng <[email protected]>: Flip two incompatible flags in .bazelrc to prevent regression -- c33c0dd6e5744393fe2b1909efc91dc19bd6dda1 by Yun Peng <[email protected]>: Add visibility to more targets -- 34d1bfe1564f0905993c26a891f2385b83480e3d by Yun Peng <[email protected]>: --incompatible_config_setting_private_default_visibility cannot be flipped yet Merging this change closes #58832 PiperOrigin-RevId: 494895621
…bazelrc to prevent regression Imported from GitHub PR tensorflow/tensorflow#58832 Related: bazelbuild/bazel#12932 and bazelbuild/bazel#12933 Copybara import of the project: -- b4edfd47bd9a0c0363a9feb1eb1b48d067e4644e by Yun Peng <[email protected]>: Flip two incompatible flags in .bazelrc to prevent regression -- c33c0dd6e5744393fe2b1909efc91dc19bd6dda1 by Yun Peng <[email protected]>: Add visibility to more targets -- 34d1bfe1564f0905993c26a891f2385b83480e3d by Yun Peng <[email protected]>: --incompatible_config_setting_private_default_visibility cannot be flipped yet Merging this change closes #58832 PiperOrigin-RevId: 494895621
Thanks for recent updates, @keertk . I'm excited to see you making this 7.0 compatible. Is there an appropriate 7.0 Github label for this issue? Or does |
Hey @gregestren, just created/added |
…g_private_default_visibility to true This was rolled back in bazelbuild@d4fc6db due to failing Bazel downstream tests. I fixed as many as I was aware of: see latest comments on bazelbuild#12933. Fixes bazelbuild#12933. PiperOrigin-RevId: 485694298 Change-Id: Ie38ce2f6cdbf952a208de37a96658770004b2bf8
*** Reason for rollback *** Incompatible pipeline still showing failures: https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1320 Need to investigate why some fixes @Head seem not to work. *** Original change description *** Roll forward bazelbuild@1fdc628: default --incompatible_config_setting_private_default_visibility to true This was rolled back in bazelbuild@d4fc6db due to failing Bazel downstream tests. I fixed as many as I was aware of: see latest comments on bazelbuild#12933. Fixes bazelbuild#12933. PiperOrigin-RevId: 485901094 Change-Id: Ifed7424c482d4feda76811838209e3e10607471b
Visibility on
config_setting
isn't historically enforced. This is purely for legacy reasons. There's no philosophical reason to distinguish them.This flag, in conjunction with
--incompatible_enforce_config_setting_visibility
(#12932), removes that distinction.Values:
--incompatible_config_setting_private_default_visibility=off
: if--incompatible_enforce_config_setting_visibility=off
, every config_setting is visible to every target, regardless of visibility settings. Else, everyconfig_setting
without an explicitvisibility
setting is//visibility:public
(ignoring package visibility defaults)--incompatible_config_setting_private_default_visibility=on
: if--incompatible_enforce_config_setting_visibility=off
, every config_setting is visible to every target, regardless of visibility settings. Else,config_setting
follows the same visibility rules as all other targets.Incompatibility error:
ERROR: myapp/BUILD:4:1: in config_setting rule //myapp:my_config: target 'myapp:my_config' is not visible from target '//some:other_target. Check the visibility declaration of the former target if you think the dependency is legitimate
Migration:
Treat all
config_setting
s as if they follow standard visibility logic at https://docs.bazel.build/versions/master/visibility.html: have them set visibility explicitly if they'll be used anywhere outside their own package. The ultimate goal of this migration is to fully enforce that expectation.The text was updated successfully, but these errors were encountered: