Throw exception if 'None' value given to List-type command line options in Bazel transitions #13286
Conversation
tobiade
commented
Apr 1, 2021
tobiade
commented
- Proposed fix for NullPointerException when overriding copts and linkopts to None in transition #12559
- Command line options specified as Java List types should default to empty when given 'None' Starlark value for Bazel transitions
| @@ -332,6 +332,8 @@ private static BuildOptions applyTransition( | |||
| .convert( | |||
| optionValueAsList.stream().map(Object::toString).collect(joining(","))); | |||
| } | |||
| } else if (def.getType() == List.class && optionValue == null) { | |||
| convertedValue = def.getDefaultValue(); | |||
There was a problem hiding this comment.
Please let me know if it's okay to replace with the default value here or if we should be using StarlarkList.empty() instead.
There was a problem hiding this comment.
I'm also curious if it's okay to perform the override here, or if it makes sense to do it within this block of code above:
if (optionValue instanceof NoneType) {
optionValue = null;
}
I need the OptionDefinition to check if it's a List, so the current spot seemed more convenient.
There was a problem hiding this comment.
About the first question, it's a good question. My first intuition was None = empty, but there really is something to say for it meaning default. Especially if we already have ways to express empty ("", []).
There's a sensible interpretation that says None = unset, which implies the default.
@brandjon - does None have any incompatible implications in Starlark? Can
"//command_line_option:copt": None,
(in a transition) reasonably imply unsetting the flag?
There was a problem hiding this comment.
I just posted an update in the associated issue. I'm still trying to wrap my mind around the three conceptual options:
- disallow
None None= emptyNone= the flag's default value (as you currently have encoded).
There was a problem hiding this comment.
Thanks @gregestren. Makes sense. I think if a use-case exists where a user wants to set a flag back to its default (without actually knowing what the default value is), then maybe None might be a good way to achieve this.
There was a problem hiding this comment.
Okay, I also asked some other core devs. including @brandjon who curates the core Starlark API.
The short consensus, which I find compelling, is these are intriguing design issues. But they're not trivial. Not only are all the above interpretations possible, but None could potentially be a distinct value in its own right, since it's a legitimate value Starlark variables can take.
So basically, we shouldn't try to answer these questions yet. They deserve a deeper and broader design about what flag parsing looks like and how it mixes with Starlark. So if we simply disallow None with a clear error message now, we've solved the crash in #12559, can give users clear enough guidance with the error message, and aren't narrowing our options yet for however we'd ultimately want this to work.
How does that sound to you?
There was a problem hiding this comment.
Understood - that makes sense. I'll make that change, thanks for the following up!
aiuto
added
the
team-Configurability
…y when given 'None' Starlark value during Bazel transitions
…ns in Bazel transition definition
tobiade
changed the title
|
@gregestren Updated as discussed. |
|
Please hold for the PR to auto-merge in a timely manner... |
