Skip to content
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

Move the pants-pyenv-install target to a sub-plugin #18499

Merged
merged 1 commit into from
Mar 15, 2023

Conversation

thejcannon
Copy link
Member

@thejcannon thejcannon commented Mar 14, 2023

This was leftover from the prior PR, but split the magic into an opt-in plugin. I also added tests for it, yay.

Expected usage (which will be documented) is:
ENVVAR1=blah pants --concurrent --backend-packages=pants.backend.python.providers.experimental.pyenv.custom_install run :pants-pyenv-install -- 3.9

@thejcannon thejcannon added needs-cherrypick category:internal CI, fixes for not-yet-released features, etc. labels Mar 14, 2023
@thejcannon thejcannon added this to the 2.16.x milestone Mar 14, 2023
@thejcannon thejcannon requested review from stuhood and kaos March 14, 2023 19:55
@stuhood
Copy link
Member

stuhood commented Mar 15, 2023

Thanks for deferring this from #18352, as I continue to think this implementation is a bit contorted.

As mentioned in #18352 (comment), I think that providing a way to pass environment variables / flags to pyenv via options would:

  1. allow for some common non-optimization use cases like declaring where openssl lives
  2. allow for hermeticity of options, so that rather than relying on a user running commands in a particular order to mutate their cache, if they have an option set (perhaps via pantsrc) and a new python version comes in in pants.toml, their options are reapplied and the interpreter is rebuilt automatically
  3. be less magical: setting an option leaves behind evidence that something is different in a user's environment. This current implementation requires a seperate plugin, and special documentation... whereas an option would be self explanatory.
  4. be easier to implement:
    • hermeticity of options values would involve templating the options into the generated pyenv script so that its digest changed, and then nesting the named cache under the digest of the script (or under a direct hash of the options string, if you choose)

As mentioned elsewhere, I won't block though.

@thejcannon
Copy link
Member Author

  1. is definitely a use-case we should solve with options. 💯 on that one.
  2. Is also one that's probably worth solving for, for simplicity sake.

4 I'm skeptical about. I'm very weary about unecessary re-installs (especially since this can't be cached), especially if a 30-minute install process is at stake.
Additionally, we haven't addressed the issue of "maybe a 30-minute install"-but-only-when-needed issue. That's about as bad UX as it gets 😂

I think in the end, I don't see a way around giving the user a level to pull on their own, when ready, to prime their cache in such a timesink way.

@stuhood
Copy link
Member

stuhood commented Mar 15, 2023

Additionally, we haven't addressed the issue of "maybe a 30-minute install"-but-only-when-needed issue. That's about as bad UX as it gets 😂

I think in the end, I don't see a way around giving the user a level to pull on their own, when ready, to prime their cache in such a timesink way.

How would a user know that they need to re-run the magical step after a Pants version or Python version change had occurred? Their changes would just disappear.

@thejcannon
Copy link
Member Author

Yeah OK the "I always want optimized Python" people aren't completely solved here. It's helpful to have this lever to pull, but it isn't obvious when it needs to be pulled.

My surface level thought here is maybe an extra option for "error and bail if you would install Python, I wanna do it myself". I'll think on it though.

@thejcannon thejcannon merged commit 3b96362 into pantsbuild:main Mar 15, 2023
@thejcannon thejcannon deleted the pyenv-extra branch March 15, 2023 18:07
@stuhood stuhood mentioned this pull request Mar 24, 2023
thejcannon added a commit to thejcannon/pants that referenced this pull request Apr 14, 2023
This was leftover from the prior PR, but split the magic into an opt-in
plugin. I also added tests for it, yay.

Expected usage (which will be documented) is:
`ENVVAR1=blah pants --concurrent
--backend-packages=pants.backend.python.providers.experimental.pyenv.custom_install
run :pants-pyenv-install -- 3.9`
thejcannon added a commit that referenced this pull request Apr 14, 2023
…18499) (#18744)

This was leftover from the prior PR, but split the magic into an opt-in
plugin. I also added tests for it, yay.

Expected usage (which will be documented) is:
`ENVVAR1=blah pants --concurrent
--backend-packages=pants.backend.python.providers.experimental.pyenv.custom_install
run :pants-pyenv-install -- 3.9`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants