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

Allow builtin interpreter discovery to find specific Python versions given a general spec #2709

Merged
merged 12 commits into from
Apr 23, 2024

Conversation

flying-sheep
Copy link
Contributor

@flying-sheep flying-sheep commented Apr 23, 2024

Thanks for contributing, make sure you address all the checklists (for details on how see development documentation)

  • ran the linter to address style issues (tox -e fix)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

Fixes #2708, fixes pypa/hatch#1395

This PR fixes the discovery in virtualenv.discovery.builtin.

It works by removing the method PythonSpec.generate_names and replacing it with generate_re:

A static name list can’t represent all Python executables that are compatible with a given spec, i.e. PythonSpec.from_string_spec('3') should match a executable called /usr/bin/python3.12. I therefore inverted the way candidates are found. Instead of generating all possible candidates, we simply loop and use a regex to find candidates.

Pro:

  • Fixes the bug
  • Instead of just trying all-lowercase and all-uppercase variants of candidates, we’re now truly case-insensitive

Con:

  • Might be slower if a lot of executables hang out on PATH. Since Python’s glob just traverses directories in Python code as well, it wouldn‘t help here.

How to review

The main commits are

  • f29be4a: Fixes the tox configuration so it can cope with spaces. Allows me to run tests on my system. Humanity needs to stop interpolating strings into shell scripts.
  • 6f9f270: Adds typing to the file I’m going to edit so I can make sense of what’s going on
  • 16a6e1d: Switch that file to pathlib, since you seem to be doing that elsewhere and it makes things more readable
  • d9b52ad: The actual changes, described above

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Tests please 🥺

@flying-sheep
Copy link
Contributor Author

On it! It’s a draft so you can look at it if you want while I check those last boxes!

@flying-sheep flying-sheep marked this pull request as ready for review April 23, 2024 13:51
@flying-sheep
Copy link
Contributor Author

Now I’m done!

Seems like this change snuck into one of the tests:

-    pyvenv_cfg = Path(sys.executable).parents[1] / "pyvenv.cfg"
-    if pyvenv_cfg.exists():
-        (target / pyvenv_cfg.name).write_bytes(pyvenv_cfg.read_bytes())

that fragment doesn’t seem to be necessary to run the test locally?

gaborbernat
gaborbernat previously approved these changes Apr 23, 2024
@gaborbernat
Copy link
Contributor

Seems you've upset the CI 😅

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Apr 23, 2024

I guess I need to try this on macOS. No idea what to do if I can’t placate windows though …

Seems like it works in practice though!

  1. create project requiring Python 3.12 on a system where /usr/bin/python is 3.11:

    $ cd some-project
    $ rg requires-python pyproject.toml 
    4:requires-python = ">=3.12"
  2. try finding an interpreter with hatch:

    $ pipx install hatch
    $ hatch env create
    Environment `default` is incompatible: no compatible Python distribution available
  3. try my patch (no clue why virtualenv doesn’s:

    $ ~/.local/pipx/venvs/hatch/bin/python -m pip install 'virtualenv@ git+https://github.com/flying-sheep/virtualenv.git@discover-more'
    […]
    Successfully installed virtualenv-20.25.4.dev11+g66d9408
    $ hatch env create

    no output means success!

@gaborbernat
Copy link
Contributor

gaborbernat commented Apr 23, 2024

Huh, tests run fine locally … let’s see.

I cannot merge the pull request until the CI is green so it doesn't really matter that "it works on my machine" 😅. Sorry but the CI gods are absolute. 😭

@flying-sheep
Copy link
Contributor Author

Haha I’m a believer, don’t worry. It’s great that you test on windows and macOS too, as clearly with finnicky path stuff like this, that’s an issue. I’ll see what I can do.

@flying-sheep flying-sheep changed the title Discover-more Allow builtin interpreter discovery to find specific Python versions given a general spec Apr 23, 2024
@flying-sheep
Copy link
Contributor Author

Ah, the regex always needs to be case insensitive to get the intended effect. I tested locally on macOS and Linux now, so I hope windows doesn’t throw a wrench.

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Apr 23, 2024

OK, as feared everything passes except for on windows. I can only speculate that the pyvenv.cfg stuff was load bearing on windows?

If that’s not it, someone with a windows machine needs to help.

/edit: e.g. @ofek, who just offered

gaborbernat
gaborbernat previously approved these changes Apr 23, 2024
@flying-sheep
Copy link
Contributor Author

Looks good! I don’t think this timeout is caused by my code

@gaborbernat gaborbernat enabled auto-merge (squash) April 23, 2024 15:35
@flying-sheep
Copy link
Contributor Author

OK, pypy on windows fails with

125251 get interpreter info via cmd: 'C:\Users\runneradmin\AppData\Local\Temp\pytest-of-unknown\pytest-0\test_discovery_via_path_specif0\Scripts\somethingVeryCryptic3.10.exe' 'D:\a\virtualenv\virtualenv\.tox\pypy310\lib\site-packages\virtualenv\discovery\py_info.py' 2o7zKhwQ7RmUZlP1L7qYsRzWDWJd2Uc0 266eD4ebldJtJex0LCwbXGC040ZIuScj [DEBUG cached_py_info:112]
125280 failed to query C:\Users\runneradmin\AppData\Local\Temp\pytest-of-unknown\pytest-0\test_discovery_via_path_specif0\Scripts\somethingVeryCryptic3.10.exe with code 3221225781 [INFO cached_py_info:34]

I have absolutely no idea what “125280 failed to query …exe with code 3221225781” means, so I don’t think I can proceed here.

auto-merge was automatically disabled April 23, 2024 15:50

Head branch was pushed to by a user without write access

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants