Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow combining select[Label]s #24674

Open
RyanDraves opened this issue Dec 12, 2024 · 5 comments
Open

Allow combining select[Label]s #24674

RyanDraves opened this issue Dec 12, 2024 · 5 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions team-Starlark-Interpreter Issues involving the Starlark interpreter used by Bazel type: feature request

Comments

@RyanDraves
Copy link

Description of the feature request:

With the new symbolic macros, it's difficult to manipulate attr.label inputs, as they're wrapped in select[Label] objects. Allowing the following operation would regain a lot of functionality: select[Label] + select[Label] -> select[list[Label]].

Which category does this issue belong to?

Configurability, Starlark Interpreter

What underlying problem are you trying to solve with this feature?

To retain flexibility in making labels a list of labels within a macro.

Which operating system are you running Bazel on?

Ubuntu 24.04

What is the output of bazel info release?

release 8.0.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 HEAD ?

[email protected]:RyanDraves/nlb.git
b33e99e0d05e3016ca92680b186479c7b0ca0e2e

I removed the symbolic macros; I can make a branch re-adding them if requested. A full example is on [Slack](https://bazelbuild.slack.com/archives/CA31HN1T3/p1733981276254349?thread_ts=1733981177.926289&cid=CA31HN1T3).

Have you found anything relevant by searching the web?

Relevant docs: https://bazel.build/docs/configurable-attributes#faq-select-macro
Relevant proposal: https://docs.google.com/document/d/1c-8GNkz57pDgBGPdJoXJ42aCFHr-gN-Ebon9Zx1COsk/edit?tab=t.0#heading=h.5mcn15i0e1ch

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

No response

@github-actions github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions team-Starlark-Interpreter Issues involving the Starlark interpreter used by Bazel labels Dec 12, 2024
@fmeum
Copy link
Collaborator

fmeum commented Dec 12, 2024

@brandjon

@fmeum
Copy link
Collaborator

fmeum commented Dec 12, 2024

I just realized that the "Introspecting selects" proposal could solve this by mapping each value with lambda x: [x], so maybe that suffices as a neat generic solution.

@brandjon
Copy link
Member

Allowing the + operator on two select-of-label operands would break the invariant, that the operator on a select produces a new select with the operator applied pairwise to the select values.

I just realized that the "Introspecting selects" proposal could solve this by mapping each value with lambda x: [x]

Yes, that would be preferrable. In fact, I was looking forward to that as a possible solution for the pattern where a symbolic macro calls some_rule(..., a = [b]), where a has type label_list and b has type label, but it breaks because b gets promoted to a select inside a list.

Unfortunately, that proposal ran into issues around 1) how much information does the callback see about the structure of a concatenation of selects; and 2) what is the behavior of the callback w.r.t. a None select value. So that proposal isn't being pursued right now, and a modified or alternative design would be needed.

In the meantime, when select()s get in the way of a symbolic macro, you can set the attr to be configurable=False. (Yes, this is a restriction on callers of the macro, but it's not a new restriction; it just makes the latent failure into a fail-fast.)

@RyanDraves
Copy link
Author

configurable=False has been very helpful with getting some of my macros ported, thank you. I keep encountering the default of configurable=True as a foot-gun when trying to use symbolic macros. A simple case I just encountered and debugged: using an attr.bool as a flag to switch on in a macro. E.g.

def _my_macro_impl(name, visibility, my_flag):
  print(my_flag)
  if my_flag:
    ...

my_macro = macro(
  implementation = _my_macro_impl,
  attrs = {
    "my_flag" = attr.bool(default = False),
  }
)

This will log select({"//conditions:default": False}) and evaluate to True in the if condition.

I'm finding this default behavior of receiving unresolved select objects and being unable to work with them a pitfall of these symbolic macros. Are there plans for macros to be more akin to results where the attributes are first resolved, or is it intended to use configurable = False anytime an attribute is used beyond a simple pass-through?

@brandjon
Copy link
Member

I keep encountering the default of configurable=True as a foot-gun when trying to use symbolic macros.

Yes, basically anytime you need to consume the value in the macro's logic you need to remember to opt out of configurability. This was deemed better than defaulting to nonconfigurable because it's more consistent with rules (whose attributes are almost always configurable) and errs on the side that you're more likely to notice right away if you get it wrong (as opposed to forgetting to declare srcs to be configurable, which leaves functionality on the table unnecessarily).

evaluate to True in the if condition.

This is a known pitfall that would be fixed by #14506. The only problem is it's weird to have a Starlark value that fails when you implicitly attempt to convert it to a bool, but I suspect that's not enough of a blocker to justify not doing it.

Are there plans for macros to be more akin to results where the attributes are first resolved, or is it intended to use configurable = False anytime an attribute is used beyond a simple pass-through?

The latter. By design macros operate during the loading phase before it is possible to resolve select()s.

@gregestren gregestren added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions team-Starlark-Interpreter Issues involving the Starlark interpreter used by Bazel type: feature request
Projects
None yet
Development

No branches or pull requests

7 participants