Skip to content
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

Unexpected visibility limitations with config_setting visibility #404

Open
keith opened this issue Oct 26, 2022 · 5 comments
Open

Unexpected visibility limitations with config_setting visibility #404

keith opened this issue Oct 26, 2022 · 5 comments

Comments

@keith
Copy link
Member

keith commented Oct 26, 2022

As of bazelbuild/bazel#12932, using selects.config_setting_group produces unexpected results because of how it uses an underlying alias to surface the settings. For example if you have:

load("@bazel_skylib//lib:selects.bzl", "selects")

config_setting(
    name = "dbg_build",
    values = {"compilation_mode": "dbg"},
    visibility = ["//visibility:public"],
)

config_setting(
    name = "some_define",
    values = {"define": "some_define=true"},
    visibility = ["//visibility:private"],
)

selects.config_setting_group(
    name = "public_setting",
    match_any = [
        ":dbg_build",
        ":some_define",
    ],
    visibility = ["//visibility:public"],
)

And from another package you depend on this with:

cc_library(
    name = "foo",
    srcs = ["foo.cc"] + select({
        "//:public_setting": [],
        "//conditions:default": [],
    }),
)

You see this error:

ERROR: /private/tmp/config_setting_repro/package/BUILD:1:11: in cc_library rule //package:foo: target '//:some_define' is not visible from target '//package:foo'. Check the visibility declaration of the former target if you think the dependency is legitimate
ERROR: /private/tmp/config_setting_repro/package/BUILD:1:11: Analysis of target '//package:foo' failed
ERROR: Analysis of target '//package:foo' failed; build aborted:
INFO: Elapsed time: 0.059s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 3 targets configured)

The macro generates:

# /private/tmp/config_setting_repro/BUILD:3:15
config_setting(
    name = "dbg_build",
    values = {"compilation_mode": "dbg"},
    visibility = ["//visibility:public"],
)
# Rule dbg_build instantiated at (most recent call last):
#   /private/tmp/config_setting_repro/BUILD:3:15 in <toplevel>

# /private/tmp/config_setting_repro/BUILD:15:29
alias(
    name = "public_setting",
    actual = select({
        "//:dbg_build": "//:dbg_build",
        "//conditions:default": "//:some_define",
    }),
    generator_function = "_config_setting_group",
    generator_location = "/private/tmp/config_setting_repro/BUILD:15:29",
    generator_name = "public_setting",
    visibility = ["//visibility:public"],
)
# Rule public_setting instantiated at (most recent call last):
#   /private/tmp/config_setting_repro/BUILD:15:29                                                                 in <toplevel>
#   /private/var/tmp/_bazel_ksmiley/e6a33c534f9680bff2cf4f056c41ed7e/external/bazel_skylib/lib/selects.bzl:125:33 in _config_setting_group
#   /private/var/tmp/_bazel_ksmiley/e6a33c534f9680bff2cf4f056c41ed7e/external/bazel_skylib/lib/selects.bzl:178:21 in _config_setting_or_group

# /private/tmp/config_setting_repro/BUILD:9:15
config_setting(
    name = "some_define",
    values = {"define": "some_define=true"},
    visibility = ["//visibility:private"],
)
# Rule some_define instantiated at (most recent call last):
#   /private/tmp/config_setting_repro/BUILD:9:15 in <toplevel>

So this makes sense, since aliases propagate visibility, but I think it's a bit unexpected in this case. Maybe there's another solution to how this could be written to allow having private settings.

@keith
Copy link
Member Author

keith commented Oct 26, 2022

cc @gregestren

keith added a commit to keith/envoy that referenced this issue Oct 27, 2022
Fixes envoyproxy#23390

This is an edge case, upstream issue: bazelbuild/bazel-skylib#404

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
keith added a commit to envoyproxy/envoy that referenced this issue Oct 28, 2022
Fixes #23390

This is an edge case, upstream issue: bazelbuild/bazel-skylib#404

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@gregestren
Copy link
Contributor

Interestingly adding --incompatible_config_setting_private_default_visibility fixes this.

@keith
Copy link
Member Author

keith commented Nov 7, 2022

shouldn't that be the same behavior as what we're getting with the default in this case?

@gregestren
Copy link
Contributor

https://github.com/bazelbuild/bazel/blob/9ba1adab83703ca756ff05d1298e511dc8e09320/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java#L1751-L1760 shows the intention.

So --incompatible_enforce_config_setting_visibility=1 --incompatible_config_setting_private_default_visibility=0 invokes non-standard that skips the alias' independent visibility setting in the interest of an easier migration. Which obviously doesn't serve this case.

Unfortunately it's proving cumbersome to also land --incompatible_config_setting_private_default_visibility=1 for Bazel 6.0, which would resolve this. I haven't considered that a release-blocking problem. If you do we could adjust priority. Landing --incompatible_config_setting_private_default_visibility=1 is just a matter of getting new releases on various downstream projects in Bazel's CI (like rules_go).

jwendell pushed a commit to jwendell/envoy that referenced this issue Dec 22, 2022
Fixes envoyproxy#23390

This is an edge case, upstream issue: bazelbuild/bazel-skylib#404

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Jonh Wendell <jwendell@redhat.com>
@gregestren
Copy link
Contributor

Looks like some recent progress on flipping --incompatible_config_setting_private_default_visibility: bazelbuild/bazel#12933.

I think it's marked for Bazel 7.0? (but no Github tags confirm that). If so, I think we can just sit back and wait. This doesn't help Bazel 6.0 users though. Maybe enough downstream projects will be updated that 6.0 users can just manually trigger --incompatible_config_setting_private_default_visibility?

@gregestren gregestren removed their assignment Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants