Skip to content

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

Merged
gaborbernat merged 12 commits into
pypa:mainfrom
flying-sheep:discover-more
Apr 23, 2024
Merged

Allow builtin interpreter discovery to find specific Python versions given a general spec#2709
gaborbernat merged 12 commits into
pypa:mainfrom
flying-sheep:discover-more

Conversation

@flying-sheep

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

Copy link
Copy Markdown
Member

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

@gaborbernat gaborbernat left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member 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
Copy Markdown
Member 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
Copy Markdown
Contributor

Seems you've upset the CI 😅

@flying-sheep

flying-sheep commented Apr 23, 2024

Copy link
Copy Markdown
Member Author

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

gaborbernat commented Apr 23, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member 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
Copy Markdown
Member 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

flying-sheep commented Apr 23, 2024

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Member 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
Copy Markdown
Member 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

2 participants