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

Add source interpreter to pipx metadata #1251

Merged
merged 8 commits into from
Feb 13, 2024

Conversation

Gitznik
Copy link
Contributor

@Gitznik Gitznik commented Feb 7, 2024

  • I have added a news fragment under changelog.d/ (if the patch affects the end users)

Summary of changes

I added the source interpreter of a venv to the pipx metadata in preparation of #1248 and #1249. We need this to be able to find out if a standalone interpreter is still being used.

I've done it this way following the logic of the python_version part of the pipx metadata. Is there any reason we don't use the pyvenv.cfg for obtaining this information? It has the python version and the source interpreter in it, and it has been stable since python 3.3 AFAIK.

I have not added a news fragment yet, as IMO this is part of #1243

Test plan

Tested by running

pipx install --fetch-missing-python --python { a python version you don't have} pycowsay
pipx list

@Gitznik
Copy link
Contributor Author

Gitznik commented Feb 7, 2024

FYI @chrysle and @alextremblay, it seems the standalone python downloaded on windows right now can't actually be executed. See the windows test workflow. I've added executing it to the test to validate this. Did you come across this in your tests @alextremblay?

@chrysle for us this means we can't release this feature just yet.

@alextremblay
Copy link
Contributor

FYI @chrysle and @alextremblay, it seems the standalone python downloaded on windows right now can't actually be executed. See the windows test workflow. I've added executing it to the test to validate this. Did you come across this in your tests @alextremblay?

@chrysle for us this means we can't release this feature just yet.

oh dear, that's unfortunate

as i mentioned in the PR for standalone python, i don't have a windows machine to test with, and i don't have much experience developing python projects in windows

maybe i can get a windows VM spun up for development or something... that might take me a few days

@Gitznik
Copy link
Contributor Author

Gitznik commented Feb 8, 2024

as i mentioned in the PR for standalone python, i don't have a windows machine to test with, and i don't have much experience developing python projects in windows

maybe i can get a windows VM spun up for development or something... that might take me a few days

Same here 👍

@Gitznik
Copy link
Contributor Author

Gitznik commented Feb 8, 2024

Great news. The installed_python path was wrong on windows. This is fixed now and ready for review.

@Gitznik Gitznik marked this pull request as ready for review February 8, 2024 17:55
@chrysle
Copy link
Contributor

chrysle commented Feb 8, 2024

Thanks for catching! You had a look at the source tarball, right?

@Gitznik
Copy link
Contributor Author

Gitznik commented Feb 8, 2024

Thanks for catching! You had a look at the source tarball, right?

I actually setup a windows VM and tried it out there.

@chrysle
Copy link
Contributor

chrysle commented Feb 8, 2024

I actually setup a windows VM and tried it out there.

Well, I'm all for my way of debugging ;-)

$ tar tvf cpython-3.10.13+20240107-x86_64-pc-windows-msvc-shared-install_only.tar.gz | grep "python.exe"
-rwxrwxrwx root/root    255488 2021-01-01 09:00 python/Lib/venv/scripts/nt/python.exe
-rwxrwxrwx root/root     91136 2021-01-01 09:00 python/python.exe
$ tar tvf cpython-3.10.13+20240107-x86_64_v2-unknown-linux-gnu-install_only.tar.gz | grep "bin/python"
lrwxrwxrwx root/root         0 2021-01-01 09:00 python/bin/python3 -> python3.10
lrwxrwxrwx root/root         0 2021-01-01 09:00 python/bin/python3-config -> python3.10-config
-rwxrwxr-x root/root     20944 2021-01-01 09:00 python/bin/python3.10
-rwxrwxr-x root/root      3130 2021-01-01 09:00 python/bin/python3.10-config

Will review shortly.

@Gitznik
Copy link
Contributor Author

Gitznik commented Feb 8, 2024

Well, I'm all for my way of debugging ;-)

$ tar tvf cpython-3.10.13+20240107-x86_64-pc-windows-msvc-shared-install_only.tar.gz | grep "python.exe"
-rwxrwxrwx root/root    255488 2021-01-01 09:00 python/Lib/venv/scripts/nt/python.exe
-rwxrwxrwx root/root     91136 2021-01-01 09:00 python/python.exe
$ tar tvf cpython-3.10.13+20240107-x86_64_v2-unknown-linux-gnu-install_only.tar.gz | grep "bin/python"
lrwxrwxrwx root/root         0 2021-01-01 09:00 python/bin/python3 -> python3.10
lrwxrwxrwx root/root         0 2021-01-01 09:00 python/bin/python3-config -> python3.10-config
-rwxrwxr-x root/root     20944 2021-01-01 09:00 python/bin/python3.10
-rwxrwxr-x root/root      3130 2021-01-01 09:00 python/bin/python3.10-config

Will review shortly.

Yeah that would've been probably the way more efficient approach 😅 ah well, now I have a windows VM for debugging.

Copy link
Contributor

@chrysle chrysle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add a newsfragment for the bugfix.

src/pipx/pipx_metadata_file.py Outdated Show resolved Hide resolved
src/pipx/pipx_metadata_file.py Outdated Show resolved Hide resolved
src/pipx/venv.py Outdated Show resolved Hide resolved
tests/test_interpreter.py Outdated Show resolved Hide resolved
src/pipx/pipx_metadata_file.py Show resolved Hide resolved
@Gitznik
Copy link
Contributor Author

Gitznik commented Feb 8, 2024

Thanks for the review.

Please also add a newsfragment for the bugfix.

Do you reckon we need a news fragment for the bugfix? The bug was never released, so it's not really a change for the user.

Also what do you think about using the pyvenv.cfg to obtain the source interpreter, instead of writing it to our own metadata? I just didn't do it because it's not done for the python version either, but I don't know why it wasn't done there.

@chrysle
Copy link
Contributor

chrysle commented Feb 12, 2024

Do you reckon we need a news fragment for the bugfix? The bug was never released, so it's not really a change for the user.

That's true, didn't think of it.

Also what do you think about using the pyvenv.cfg to obtain the source interpreter, instead of writing it to our own metadata? I just didn't do it because it's not done for the python version either, but I don't know why it wasn't done there.

Well, the executable key to pyvenv.cfg was added in Python 3.11 (changelog). So I think we should delay switching the behaviour until then.

By the way, you'll need to update helpers.py.

@Gitznik Gitznik requested a review from chrysle February 12, 2024 20:59
tests/helpers.py Outdated Show resolved Hide resolved
Co-authored-by: chrysle <fritzihab@posteo.de>
@Gitznik Gitznik requested a review from chrysle February 13, 2024 07:39
@Gitznik Gitznik merged commit 1e5d03b into pypa:main Feb 13, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants