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

Can not manipulate select statement in macros #8419

Open
Globegitter opened this issue May 21, 2019 · 11 comments
Open

Can not manipulate select statement in macros #8419

Globegitter opened this issue May 21, 2019 · 11 comments
Labels
not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request

Comments

@Globegitter
Copy link

Description of the feature request:

The rules_k8s have a rule k8s_objects which takes a list of labels (k8s_object targets) and creates multiple targets with these labels, with custom endings, that it knows have been created from the underlying k8s_object. So there is code like [x + ".create" for x in objects] If objects is now a select statement that fails with something like: select is not iterable. IT would be nice if I could still iterate over a select statement similar to a dict, so I could manipulate the labels in the macro.

Feature requests: what underlying problem are you trying to solve with this feature?

Now I have to use a normal dict as a workaround which the macro then converts into a select statement. I think this ends up being confusing for the end-user as that is not usual

What's the output of bazel info release?

0.25.2

Have you found anything relevant by searching the web?

#8171 but seems not directly related

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

For the exact use of this see PR: bazelbuild/rules_k8s#342

@gregestren
Copy link
Contributor

I'm glad you found the workaround of just using a direct dict. I acknowledge its limited appeal, although I think you could also make the argument that it's a cleaner approach, since macro arguments are not rule arguments and select() is really only intended for rule arguments. That said, this is already getting conceptually more complicated than most users are prepared for, especially when it all looks the same in a BUILD file.

This general request has been discussed a lot, particularly with the Starlark designers. In principle everyone supports it. But no one's yet figured out a clean design. I think it'd have to be its own dedicated mini-project to make this happen (i.e. go through a design review vs. just spinning off a PR when someone has a few spare cycles).

The core problem is that there are fundamental limits to how deeply macros can explore select(). Today select() is a 100% opaque object, which is not giving macros enough exposure. But we can't swing all the way in the other direction and make them 100% transparent. Macros simply don't have the power to manipulate select() as flexibly as rules can (unless we did something crazy like late-evaluate macros in Bazel's analysis phase).

So the question is how far to turn that knob. A simple iteration like you describe seems reasonable since it doesn't imply macros have to know anything about which select() path will actually get chosen. But it's easy to imagine all kind of things macros would like to do that might blur these distinctions, and at worst try to make macros exceed their actual power. So... should we

  • directly support iteration and only iteration? Maybe something like map(lambda cond: cond + "_suffix", my_select)?
  • Or just an iterator: for entries in my_select: ...?
  • Or provide a more general API that includes iteration but maybe other functionality?

And how can we prevent select() from being a "special" Starlark symbol any more than it already is? The more it represents an exception from standard Pythonic / Starlarkish expressions the messier the overall language is.

I have no doubt reasonable answers can come out of this. But it needs to be designed. So FYI about why I think we're not going to get a solution to this tomorrow.

Paging @brandjon who I've discussed this with on and off for some time.

@gregestren gregestren added P3 We're not considering working on this, but happy to review a PR. (No assignee) unprioritized projects labels May 28, 2019
@gregestren
Copy link
Contributor

Also @serynth - we've talked about having a better list of worthwhile but not-currently-prioritized projects. The label I just created for this issue is a simple attempt to nudge that idea forward (although I'm sure we'll want to do more).

@brandjon
Copy link
Member

brandjon commented May 28, 2019

I know I had a whole host of opinions last time select() in macros came up, and I've forgotten them all now. Off-hand, I'm inclined to favor an approach of "expose the structure of select" (opening it up to compatibility requirements, but how often would we break that?) and provide utilities in skylib to do abstract operations over them, like map and union.

@gregestren
Copy link
Contributor

gregestren commented May 28, 2019

To be clear, I'm happy to support anyone who wants to try designing an answer to this (design review, code review, code help, etc.) Starting from @brandjon 's suggestions would be a good start.

@cpsauer
Copy link
Contributor

cpsauer commented Mar 10, 2021

Just ran into this, too, trying to make cross-platform cc macros that can call into Java on Android. In my case, there's enough Android specialization already that I'll just weave a workaround into that, but very interested in a solution if one comes about. (subscribing and +1 :)

@c-parsons
Copy link
Contributor

Can we just make the underlying dictionary items passed to select() readable and immutable, like dict.items ?

x = select({
    "foo" : "bar",
    "baz" : "qux",
    "//conditions:default" : "default"
})
for k, v in x.items():
   print(k, v)

We don't expect selects to be excessively large, so I don't have efficiency concerns, and this allows macros to reconstruct selects as they see fit.

WDYT?

@c-parsons
Copy link
Contributor

c-parsons commented Jul 29, 2021

Scratch my previous message. Let me clarify a more rigorous proposal:

Some background:

As a mechanism to workaround this issue, I'm proposing that macro writers and users use the following convention:

  • Use a starlark list<string or dict> to signify a pre-constructed select.
  • Read / make modifications to this value in macro code as you see fit
  • Wrap in a select directly before passing to a rule attribute

Thus, instead of:

static_deps = ["//foo:bar"] + select({
        "//axisone:condition": ["//baz:qux"],
        "//conditions:default": [],
    }) + select({
        "//axistwo:conditiontwo": ["//my:othertarget"],
        "//conditions:default": [],
    })

You might create:

static_deps = ["//foo:bar"] + [{
        "//axisone:condition": ["//baz:qux"],
        "//conditions:default": [],
    }] + [{
        "//axistwo:conditiontwo": ["//my:othertarget"],
        "//conditions:default": [],
    }]

Creating an actual selector list from this type is fairly trivial:

def createselect(orig):
    result = []
        for v in orig:
            if type(v) == "dict":
                result += select(v)
            else:
                result += [v]
    return result

Upstream design proposal:

Expose a native method on selector list, items(), which returns an immutable data structure akin to the above proposal.

Thus:

    s = ["//foo:bar"] + select({
        "//axisone:condition": ["//baz:qux"],
        "//conditions:default": [],
    }) + select({
        "//axistwo:conditiontwo": ["//my:othertarget"],
        "//conditions:default": [],
    })
    
    readable_items = ["//foo:bar"] + [{
        "//axisone:condition": ["//baz:qux"],
        "//conditions:default": [],
    }] + [{
        "//axistwo:conditiontwo": ["//my:othertarget"],
        "//conditions:default": [],
    }]

    print(s.items() == readable_items) # true

I'll mail a PR which takes this approach, and ties it to an experimental flag.

@gregestren
Copy link
Contributor

Hi anyone still reading this. Please see the latest conversation at #14157. I'm trying to push forward a solution to this, but need the help of anyone who would like to see this happen.

@gregestren gregestren added the next_month Issues that we will review next month. This is currently mainly used by Configurability team label Nov 4, 2022
@aranguyen aranguyen removed the next_month Issues that we will review next month. This is currently mainly used by Configurability team label Jan 13, 2023
@gregestren gregestren removed their assignment Jan 13, 2023
@gregestren
Copy link
Contributor

We just reviewed this issue. My last comment remains relevant.

@sgowroji sgowroji added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 15, 2023
@sgowroji
Copy link
Member

Hi there! We're doing a clean up of old issues and will be closing this one. Please reopen if you’d like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

@fmeum
Copy link
Collaborator

fmeum commented Feb 15, 2023

@sgowroji Still relevant

@sgowroji sgowroji added not stale Issues or PRs that are inactive but not considered stale and removed stale Issues or PRs that are stale (no activity for 30 days) labels Feb 15, 2023
@sgowroji sgowroji reopened this Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not stale Issues or PRs that are inactive but not considered stale P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request
Projects
None yet
Development

No branches or pull requests