Use boolean instead of string parameter for repeatable Starlark flags #266
Conversation
|
|
||
| ``` | ||
| --copt=a,b,c --> ["a,b,c"] | ||
| --copt=a --copt=b,c --> ["a", "b,c"] |
There was a problem hiding this comment.
Ok the example really make the paragraph clearer. Thanks. Why can't this be ["a", "b", "c"]?
There was a problem hiding this comment.
It could, but I think that it shouldn't:
If we pass ["a", "b,c"] to the build setting implementation function unprocessed, it is free to turn this into ["a", "b", "c"] and pass it on to consumers via a provider in that form.
But if we pass ["a", "b", "c"] directly, we forcibly remove information that the build setting implementation function wouldn't be able to recover. For example, this would mess up linker flags such as -Wl,--enable-auto-import.
Given that build settings can already do any post-processing they require in Starlark, I would opt for doing as little pre-processing as possible within Bazel.
There was a problem hiding this comment.
Thanks for the explanation. I'm not familiar with the linker flags mentioned. Could you elaborate on how having ["a", "b", "c"] would mess them up?
There was a problem hiding this comment.
If you have a (hypothetical) Starlark version of the native --copt, then you may want to pass in -Wl,--enable-auto-import as a single argument. But if all repeated flags also split on ,, then this would be impossible: The final list of args would always include two separate args -Wl and --enable-auto-import, which changes semantics (in fact, in the case of this particular flag, this would likely lead to a compiler error).
@aranguyen @gregestren This is the update to the proposal following the result of our discussion.
Thanks! Could you merge? I don't have the rights. |
|
Done! FYI, I'm working on your PR and will update as soon as I can. |
@aranguyen @gregestren This is the update to the proposal following the result of our discussion.