User-defined build flag bypasses its implementation function when used in transition.

[konste]

Description of the bug:

User-defined build flag does not execute its implementation function when it is used only in transition.

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

I have compact repro here: https://github.com/Bazel-snippets/multistring

There are two user-defined flags: target_platforms and flavors. Implementation function _multistring_impl validates given value. Command line to try is bazel build transistor --//:flavors=dbg

Observed outcome is the build success, but it is wrong because dbg is not a valid value for this flag (it is commented out) and _multistring_impl supposed to complain about it, but it is never executed!

Now if you uncomment line 50 in transistor.bzl to mention //:flavors flag in the rule then _multistring_impl gets executed and produces expected error message.

Which operating system are you running Bazel on?

Linux, Mac, Windows

What is the output of bazel info release?

5.2.0

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 master; git rev-parse HEAD ?

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

[gregestren]

Quick glance response:

This isn't a bug, per se, but an intentional part of the configuration API (I updated the tag accordingly). Flags are set at the command line as raw unparsed strings. They're consumed by transitions as lightly parsed Starlark objects. They don't take their final form until consumed by the implementation as a dependency of some target that needs them.

That said, I undersatnd this is subtle, and not necessarily what you want. We could contemplate tweaks to the APi. Two immediate followups:

1. What does it ultimately mean to you if the validation doesn't happen? i.e. if it's only used by a transition then no targets actually use it, so how important is its value? I realize select() could also come into play
2. As an admittedly awkward workaround, could you have some target depend on the build flag to trigger its implementation function, and then just do nothing with the dependency?

[sdtwigg]

select() should trigger the implementation function since it is ultimately provider-driven. So, you would only see these raw values in transitions, which is fine. (There are performance and correctness concerns with trying to validate in the middle of transitions, especially if multiple transitions have been composed into one transition.)

[konste]

if it's only used by a transition then no targets actually use it, so how important is its value?

First of all my repro is not artificial - it is minified version of the actual code we use, so the issue is very much practical.
As you can see from my repro even though flags are only used by transition and no target depend on them directly - transition changes //command_line_option:platforms and //command_line_option:compilation_mode based on those flags and therefore all targets behind that transition get indirectly affected by those flags and therefore their values matter. This is very much intended.

What we observed is that users may specify illegal values for the flags on the command line and unless transition implementation is smart enough to perform its own validation we end up with unpredictable transitions and very obscure errors, rather than a clear message that illegal value is specified for particular flag. Worst case there are no errors, just unpredictable behavior which is even harder to discover and diagnose.

As an admittedly awkward workaround, could you have some target depend on the build flag to trigger its implementation function, and then just do nothing with the dependency?

In my repro the line commented in transistor_ruledefinition does exactly that. And it helps, but as soon as anybody sees it they try to remove those seemingly unused attributes and because validation builds typically use correct values for the flags they get confirmed as unused and therefore removed!

[sdtwigg]

Re-affirming and restating #15994 (comment), it is currently by-design that transitions operation on the 'raw' values of the configuration before those implementations are run. Changing this is infeasible.

Philosophically, build_setting or build_flag is actually a (slightly special) rule and the implementation function of a build_setting is actually a (slightly special) rule implementation function that takes the raw configuration value and other attributes (which may be Label-type and thus the results of other rule evaluations!) to put the configuration value in a provider suitable for consumption by other rules (like config_setting).

In contrast, transitions are designed to only work on configurations (and not consume rules as metadata), which are essentially just dicts of dicts of text to text at this point. There would be some serious performance concerns if transitions alone could trigger ancillary rule analysis, especially when transitions are composed or chained.

We could probably revamp the documentation on transitions to make it clearer (e.g. notice also how transitions on label-typed settings only see the label as a string, not the transitive provider set). But, in your case, if the transition itself needs to do validation then it should do the validation in the transition. (Perhaps define a helper function in a bzl file that the build_setting implementation and transitions can both call to do the validation uniformly?)

[konste]

I understand the reasoning. Speaking of a workaround adding unused private label typed attribute pointing to build_flag to any (?) rule looks like an easier workaround, although it requires extensive comment about why it is there.

To mitigate it for others I would simply add a sentence on this page informing that build_setting implementation function is only invoked when build_setting is used as some rule label attribute value and is not invoked if build_setting is ONLY used in transitions.