Error reporting for select-based target_compatible_with expressions is too vague

armandomontanez commented 4 months ago

Description of the bug:

Due to limitations with config_setting rules used in target_compatible_with (also see https://github.com/bazelbuild/bazel/issues/12614), I use a lot of select-based target_compatible_with expressions. Unfortunately, this usually means the error messages that are presented are inscrutable:

    @@pico-sdk~1.6.0-rc1~_repo_rules~btstack//:pico_btstack_base (d6a49d)   <-- target platform (@@pico-sdk~1.6.0-rc1//bazel/platform:rp2040) didn't satisfy constraint @@platforms//:incompatible

The actual config_setting branch that is evaluated here is named nicely: "@pico-sdk//bazel/constraint:pico_btstack_config_unset", but because of how the select is evaluated, that pathway never gets surfaced. The technical limitation here is pretty obvious, but perhaps we can reorient this problem a bit and consider ways to allow developers to provide friendly error messages here? I'd love to get something like this:

ERROR: Analysis of target '//examples:hello_world_pico' failed; build aborted: Target //examples:hello_world_pico is incompatible and cannot be built, but was explicitly requested.
Dependency chain:
    //examples:hello_world_pico (9d7924)
    //examples:hello_world_pico.elf (d6a49d)
    @@pico-sdk~1.6.0-rc1//src/rp2_common/pico_cyw43_arch:pico_cyw43_arch_none (d6a49d)
    @@pico-sdk~1.6.0-rc1//src/rp2_common/pico_cyw43_arch:pico_cyw43_arch_threadsafe_background (d6a49d)
    @@pico-sdk~1.6.0-rc1//src/rp2_common/pico_cyw43_driver:pico_cyw43_driver (d6a49d)
    @@pico-sdk~1.6.0-rc1//src/rp2_common/pico_btstack:btstack_run_loop_async_context (d6a49d)
    @@pico-sdk~1.6.0-rc1~_repo_rules~btstack//:pico_btstack_base (d6a49d)   <-- target platform (@@pico-sdk~1.6.0-rc1//bazel/platform:rp2040) didn't satisfy constraint @@platforms//:incompatible
ERROR: BT stack was not properly configured, please set --@pico-sdk//bazel/config:PICO_BTSTACK_CONFIG to point to a library that provides BT stack configuration options and defines at least one of `ENABLE_CLASSIC=1` or `ENABLE_BLE=1`

At a minimum, it'd be nice to get some sort of actionable output here, but in the bigger picture there's a lot of libraries that require manual configuration before use and it'd be AMAZING if library authors could manually provide helpful error messages.

Which category does this issue belong to?

Configurability

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

cc_library(
    name = "foo",
    target_compatible_with = select({
        "//conditions:default": ["@platforms//:incompatible"],
    }),
)

Which operating system are you running Bazel on?

No response

What is the output of `bazel info release`?

No response

If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.

No response

What's the output of `git remote get-url origin; git rev-parse HEAD` ?

No response

If this is a regression, please try to identify the Bazel commit where the bug was introduced with bazelisk --bisect.

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

armandomontanez commented 3 weeks ago

This is a duplicate of #24055

gregestren commented 3 weeks ago

This is nitpicky, but if we're dreaming of an ideal message do we even want

  @@pico-sdk~1.6.0-rc1~_repo_rules~btstack//:pico_btstack_base (d6a49d)   <-- target platform (@@pico-sdk~1.6.0-rc1//bazel/platform:rp2040) didn't satisfy constraint @@platforms//:incompatible

in that @@platforms//:incompatible is really an implementation detail that doesn't speak directly to users?

I can't offhand think of anything cleaner than custom_incompatible_error on either the select() or the target it's attached to (would you still want this support for targets without select()s)?

I'd prefer a generic API that can expand without needing a new attribute per use case. But I can't think of anything better short of calling out to some special Starlark function, which sounds excessive.

armandomontanez commented 3 weeks ago

in that @@platforms//:incompatible is really an implementation detail that doesn't speak directly to users?

I think you're right. @platforms//:incompatible is just an implementation detail that communicates "is never compatible". Ideally, we spell that out in the error message.

Rather than a single custom_incompatible_error, it'd be nice if we could spell it per branch, but I doubt that's feasible at a technical level:

    target_compatible_with = select({
        "//:foo": incompatible("Never works with //:foo, sorry!"),
        "//:a_and_b_in_use": incompatible("Both //:a and //:b are enabled, which is incompatible with this library. Disable either //:a or //:b first."),
        "//conditions:default": [],
    })

I think there's ways you can take that idea and make it more realistic, perhaps by making it possible for custom @platforms//:incompatible rules that emit custom error messages when they're the value of a target_compatible_with attribute?

Another idea I had for better DX while brainstorming this is to just reproduce the select verbatim:

In @@pico-sdk~1.6.0-rc1~_repo_rules~btstack//:pico_btstack_base (d6a49d):

    target_compatible_with = select({
        "//:foo": ["@platforms//:incompatible"],  <-- Selected condition is always incompatible.
        "//:bar": ["@platforms//:incompatible"],
        "//conditions:default": [],
    })

gregestren commented 3 weeks ago

Rather than a single custom_incompatible_error, it'd be nice if we could spell it per branch, but I doubt that's feasible at a technical level:

Right, unfortunately that example messes too much with the attribute type system.

perhaps by making it possible for custom @platforms//:incompatible rules that emit custom error messages when they're the value of a target_compatible_with attribute?

That's an interesting idea!

@platforms//:incompatible is defined here:

constraint_value(
    name = "incompatible",
    constraint_setting = ":incompatible_setting",
)

You could define any constraint_value with the same constraint_setting and it should operate the same way. Slap an incompatible_explanation attribute on the constraint_value rule and I think we could get the desired result with simple, targeted code changes. And it would support your "message per branch" idea naturally.

The most complicated part would be connecting with the logic that throws the error. But I don't think that's unwieldy.

The main downside is the boilerplate of writing extra build rules to get your message, vs. simple inlining.

I think there's ways you can take that idea and make it more realistic, perhaps by making it possible for custom @platforms//:incompatible rules that emit custom error messages when they're the value of a target_compatible_with attribute?

Oh yes, I think we're saying the same thing. :)

Another idea I had for better DX while brainstorming this is to just reproduce the select verbatim:

Doesn't that neglect the actionable part of the message?

armandomontanez commented 3 weeks ago

Doesn't that neglect the actionable part of the message?

It does, but also

There's a bunch of select() statements that do this in the wild already. Would be nice for those to be more debuggable for free.
I suspect few projects are going to spend time to write out error messages for every case, so this helps with debugability when nobody has provided any actionable next-steps.

I don't think one solution necessarily precludes the other, though the presentation of both definitely require some thought so as not to overwhelm the user with too much text.

gregestren commented 3 weeks ago

In team chat today @katre supported the "override constraint_value" approach, with caveats:

extend the constraint_value rule (with extensible rules?) instead of adding an attribute to constraint_value that's special-purpose. Especially: this only applies to target_compatible_with but constraint_value is consumed more broadly than that
if we expose the message in a provider, we'd need a native provider implementation since core Bazel would consume it. Even if we expose it some other way (i.e. directly on some attribute), Bazel would have to be specially aware of it somehow.

I also want to re-emphasize this isn't intrinsic to select(), so I'd hope a good incompatibility message doesn't depend on select()'s API.

CC @philsc if you have any interesting ideas on better integrating @@platforms//:incompatible.

bazelbuild / bazel