incompatible_config_setting_private_default_visibility

[gregestren]

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 (https://github.com/bazelbuild/bazel/issues/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, every config_setting without an explicit visibility 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_settings 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.

[meteorcloudy]

I assume this flag is migration ready? Should we flip it before 6.0 cut?

[gregestren]

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 https://github.com/bazelbuild/bazel/issues/12932. If we want to be more careful I'd just flip https://github.com/bazelbuild/bazel/issues/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 config_setting. So if a repo has config_setting, visibility = '//visibility:private'), today it's actually public and may be depended on by targets in other packages. If we flip these flags then suddenly we give that declaration meaning and, while technically correct, existing dependencies may no longer be valid.

So it's definitely a strict improvement but we have to get mislabeled repo code fixed up for it to finally stick.

[meteorcloudy]

I'm hoping the answer is "flip both and adjust whatever breakages we see."

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

[meteorcloudy]

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!

[gregestren]

Great! What's my deadline for fixing Envoy and TensorFlow and flipping --incompatible_enforce_config_setting_visibility? That's the more important one to set first.

[meteorcloudy]

6.0 is currently scheduled to be cut on Sep 26, see https://github.com/bazelbuild/bazel/issues/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 ;)

[meteorcloudy]

Bumping to P1 since it's blocking 6.0 release

[lberki]

Bumping this down to P2 to indicate our collective desire to get this into Bazel 6.0, but also that we won't postpone the release cut for this, should this and other P2s be the only things that remain.

[meteorcloudy]

Flipping this flag is breaking downstream projects: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2678#_

@gregestren Please check https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1297 before flipping the flag

I'm rolling back the flag flip now

[meteorcloudy]

@gregestren Can you please make sure the incompatible pipeline says this flag is ready to flip before actually flipping it? This is causing a very red downstream: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/2717#_

We'll have to rollback the flag flip. @SalmaSamy

[gregestren]

Yep, incompatible pipeline still failing: https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1320

• The bazel-skylib failure is because bazel-skylib uses rules_go release 0.33. That predates the rules_go fixes that resolve this error. Note that even the latest release 0.35 is too old.
• The Bazel failure is because of an outdated dep on @rules_pkg//pkg/private:private_stamp_detect, which it says is not visible. That was fixed in rules_pkg in https://github.com/bazelbuild/rules_pkg/commit/7693abc8ca63612945fe8f6de85889be5fcbe457 15 days ago. I'm not sure why that's not pulled in: which version of rules_pkg does Bazel pull in?

[meteorcloudy]

Re rules_pkg version in Bazel: https://github.com/bazelbuild/bazel/blob/master/distdir_deps.bzl#L300-L309 /cc @aiuto Looks like we just had 0.8.0 release yesterday

[aiuto]

Yes. I did an 0.8.0 yesterday to unblock my development work on rules_license. I did not update Bazel to it. I'll do a PR for that. Greg: I suppose you want to backport that to the 6.0 RC branch.

[meteorcloudy]

Greg: I suppose you want to backport that to the 6.0 RC branch.

Maybe not? If we are not cherry picking the flip of this flag in 6.0.

[gregestren]

At this point I'd push this through best-effort (whatever version it lands in). There are other outdated dependencies and it's not vital to be specifically in 6.0.

[aiuto]

As much as I would like to see it in 6.x, I keep seeing rippling churn of updating dependencies. rules_pkg was merely the piece that the Bazel team saw. I can easily imagine that kind of problem lurking in any rule set that needs to define a config setting that others reuse.

On Fri, Nov 4, 2022 at 12:29 PM Greg @.***> wrote:

At this point I'd push this through best-effort (whatever version it lands in). There are other outdated dependencies and it's not vital to be specifically in 6.0.

— Reply to this email directly, view it on GitHub https://github.com/bazelbuild/bazel/issues/12933#issuecomment-1303840389, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXHHHFG5IJPX5LLFBJH6IDWGU2VBANCNFSM4WZUY76A . You are receiving this because you were mentioned.Message ID: @.***>

[keertk]

For tracking: cc @meteorcloudy

Failures: https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1341

To fix:

• [x] Bazel Remote Cache: https://github.com/buchgr/bazel-remote/issues/609
• [x] Bazel skylib: https://github.com/bazelbuild/bazel-skylib/issues/414
• [x] Bazel watcher: https://github.com/bazelbuild/bazel-watcher/issues/565
• [x] Bazelisk: https://github.com/bazelbuild/bazelisk/issues/398
• [ ] Buildfarm: https://github.com/bazelbuild/bazel-buildfarm/issues/1221
• [ ] Buildtools: https://github.com/bazelbuild/buildtools/issues/1102
• [x] rules_dotnet: https://github.com/bazelbuild/rules_dotnet/issues/333
• [x] Cloud Robotics Core: https://github.com/googlecloudrobotics/core/issues/94
• [x] Envoy: https://github.com/envoyproxy/envoy/issues/24183
• [x] FlatBuffers: https://github.com/google/flatbuffers/issues/7664
• [x] Kythe: https://github.com/kythe/kythe/issues/5453
• [x] rules_cc: https://github.com/bazelbuild/rules_cc/issues/157
• [x] rules_go: https://github.com/bazelbuild/rules_go/issues/3386
• [ ] rules_jsonnet: https://github.com/bazelbuild/rules_jsonnet/issues/163
• [ ] rules_kotlin: https://github.com/bazelbuild/rules_kotlin/issues/928
• [x] rules_nodejs: https://github.com/bazelbuild/rules_nodejs/issues/3603
• [ ] rules_webtesting: https://github.com/bazelbuild/rules_webtesting/issues/446
• [ ] upb: https://github.com/protocolbuffers/protobuf/issues/13749
• [x] rules_python: https://github.com/bazelbuild/rules_python/issues/1237
• [x] rules_swift: https://github.com/bazelbuild/rules_swift/issues/1052

[gregestren]

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 migration-ready already cover it?

[keertk]

Hey @gregestren, just created/added breaking-change-7.0. Thanks!