Transitions can inspect configurable attributes, but don't see a value for them

[illicitonion]

Description of the problem / feature request:

When a rule has a configurable attribute, a transition which operates on the rule gets an attr map which doesn't contain the attr name or value at all.

This can cause buggy behaviour. e.g. this transition in rules_go looks at what the value of getattr(attr, "static", "auto") to decide whether to statically link a binary, but if the static attribute is a configurable attribute, the transition will always see None as the value of the attribute, so will fall back to its default behaviour.

Ideally the transition would get the current configuration's value of the attribute.

If that's not feasible, trying to read one of these attributes in a configuration should be an error (conceptually, we'd be declaring any attribute used in a transition as non-configurable, but by doing this detection on the fly and raising a runtime error; alternatively, we could introduce a "non-configurable" marker on attrs, and require it to be set for any attributes read by transitions).

In the case we decide to forbid this acces, it would be great to work out how rules_go's transition should be determining whether a target should be compiled as static.

Either way, silently hiding the attribute is error-prone.

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

% touch WORKSPACE
% cat >rules.bzl <<EOF
def _print_transition_impl(settings, attr):
    print("Attr: {}".format(getattr(attr, "explicit_config", "NOT PRESENT")))
    return {}

print_transition = transition(
    implementation = _print_transition_impl,
    inputs = ["//command_line_option:cpu"],
    outputs = ["//command_line_option:compilation_mode"],
)

def _is_transitioned_impl(ctx):
    out = ctx.actions.declare_file(ctx.label.name)
    ctx.actions.write(
        output = out,
        content = ctx.attr.explicit_config,
    )
    return [DefaultInfo(files=depset([out]))]

is_transitioned = rule(
    implementation = _is_transitioned_impl,
    attrs = {
        "explicit_config": attr.string(default = "one"),
        "_whitelist_function_transition": attr.label(default = "@bazel_tools//tools/whitelists/function_transition_whitelist"),
    },
    cfg = print_transition,
)
EOF
% cat >BUILD.bazel <<EOF
load("//:rules.bzl", "is_transitioned")

is_transitioned(
    name = "transitioned",
    # This version shows up in the transition:
    #explicit_config = "String in BUILD",
    # This version does not:
    explicit_config = select({"//conditions:default": "Default condition in BUILD"}),
)
EOF
% bazel build :transitioned

Expect to see printed "Attr: Default condition in BUILD"
Actually see printed: "Attr: NOT PRESENT"

If you switch over the commented explicit_config values in the BUILD.bazel file and re-run, you'll see "Attr: String in BUILD" as expected.

What operating system are you running Bazel on?

macOS

What's the output of bazel info release?

release 6.0.0-pre.20220324.1

[illicitonion]

/cc @gregestren

[gregestren]

Just skimming this (apologies!), but I think this is an intentional design restriction described at https://bazel.build/rules/config#defining. Specifically:

When attached as an incoming edge transition, attr does not include any attributes that use a selector to resolve their value. If an incoming edge transition on --foo reads attribute bar and then also selects on --foo to set attribute bar, then there's a chance for the incoming edge transition to read the wrong value of bar in the transition.

https://docs.google.com/document/d/1VIRx06cZB4wLU-ASq1XKFHmx67yfHtNOCbCempaPeaA/ gets into the ugly details:

In order to make the second change work, we need to introduce restrictions for starlark rules that use parameterized rule class transitions. Parameterized rule class transitions cannot depend on attribute values that proceed to depend on configuration (i.e. through a select) since this would introduce a configuration->attribute->configuration dependency cycle.

To deal with this, we’ll throw runtime errors for transition implementation functions that create these dependency cycles. The specific case we’re interested in is when (1) a build setting* is declared written by a transition at any point** on the incoming edge to a rule-transitioned target, (2) an incoming rule transition transition reads an attribute of the target, and (3) that target configures the same attribute using the same build setting. If this happens, we’ll throw an error when we try to access that attribute in the transition implementation function. This all gets a bit hairy, see the examples below for a more concrete example.

I agree that silent failure is no good. It looks like the original intention was for Bazel to communicate the problem?

Also, I think the main conclusion from then was that supporting your use case is technically hard. That doesn't necessarily mean it's impossible - there may be some innovative thinking we didn't think of. The current state is largely a desire to get the functionality working and aiming for just not supporting certain use cases vs. blocking the functionality on a more complete solution. I'm open to creative ideas beside better error messaging if you can think of any.

[fmeum]

@gregestren I'm not sure how difficult this would be to implement, but how about the following: Populate the configured value for all attributes for which the select keys are disjoint from the set of output settings declared by the transition. This should get around all the difficult situations, but still allow for the common, non-pathological use cases.

[gregestren]

I think that's possible. I think the logic would be:

1. In the part of the parent's ConfiguredTargetFunction that computes child configurations:
2. if //:child has both a self-transition and a select()
3. then get its select() keys as labels and Skyframe-evaluate those labels to targets.
4. Then parse those targets to fine-tune the set of unsafe flags.

It sounds tricky but doable. Mostly tricky in not spreading the logic across too many classes or overly-complicating method signatures. I'm also not sure about manually parsing the config_setting's Target vs. calling its ConfiguredTarget and leveraging whatever code already determines what flags are in there.

[gregestren]

Re: rules_go - are there reports of breakages? How'd you find this error?

[illicitonion]

Re: rules_go - are there reports of breakages? How'd you find this error?

Yes, I ran into this issue when I started trying to conditionally make static binaries depending on the target platform. bazel-contrib/rules_go#3103 was my issue report there, and bazel-contrib/rules_go#3130 a proposed (but somewhat hacky) fix.

[gregestren]

Got it, thanks.

Principally I support narrowing down which attributes we can't access as much as we reasonably can. I don't have time at the moment to tackle this but I'm open to contributions. With the caveat that there is a balance of utility vs. code complexity, so any code I'd want to be very careful doesn't overly complicate Bazel's code in one of the more nuance parts of its code base. That's where I'd focus review attention.

[gregestren]

I've heard another request from https://github.com/lizkammer.

[aiuto]

Not really assigning myself, but github does not have a CC capability

[aiuto]

We do intend to prioritize this shortly. Stay tuned.

[gregestren]

Update:

@aranguyen has been working on this with good success. Main remaining technical complications:

• integrating with @aoeui 's refactoring of ConfiguredTargetFunction which affects all the surrounding code context (e.g. e963ec5 and 72496b5 and much of https://github.com/bazelbuild/bazel/commits?author=aoeui)
• updating the transition cache key (which currently takes Rule instances) to also consider config_settings

We intend to either have this done by end-of-June or a have a clear update of the final work remaining by then.

[gregestren]

Reviewing at https://bazel-review.googlesource.com/c/bazel/+/224533.

[aiuto]

Any updates?

[gregestren]

You can see the latest patch set from https://bazel-review.googlesource.com/c/bazel/+/224533 is from Aug 15. We're doing an internal review on this, which looks close to ready to me.