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

select() is not resolved when used in a list comprehension or dict.values() #8171

Closed
mexlez opened this issue Apr 27, 2019 · 11 comments
Closed
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Documentation Documentation improvements that cannot be directly linked to other team labels type: documentation (cleanup)

Comments

@mexlez
Copy link

mexlez commented Apr 27, 2019

Description of the problem / feature request:

If a dictionary or list contains a select(), and that dict/list is used in a list comprehension (i.e. when constructing a string of all keys in the dict), the select() is not resolved.

The select() is also not resolved when accessing the values in a dict using dict.values().

Using a for loop to construct a string in a macro instead of a list comprehension expands the select() as expected. However, constructing a list in a macro does not expand the select().

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

test.bzl:

# select() resolved
def test_stringify(d):
  s = ""
  for key in d:
    s += d[key] + "\n"
  return s

# select() NOT resolved
def test_listify(d):
  l = []
  for key in d:
    l.append(d[key])
  return l

BUILD.bazel:

load("//:test.bzl", "test_stringify", "test_listify")
d = {
  "1": "dict_constant",
  "2": select({
      "//conditions:default": "dict_selected"
    })
}
key_in_variable = "2"

l = [
  "list_constant",
  select({
    "//conditions:default": "list_selected"
    })
]

genrule (
  name = "test",
  outs = ["test"],
  cmd = "echo '" +
      "#### Select() is *NOT* expanded by these incantations:\n" +
      "\n".join([d[x] for x in d]) + "\n\n" +
      "\n".join(d.values()) + "\n\n" +
      "{}".format(d.values()) + "\n\n" +
      "\n".join([d[k] for (k, x) in zip(d.keys(), d.values())]) + "\n\n" +
      "\n".join(l) + "\n\n" +
      "\n".join([l[i] for i in range(len(l))]) + "\n\n" +
      str(test_listify(d)) + "\n\n" +
      "#### Select() *IS* expanded by these incantations:\n" +
      l[1] + "\n\n" +
      d["2"] + "\n\n" +
      d[key_in_variable] + "\n\n" +
      test_stringify(d) +
  "' > $(location test)"
)
$ bazel build //:test && cat bazel-genfiles/test
#### Select() is *NOT* expanded by these incantations:
dict_constant
select({"//conditions:default": "dict_selected"})

dict_constant
select({"//conditions:default": "dict_selected"})

["dict_constant", select({"//conditions:default": "dict_selected"})]

dict_constant
select({"//conditions:default": "dict_selected"})

list_constant
select({"//conditions:default": "list_selected"})

list_constant
select({"//conditions:default": "list_selected"})

["dict_constant", select({"//conditions:default": "dict_selected"})]

#### Select() *IS* expanded by these incantations:
list_selected

dict_selected

dict_selected

dict_constant
dict_selected

What operating system are you running Bazel on?

Ubuntu 18.04

What's the output of bazel info release?

release 0.22.0

@laurentlb laurentlb added team-Configurability platforms, toolchains, cquery, select(), config transitions and removed team-Starlark labels May 7, 2019
@laurentlb
Copy link
Contributor

I think it's working as intended. The select object supports the + operator (this returns a new select object).

