Update ctexplain to run with bazel and newer python versions #17109
Conversation
torgil
force-pushed
the
run-ctexplain-with-bazel-and-newer-python-versions
branch
2 times, most recently
from
a58a457
to
5be7ae1
Compare
Use "bazel" instead of "blaze" as default binary. Add option to specify bazel binary. Detect label arguments that starts with "@". Import bazel_api, lib, util and summary from qualified path to avoid collisions for users using this as a module. It also requires fewer entries in PYTHONPATH. Updated to run with Python 3.10. "Mapping" has been moved from collections to collections.abc. Use regular expression instead of split on space to cope with spaces in label name for cquery output.
torgil
force-pushed
the
run-ctexplain-with-bazel-and-newer-python-versions
branch
from
5be7ae1
to
4880721
Compare
sgowroji
added
team-Rules-Python
gregestren
added
team-Configurability
|
Thank you for providing this. I'd like to follow up on this and #14236 soon. I'd really love to see a usable version of |
There was a problem hiding this comment.
Overall this looks good to me. Thanks for the clear breakdown of changes.
I'm happy you're doing this and want to support a full landing of ctexplain into the repo in a truly helpful state. I do need to dust off the code and remember its details - it's been sitting unmonitored for a while - so forgive me as I bring myself back up to speed, and am uncertain about some details.
First and foremost, I can't remember if the ctexplain tests automatically run as part of the CI. I hope so, but am not sure. Let's verify?
Otherwise, this basically LGTM. When imported into Google's repo Google runs some of its own presubmit checks (linters particular). So we may both get some further feedback from there after first trying to import. It should all be manageable, though.
I'll follow up in a comment with other code to import, to make the tool more feature-full. You don't have to do specifically import it, but if you want to I'm all in for reviewing. Someone else once volunteered to help with importing too if I can find their Github handle.
| config_hash = tokens[1][1:-1] | ||
| if config_hash == "null": | ||
| fragments = () | ||
| result = CQUERY_RESULT_LINE_REGEX.search(line) |
There was a problem hiding this comment.
Implementing the TODO from line 139 would be a great followup, especially if it's not hard (not sure it is).
I know you're just expanding on the parsing logic already implemented, but this (existing) logic is way more brittle than it should be.
| @@ -28,8 +30,12 @@ | |||
| from tools.ctexplain.types import HostConfiguration | |||
| from tools.ctexplain.types import NullConfiguration | |||
|
|
|||
| CQUERY_RESULT_LINE_REGEX = re.compile(r"^(.*?) \((\S+)\) \[([^[]*)\]$") | |||
There was a problem hiding this comment.
I'm not sure what official style recommendations are, but can this be colocated closer to its sole consumer for less context jumping?
|
I consolidated what I think are the steps to get all the current ctexplain logic checked in: #14236 (comment) This is a generic tool so there are all kinds of ways we can imagine extending / enhancing that logic. But as the baseline I'd support landing all the above. Some are already PRs. The main blocker is review. My ideal is someone else carries through the PRs and I review them. I'm also happy to reverse if either of you wanted to review. But I might be a bit slower on that. |
Use "bazel" instead of "blaze" as default binary.
Add option to specify bazel binary.
Import bazel_api, lib, util and summary from qualified path to avoid collisions for users using this as a module. It also requires fewer entries in PYTHONPATH.
Updated to run with Python 3.10. "Mapping" has been moved from collections to collections.abc.
Cope with spaces in label name for cquery output. Regular expression is used instead of split.