Adding ASDF support to interpreter-search-paths #12028
Conversation
cjntaylor
force-pushed
the
asdf
branch
2 times, most recently
from
3aa364c to
641d7c5
Compare
There was a problem hiding this comment.
Hey @cjntaylor, pardon for the really long delay on this! The past two weeks were really busy with Pycon.
Thank you for the contribution!
|
@cjntaylor this PR is great! We're looking to also use asdf with Pants as well. @chipola and I would be happy to help implement feedback if needed. |
There was a problem hiding this comment.
Sorry for the huge delay all - I got called away to a rather complicated project for work and I just now resurfaced. I made the adjustments you requested @Eric-Arellano, and I rebased this branch up to the current master just to make things current. Everything you suggested was reasonable, with the exception of a few things I left open below. Let me know what you think.
Thanks for your help 😄
Yay! Great to know that there are others out there who are also part of the ASDF user community. It's helpful to motivate that I'm not an isolated case 😅 Apologies for the delay, and thanks for the offer of help - I think we're good to go on implementation but I'll definitely take you up on that in the future 👍. It's really nice to get actual feedback and eager participants over just a bunch of +1's for once 😁 |
| @@ -49,6 +53,57 @@ def fake_pyenv_root(fake_versions, fake_local_version): | |||
| yield pyenv_root, fake_version_dirs, fake_local_version_dirs | |||
|
|
|||
|
|
|||
| def materialize_indices(sequence: Sequence[_T], indices: Iterable[int]) -> Tuple[_T, ...]: | |||
There was a problem hiding this comment.
This would be better as an inner function defined inside fake_asdf_root. It's only used there currently, and would help with namespacing.
There was a problem hiding this comment.
It's actually also used in test_get_asdf_paths and test_expand_interpreter_search_paths as well. I believe that's why it was pulled out in the first place (it's a tiny helper function; normally I would have done exactly as you're suggesting - just writtten it as an inner function instead).
cjntaylor
force-pushed
the
asdf
branch
4 times, most recently
from
5cebed9 to
4c7803f
Compare
|
I fixed the issue with the typing. I'll try to remember in the future I need to run |
We went with this design in large part to facilitate that you normally want to run |
| @@ -255,6 +391,16 @@ def get_pyenv_paths(env: Environment, *, pyenv_local: bool = False) -> List[str] | |||
| return paths | |||
|
|
|||
|
|
|||
| def get_asdf_dir(env: Environment) -> PurePath | None: | |||
| """See https://asdf-vm.com/#/core-configuration?id=environment-variables.""" | |||
| asdf_dir = env.get("ASDF_DIR", env.get("ASDF_DATA_DIR")) | |||
There was a problem hiding this comment.
These environment variables have not been include-listed into the Environment that will be exposed to the @rules which call PythonSetup.interpreter_search_paths: the primary caller we care about is find_pex_pythons:
pants/src/python/pants/backend/python/util_rules/pex_environment.py
Lines 134 to 136 in b0b7b0b
Without doing so, this code will never observe those variables, because they will be filtered out.
There was a problem hiding this comment.
Unless I've misunderstood, I've added the requested variables to the whitelist so that it will function as expected. Is there more that needs to be done?
There was a problem hiding this comment.
Not that I know of off the top of my head, but: actually trying it on your machine and confirming that you're seeing it working is your best bet. I don't have ASDF installed (yet), and it doesn't seem worth the trouble to bootstrap it in your integration tests (yet).
Adding [ASDF](https://asdf-vm.com) support to the python interpreter-search-paths option, through an additional special strings (<ASDF>, <ASDF_LOCAL>). ASDF actually wraps pyenv (and python-build) internally, but relocates and adapts its layout and configuration so it conforms to the standards it employs. The implementation avoids calling ASDF directly as it requires heavy use of an agumented shell (source $ASDF_DIR/asdf.sh) and a supported interpreter. Instead, it minimally re-implements the configuration and layout algorithm used by the tool to find the direct path to python interpreters, based on its standard mode of operation. This is to say, the implementation is ASDF "compatible", but may need adaptation as the tool and python plugin change over time (although the basic principles used are very unlikely to change in any way that would affect functionality). ASDF does provide support for many other languages/interpreters; additional tools could be added in the future (how they should be integrated, and how that would affect this plugin is TBD). [ci skip-rust] [ci skip-build-wheels]
|
Thanks for the contribution @cjntaylor ! This will go out into the upcoming 2.7 release on Friday. |
Adding ASDF (https://asdf-vm.com) support to the python interpreter-search-paths option, through an additional special strings (
<ASDF>,<ASDF_LOCAL>). ASDF wraps pyenv (and python-build) internally, but relocates and adapts its layout and configuration so it conforms to the standards it employs.The implementation avoids calling ASDF directly as it requires heavy use of an agumented shell (source $ASDF_DIR/asdf.sh) and a supported interpreter. Instead, it minimally re-implements the configuration and layout algorithm used by the tool to find the direct path to python interpreters, based on its standard mode of operation. This is to say, the implementation is ASDF "compatible", but may need adaptation as the tool and python plugin change over time (although the basic principles used are very unlikely to change in any way that would affect functionality).
ASDF does provide support for many other languages/interpreters; additional tools could be added in the future (how they should be integrated, and how that would affect this plugin is TBD).