-
-
Notifications
You must be signed in to change notification settings - Fork 644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor: DRY adhoc_tool code_quality_tool #20255
Refactor: DRY adhoc_tool code_quality_tool #20255
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I spotted that AdhocToolNamedCachesField doesn't seem to be ported over, though.
…pants into refactor/adhoc-runnable-dry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one, thanks for waiting!
It should not escape our attention that this ToolRunner could also be surfaced as a Target, to be used by adhoc_tool and code_quality_tool rather than each specifying all these fields together. It would also help to reduce confusion when handling all the kinds of 'dependencies' arguments that adhoc_tool takes.
Yeah, I agree :) I think it matches with how I've noticed people interpreting adhoc_tool
too: "here's a prepackaged command that could be invoked with pants run ...
(or similar)". That is to say, it feels like that's potentially a very useful building block 👍
(I hit the "update branch" button to trigger a full CI rerun with latest code, since it's been so long, to avoid for "merge skew".) |
… to target location (#20581) This PR fixes #20575 by adjusting the shared infrastructure for `adhoc_tool` and `code_quality_tool` to resolve the `execution_dependencies` field relative to the `adhoc_tool`/`code_quality_tool` target location, rather than relative to the `runnable=...` arg. I think this was a minor typo in #20255, and we didn't have tests covering it. I imagine it's common that the runnable and `..._tool` targets are often in the same BUILD file (in which case the behaviour is the same either way), but it's not impossible to have them be different (e.g. my work repo has a a few shared runnable that are used by more than one `adhoc_tool`). I think being relative to the target is easier to reason about, and this was the old behaviour of `adhoc_tool`. This PR also adds tests to confirm the behaviour of `execution_dependencies` (including its relative-path resolution behaviour), as well as the behaviour of `runnable_dependencies` while I'm there.
… to target location (#20581) This PR fixes #20575 by adjusting the shared infrastructure for `adhoc_tool` and `code_quality_tool` to resolve the `execution_dependencies` field relative to the `adhoc_tool`/`code_quality_tool` target location, rather than relative to the `runnable=...` arg. I think this was a minor typo in #20255, and we didn't have tests covering it. I imagine it's common that the runnable and `..._tool` targets are often in the same BUILD file (in which case the behaviour is the same either way), but it's not impossible to have them be different (e.g. my work repo has a a few shared runnable that are used by more than one `adhoc_tool`). I think being relative to the target is easier to reason about, and this was the old behaviour of `adhoc_tool`. This PR also adds tests to confirm the behaviour of `execution_dependencies` (including its relative-path resolution behaviour), as well as the behaviour of `runnable_dependencies` while I'm there.
… to target location (Cherry-pick of #20581) (#20608) This PR fixes #20575 by adjusting the shared infrastructure for `adhoc_tool` and `code_quality_tool` to resolve the `execution_dependencies` field relative to the `adhoc_tool`/`code_quality_tool` target location, rather than relative to the `runnable=...` arg. I think this was a minor typo in #20255, and we didn't have tests covering it. I imagine it's common that the runnable and `..._tool` targets are often in the same BUILD file (in which case the behaviour is the same either way), but it's not impossible to have them be different (e.g. my work repo has a a few shared runnable that are used by more than one `adhoc_tool`). I think being relative to the target is easier to reason about, and this was the old behaviour of `adhoc_tool`. This PR also adds tests to confirm the behaviour of `execution_dependencies` (including its relative-path resolution behaviour), as well as the behaviour of `runnable_dependencies` while I'm there. Co-authored-by: Huon Wilson <huon@exoflare.io>
Before moving to step 2 of the plan described in #17729 (comment) , cleaning up a gross duplication of rule code that I introduced in #20135 between
adhoc_tool
and the newcode_quality_tool
.This PR extracts the shared logic into the concept of a ToolRunner and a rule to hydrate it in
adhoc_process_support
.Both
adhoc_tool
andcode_quality_tool
have the latent idea of a tool runner and a considerable machinery to build it. Starting from something likethey need to assemble things like locate the actual runnable by str and figure out what should be its base digest, args, env, etc. and also co-locate the execution and runnable dependencies. We now capture that information as a "runner":
After this,
adhoc_tool
andcode_quality_tool
diverge in what they do with it.adhoc_tool
uses this runner to generate code and code_quality_tool uses it to run batches of lint/fmt/fix on source files.Food for thought ...
It should not escape our attention that this
ToolRunner
could also be surfaced as a Target, to be used byadhoc_tool
andcode_quality_tool
rather than each specifying all these fields together. It would also help to reduce confusion when handling all the kinds of 'dependencies' arguments thatadhoc_tool
takes.