-
Notifications
You must be signed in to change notification settings - Fork 400
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
[feature] Download/use standalone python build when chose --python version doesn't exist #1243
Conversation
1ead70b
to
97cadfc
Compare
@dukecat0 @uranusjr I've got a preliminary POC of this feature which I'm quite happy with, but before I sink any more time into this, I'm hoping to get some feedback and make sure I'm on the right track. Is this something you would like me to keep working on? do you have any notes on my implementation? |
Thanks for working on this! I personally find this feature very useful. 👍 Probably it will be better if there's an environment variable that allows users to disable this behavior. |
Looks like a helpful feature, thanks!
I agree with that – or even a flag to toggle the behaviour. |
Good stuff! I would argue that this feature should be opt-in, rather than opt-out. I would be very confused if I accidentally mistype a python version and pipx just starts downloading it unprompted. |
I agree – that's why I suggest a flag (with negative default value). |
Excellent idea! I’m more than happy to implement that. Not sure what to call the flag though… something succinct? ideally also an environment variable to flip the default maybe something like |
1
|
Please add a news fragment, too. And we'll need tests. |
Hi All, this feature is now, to the best of my ability, complete. I've added tests, a news entry, and adopted / implemented just about every change that's been requested. The tests are all now passing on all platforms and python versions. I've spent a not insignificant amount of time and energy making this feature implementation as good as it can be in the hopes that it will be accepted and merged. I am running out of time and energy to devote to this now. I think it's ready to review and/or merge. I'm going to step away from it now. If there are any additional changes you would like to make, feel free to make them before or after merging the feature. Thank you |
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 can work on my change requests myself.
Would be nice if there was a command to clean up the python versions, e.g. when a downloaded python version does not have a installed app that uses it anymore. We can treat that as a separate feature request though IMO. |
Also a command to update one or all standalone versions (ie someone uses That's a hard problem to solve automatically, so i think a manual process / command would be the way to go |
This latest error in the windows unit test is... interesting, to say the least. is I'm not quite sure how else to explain why |
Probably we should use pytest's |
to override the I suppose so, but that somehow feels wrong. the unit test should as closely as possible reflect what will actually happen on a user's machine ordinarily, this error we're seeing should be impossible. if standalone python 3.11's already been downloaded, the function should return early without trying to redownload it. this error seems to indicate that a previous run failed and produced an empy |
4cbba39
to
7380938
Compare
Well, apart from that rate limit error on the windows test, I think we're now good to go. Are there any additional changes you need, or can this be merged? |
I'd like to raise awareness for #1243 (comment) that should still be implemented IMHO, as it tests basic functionality. As for the rate limit, I see that as a possible issue, as these tests will be run many times. I propose skipping this test if |
The first suggestion from that comment (ensuring the target python isn't found on dev's machines) has already been implemented (by mocking out The second comment, adding a |
regarding the rate limit, that is definitely a concern. I'm going to try and write a fixture to mock out the API call specifically, that should take care of that problem stand by |
However, implementing it would ensure the exception is raised correctly, which isn't entirely useless. |
7380938
to
c3f92ee
Compare
for more information, see https://pre-commit.ci
oh, i see, you're suggesting i write a test for the failure mode. gotcha, will do |
9fb8ea0
to
00cf6a7
Compare
hi @chrysle, I've implemented test cases for versions of python which aren't available in the python-build-standalone project in the future, we might want to expand the logic of the |
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.
Two suggestions, looks good to me other than that!
Co-authored-by: chrysle <[email protected]>
Thank you for all the work you have invested into this contribution!
Additional concerns, as a |
Thank you for bearing with us through the review process @alextremblay. Great contribution! We also moved logging the exceptions from the interpreter resolution outside of the scope of this issue, so that also needs a follow-up. |
changelog.d/
(if the patch affects the end users)Summary of changes
As per #1242, this PR updates
pipx.interpreter.find_python_interpreter
to download the desired python version ifpipx
is not able to find it in the system, rather than simply failing with an exception.Test plan
Tested by running
The following was also tested:
myscript.py
: