bazelbuild / bazel-skylib

Common useful functions and rules for Bazel
https://bazel.build/
Apache License 2.0
394 stars 180 forks source link

Unexpected visibility limitations with config_setting visibility #404

Open keith opened 2 years ago

keith commented 2 years ago

As of https://github.com/bazelbuild/bazel/issues/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 commented 2 years ago

cc @gregestren

gregestren commented 1 year ago

Interestingly adding --incompatible_config_setting_private_default_visibility fixes this.

keith commented 1 year ago

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

gregestren commented 1 year ago

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

gregestren commented 1 year ago

Looks like some recent progress on flipping --incompatible_config_setting_private_default_visibility: https://github.com/bazelbuild/bazel/issues/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?