Not all rules support visibility

keith commented 3 months ago

It seems like with_cfg assumes that all macros passed supports kwargs / manually visibility, but I found a case that doesn't with rules_uv:

load("@rules_uv//uv:pip.bzl", "pip_compile")
load("@with_cfg.bzl", "with_cfg")

pip_compile_min_py, _pip_compile_min_py_internal = with_cfg(pip_compile).set(
    Label("@rules_python//python/config_settings:python_version"),
    "3.8",
).build()

ERROR: package contains errors: utils/build: Traceback (most recent call last):
        File "/home/ubuntu/modular/utils/build/BUILD.bazel", line 14, column 19, in <toplevel>
                pip_compile_min_py(
        File "/home/ubuntu/.cache/bazel/_bazel_ubuntu/a8246705211370f9a29d0c6e94adcf5a/external/with_cfg.bzl~/with_cfg/private/wrapper.bzl", line 18, column 46, in lambda
                return lambda *, name, **kwargs: _wrapper(
        File "/home/ubuntu/.cache/bazel/_bazel_ubuntu/a8246705211370f9a29d0c6e94adcf5a/external/with_cfg.bzl~/with_cfg/private/wrapper.bzl", line 143, column 19, in _wrapper
                rule_info.kind(
        File "/home/ubuntu/.cache/bazel/_bazel_ubuntu/a8246705211370f9a29d0c6e94adcf5a/external/rules_uv~/uv/private/pip.bzl", line 111, column 5, in pip_compile
                def pip_compile(
Error: pip_compile() got unexpected keyword argument: visibility
ERROR: Evaluation of query "utils/build:all" failed

https://github.com/theoremlp/rules_uv/blob/ce66cb575dd67072f8de70d02c46131bad55b994/uv/private/pip.bzl#L111-L119

keith commented 3 months ago

seems like this case is actually more complicated, if i make it support visibility it fails with an args issue:

ERROR: /home/ubuntu/modular/utils/build/BUILD.bazel:14:19: //utils/build:generate_requirements_txt: no such attribute 'args' in 'alias' rule
ERROR: /home/ubuntu/modular/utils/build/BUILD.bazel:14:19: //utils/build:generate_requirements_txt: no such attribute 'data' in 'alias' rule

and it's also no longer runnable:

ERROR: Cannot run target //utils/build:generate_requirements_txt_with_cfg: Not executable

fmeum commented 3 months ago

I don't see a clear solution to the visibility issues as it isn't possible to know whether a given rule or macro supports visibility. I could add a knob for this if needed, but would prefer upstream just adding support for it - it's good hygiene for a macro to support it.

Regarding args, I only recently added somewhat complex logic to handle locations in args and it's pretty likely I messed something up. I'll investigate and push a fix.

fmeum commented 3 months ago

I drafted https://github.com/fmeum/with_cfg.bzl/pull/108 based on your example and can't reproduce the args issue, even if I remove executable = True. What did you do to get it?

The remaining problems:

The bazel run command emitted into the generated file is the label of the untransitioned target. I don't see a way to fix this.
The implicit *_diff_test target is not forwarded as a test. It probably makes sense to extend the implicit_targets parameter to cover this case.

keith commented 3 months ago

when I just added visibility and not **kwargs to that macro is when I hit the args issue, so with the patch upstream I imagine that sidesteps it

fmeum commented 3 months ago

when I just added visibility and not **kwargs to that macro is when I hit the args issue, so with the patch upstream I imagine that sidesteps it

Thanks, I sent https://github.com/fmeum/with_cfg.bzl/pull/110 to fix this part.

fmeum / with_cfg.bzl

Not all rules support visibility #107