However, if you do str(my_select) or "{}".format(my_select), it will be converted to string without expansion. The str.join method implicitly converts the arguments to string, but this behavior will change soon (#7802).

This may help understand what happens: bazel query --output=build :all.
Bazel first evaluates the BUILD file. If there are any select object after the evaluation, they will be expanded.

@mexlez
Copy link
Author

mexlez commented May 8, 2019

Ahhh, so what I'm really seeing with the + case is a big select() being constructed, which is then expanded before being printed later on in the bazel evaluation process; while in the str(my_select) case, the select() is converted to a string immediately and printed later.

Are the str.join changes expected to make the str(my_select) case behave the same as the + case?

In the meantime, is there a recommended cleaner way to implement the behaviour I'm looking for? I can use a macro function, but it's a bit clunky.

@katre katre added P2 We'll consider working on this in future. (Assignee optional) type: feature request and removed untriaged labels May 8, 2019
@gregestren
Copy link
Contributor

Yes. Any attribute's value setting can ultimately be an arbitrary concatenation of native values and select()s. Since cmd is a string attribute, its native values are strings. So anything of the form:

cmd = "some_string" + select(...) + "other_string" + select() + ...

will always fully resolve.

Anything that's not that exact pattern, which would include explicitly converting theselect() to a string, won't resolve.

str(my_select) can never resolve because str evaluates before Bazel knows which select() path is chosen. I second @laurentlb 's suggestion to do a bazel query --output=build, which shows this phase distinction. Anything between what appears in your actual BUILD file and the output of query cannot resolve select()s. Anything after that point will be resolved.

What exact behavior are you looking for? It seems that listifying or stringifying a select() is more a means to an end vs. your explicit final goal.

@laurentlb
Copy link
Contributor

Are the str.join changes expected to make the str(my_select) case behave the same as the + case?

No. Future behavior (you can try it with --incompatible_string_join_requires_strings) is to throw an error in this case. You'll have to explicitly convert string elements to string (str). This should be more obvious for the users that we're doing a string conversion.

@mexlez
Copy link
Author

mexlez commented May 8, 2019

EDIT: something is up with github's comment ordering, the posts I'm responding to have a modification time in the future.

Yes. Any attribute's value setting can ultimately be an arbitrary concatenation of native values and select()s. Since cmd is a string attribute, its native values are strings. So anything of the form:

cmd = "some_string" + select(...) + "other_string" + select() + ...

will always fully resolve.

Anything that's not that exact pattern, which would include explicitly converting theselect() to a string, won't resolve.

str(my_select) can never resolve because str evaluates before Bazel knows which select() path is chosen. I second @laurentlb 's suggestion to do a bazel query --output=build, which shows this phase distinction. Anything between what appears in your actual BUILD file and the output of query cannot resolve select()s. Anything after that point will be resolved.

What exact behavior are you looking for? It seems that listifying or stringifying a select() is more a means to an end vs. your explicit final goal.

Correct, the listifying/stringifying is a workaround to expand the select.

The use case is that we want to be able to have a set of values set in one place with a config_setting/select that can be consumed in a bunch of places throughout the build system.

In particular, I encountered this behaviour when trying to write a genrule that inserts some data (stored inside bazel as the single source of truth) into a shell script. I was trying to iterate over a dictionary and insert all key:value pairs into the script as KEY='VALUE'. As long as there are no values that are selects, you can do a nice readable list comprehension to generate the shell script; but the presence of a select breaks it because the select is immediately transformed into a string representation before it can be expanded. In this case, I have to write a macro function, which makes things messier and less readable (and we'll likely need a few variants that all turn a select into some form of string):

d = {
  "1": "dict_constant",
  "2": select({
      "//conditions:default": "dict_selected"
    })
}

def stringify_dict_containing_select(dict, prefix, separator, suffix):
    s = ""
    for key in dict:
        s += prefix + key + separator + dict[key] + suffix
    return s

genrule (
  name = "test",
  outs = ["test"],
  cmd = "echo '" +
      # Select is not expanded here
      "\n".join([x + "='" + d[x] + "'" for x in d]) +
      # Produces equivalent text, but select is expanded here
      stringify_dict_containing_select(d, "", "='", "'\n") +
  "' > $(location test)"
)

I guess this issue has morphed into a request for a nicer way to do things like list comprehensions using selects. In the current implementation they have this sort of dual personality where they behave differently depending on the context in which they're used (i.e. + expands the select when used outside of a list comprehension, but doesn't when used inside).

@gregestren
Copy link
Contributor

I appreciate your point - these are awfully similar UIs and intuitively it's frustrating that what should be basically a small cosmetic difference has such divergent implications. Unfortunately I don't think there's an obvious fix.

The core challenge, which is decidedly not obvious just casually looking at the syntax as a user, is that while BUILD evaluation looks a lot like pure Python execution, it's actually a two-phased process. Pure Pythonic expressions like comprehensions are evaluated in the first phase. But selects can not be evaluated in that phase because they also rely on command-line flags which are not part of the BUILD file.

It's actually possible to construct a build with two targets such that the same select in the same BUILD file produces different results for each target. So "\n".join([... d[x] .. ) simply cannot get a coherent answer without additional information.

Straight-up + works because it was specially programmed to support "lazy" evaluation that can defer evaluation to the second phase. In short, the first phase just translates "foo" + select(...)" to select({"//conditions:default": "foo"}) + select(...). This is possible because there's a clear way to transform the expression that preserves its semantics without having to know what the select() resolves to. This can then get passed to the second phase for final resolution and everything just works.

Every new Pythonic expression we'd want to support select resolution would have to find an equivalent mapping. For your desired list comprehension to work, we'd have to make "\n".join (which evaluates in the first phase) accept a non-resolved select(), pass that entire expression to the second phase in a semi-structured form, resolve the select()s, create the final list in what's basically a second pass of the first phase, continue the call to "\n".join to get its final result, and finally plug that into whatever was calling "\n".join in the first place.

This is all theoretically possible. But it's not simple, and in the end I think comes down to arbitrary interleaving of these two evaluation phases, which would require some pretty deep Bazel design changes.

I realize these difficulties are subtle and non-obvious. I still appreciate what you're trying to do and would be happy to consider possibly more limited solutions that don't hit these challenges straight-on.

@gregestren gregestren added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed P2 We'll consider working on this in future. (Assignee optional) labels May 10, 2019
@mexlez
Copy link
Author

mexlez commented May 10, 2019

Understood. Thanks for the thorough explanation of what's going on, the insight is very helpful and greatly appreciated!

A low-effort-high-impact improvement might be a section in the select docs with a tl;dr version of this issue. I think it would be a big improvement for people who are trying to use select in this way if it were explicit that + is the only operator that "just works".

@gregestren
Copy link
Contributor

Marking this is a documentation issue due to the last comment.

@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
@sgowroji sgowroji added the team-Documentation Documentation improvements that cannot be directly linked to other team labels label Jan 11, 2023
@gregestren gregestren removed their assignment Jan 13, 2023
@aranguyen aranguyen removed next_month Issues that we will review next month. This is currently mainly used by Configurability team team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request labels Jan 13, 2023
@gregestren
Copy link
Contributor

I'm decisively moving select() documentation issues to the team-Documentation label.

I think the right followup is for the team focusing on docs improvement to group together select() issues and propose a consolidated set of improvements on the select() docs. Programmers can fill out the content as needed.

As always anyone is welcome to off their own ad hoc PR.

Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Mar 19, 2024
Copy link

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please post @bazelbuild/triage in a comment here and we'll take a look. Thanks!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Documentation Documentation improvements that cannot be directly linked to other team labels type: documentation (cleanup)
Projects
None yet
Development

No branches or pull requests

7 participants