Fix label conversion for --experimental_propagate_custom_flag
#25259
Conversation
Also fix and enable a test in Bazel.
github-actions
bot
added
team-Configurability
| @@ -234,6 +234,42 @@ public String getTypeDescription() { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Flag converter for canonicalizing a label (possibly with a "/..." suffix) and/or define by | |||
| * converting the label to ambiguous canonical form. | |||
There was a problem hiding this comment.
Do you mean "unambiguous canonical form", or is "ambiguous" correct here and I am misunderstanding?
There was a problem hiding this comment.
I really did mean "ambiguous" since the implementation used toString(), but I now changed this to getUnambiguousCanonicalForm() for clarity.
| } | ||
| // A "/..." suffix is not valid label syntax, so replace it with valid syntax and transform | ||
| // it back after conversion. | ||
| String invalidSubpackagesSuffix = "/..."; |
There was a problem hiding this comment.
Make these constants in the CustomFlagConverter class.
There was a problem hiding this comment.
I made /... a constant as it has meaning, but :__subpackages__ is arbitrary and not used anywhere else, so defining a constant for it may, in my opinion, even hurt readability.
| String validSubpackagesSuffix = ":__subpackages__"; | ||
| String escapedUnconvertedLabel = | ||
| input.endsWith(invalidSubpackagesSuffix) | ||
| ? input.substring(0, input.length() - invalidSubpackagesSuffix.length()) |
There was a problem hiding this comment.
Instead of the substring call, use String.replace. Any instance of /... inside the label is invalid anyway.
There was a problem hiding this comment.
Wouldn't //...super_hidden_folder:foo be a valid label? I can still do this, but I'm not sure it's theoretically correct.
There was a problem hiding this comment.
Fair enough, I hope no-one is naming packages like that but it's technically possible, so leave this as-is.
| @@ -234,6 +234,42 @@ public String getTypeDescription() { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Flag converter for canonicalizing a label (possibly with a "/..." suffix) and/or define by | |||
There was a problem hiding this comment.
Why does this mention --define (also the comment on line 245). It doesn't look like this converter is used except for with --experimental_propagate_custom_flag.
There was a problem hiding this comment.
The option accepts both Starlark flags and --defines.
| String validSubpackagesSuffix = ":__subpackages__"; | ||
| String escapedUnconvertedLabel = | ||
| input.endsWith(invalidSubpackagesSuffix) | ||
| ? input.substring(0, input.length() - invalidSubpackagesSuffix.length()) |
There was a problem hiding this comment.
Fair enough, I hope no-one is naming packages like that but it's technically possible, so leave this as-is.
katre
added
awaiting-PR-merge
|
@bazel-io fork 8.1.0 |
Also fix and enable a test in Bazel.
Fixes #25208