bazelbuild/bazel

incompatible_config_setting_private_default_visibility

gregestren opened this issue · 19 comments

gregestren commented

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, 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 commented

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

gregestren commented

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 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 commented

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 commented

image

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 commented

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 commented

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 ;)

meteorcloudy commented

Bumping to P1 since it's blocking 6.0 release

lberki commented

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 commented

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 commented

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

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 bazelbuild/rules_pkg@7693abc 15 days ago. I'm not sure why that's not pulled in: which version of rules_pkg does Bazel pull in?
meteorcloudy commented

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 commented

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 commented

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 commented

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 commented
gregestren commented

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 commented

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