Fix absolute label use in main attribute of py_test #463
Conversation
In rules_python you can specify a file from another package like: ```bzl main = "//path/to:file.py", ``` This didn't work in rules_py since it was taking the main as a string to manipulate. This appears closer to me to the upstream logic that was mirrored, so maybe this was changed for another reason
|
Hm, remembering what I did here... f2240ac#diff-6878df526d6e39f46c6e953f0cfcc2e5b7938d5c4d2cbec0657e78c99b931239R80-R81 I suspect this is a breaking change, in some cases users might already have a However this PR is green, so either we are missing some testing for the case I'm concerned about, or it's not actually possible to misuse the current string-typed attribute. |
|
I don't really have the full context here, it was Alex who made this change, so probably he's the best person to review it. |
| @@ -87,7 +87,7 @@ determine_main = rule( | |||
| implementation = _determine_main_impl, | |||
| attrs = { | |||
| "target_name": attr.string(mandatory = True, doc = "The name of the py_binary or py_test we are finding a main for"), | |||
| "main": attr.string(doc = "Hint the user supplied as the main"), | |||
| "main": attr.label(doc = "Hint the user supplied as the main", allow_single_file = True), | |||
There was a problem hiding this comment.
This is technically a breaking change as Alex pointed out, so i am not sure if we can land it as a minor.
| @@ -26,7 +26,7 @@ def _determine_main(ctx): | |||
| """ | |||
| if ctx.attr.main: | |||
| # Deviation from rules_python: allow a leading colon, e.g. `main = ":my_target"` | |||
| proposed_main = ctx.attr.main.removeprefix(":") | |||
| proposed_main = ctx.attr.main.label.name.removeprefix(":") | |||
There was a problem hiding this comment.
I don't think label.name will ever have a : as prefix. maybe we are really missing something here?
In rules_python you can specify a file from another package like:
This didn't work in rules_py since it was taking the main as a string to
manipulate. This appears closer to me to the upstream logic that was
mirrored, so maybe this was changed for another reason