-
-
Notifications
You must be signed in to change notification settings - Fork 646
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
[internal] refactor Python ASDF support into a non-tool-specific rule #16202
[internal] refactor Python ASDF support into a non-tool-specific rule #16202
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.
Thanks a lot!
@rule | ||
async def resolve_asdf_tool_paths( | ||
request: AsdfToolPathsRequest, build_root: BuildRoot | ||
) -> AsdfToolPathsResult: |
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.
Given the direct HOME and path access in the @rule_helper
, can you mark this @_uncacheable_rule
? I think that it's been a longstanding issue that ASDF edits won't invalidate correctly.
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.
Is there those path checks could use PathGlobs
or an equivalent? I tried using PathGlobs in an earlier iteration of this PR to read .tool-versions
, but the default --pants-ignore
option ignores dot files in the build root.
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.
(Any fix to be able to use PathGlobs
would be for the future, not this PR.)
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.
I don't think it would be possible to use PathGlobs
for this, because some of these file paths are absolute (in $HOME
). But labeling with a TODO that mentions #10842 would be good probably.
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.
Right, I had only tried it for the $BUILD_ROOT/.tool-versions
path. Will do on TODO.
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
d5988d3
to
e87c9b8
Compare
…pantsbuild#16202) Refactor Python ASDF into a non-tool-specific rule in anticipation of adding golang support. [ci skip-rust] [ci skip-build-wheels]
Refactor Python ASDF into a non-tool-specific rule in anticipation of adding golang support.
[ci skip-rust]
[ci skip-build-wheels]