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

refactor(env_manager): split out python detection #9050

Merged
merged 8 commits into from
Sep 23, 2024

Conversation

finswimmer
Copy link
Member

@finswimmer finswimmer commented Feb 27, 2024

At the moment the env_manager is more or less a big ball of mud, mainly due to violating the single responsible principle. Beside the original purpose - handling environments - it also includes lot of logic to find the desired python interpreter.

This PR refactor the env_manager by split out the logic for finding the path to the desired python interpreter.

Copy link
Member

@Secrus Secrus left a comment

Choose a reason for hiding this comment

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

Looks fine initially. What I thought about, accounting for the future plans, maybe we could make some kind of "backends" for python detection. Current way would be one backend and future pythonfinder/findpython solution would be another one. That way we could transition the ways for python detection similar to how we handle package installation with "modern installer" and pip-based.

src/poetry/utils/env/python_manager.py Show resolved Hide resolved
src/poetry/utils/env/python_manager.py Outdated Show resolved Hide resolved
@finswimmer finswimmer force-pushed the rework-python-detection-2 branch 7 times, most recently from bfaa7db to abb4d65 Compare March 4, 2024 18:44
@finswimmer finswimmer force-pushed the rework-python-detection-2 branch 5 times, most recently from 30cb460 to 3b70164 Compare March 12, 2024 20:36
@finswimmer finswimmer force-pushed the rework-python-detection-2 branch 9 times, most recently from dc6328c to f1a44bc Compare August 24, 2024 19:26
@finswimmer finswimmer marked this pull request as ready for review August 24, 2024 20:30
@finswimmer finswimmer requested review from Secrus and a team August 25, 2024 08:21
Copy link
Member

@Secrus Secrus left a comment

Choose a reason for hiding this comment

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

LGTM, some minor issues here and there

src/poetry/utils/env/python_manager.py Show resolved Hide resolved
src/poetry/utils/env/python_manager.py Show resolved Hide resolved
src/poetry/utils/env/python_manager.py Outdated Show resolved Hide resolved
src/poetry/utils/env/python_manager.py Outdated Show resolved Hide resolved
tests/utils/env/test_env_manager.py Show resolved Hide resolved
tests/utils/test_python_manager.py Show resolved Hide resolved
Secrus
Secrus previously approved these changes Sep 16, 2024
Copy link
Member

@Secrus Secrus left a comment

Choose a reason for hiding this comment

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

LGTM, let's resolve last conversation and can be merged

assert active_python == wrapped_python


def test_python_find_compatible(project_factory: ProjectFactory) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

I just tried the tests on a Windows machine and noticed that this one is failing. I suppose it does not work with the official Python installer because there is no python3.x executable. Maybe, we should mock shutil.which so that it returns the path of the current Python - at least on Windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting finding 👍 I believe it's not a problem with the test. The test discovers a bug/suboptimal implementation.

The logic within get_compatible_python() assumes that there as an executable with a version number in the name (e.g. python3, python312). The assumption is obviously not correct. We didn't run into any trouble until now, because this logic is only triggered if the "system" or "preferred" python isn't compatible. So an executable python was checked before.

We could add python to the list of executables that are tried. But where? At the beginning or at the end?

Copy link
Member

Choose a reason for hiding this comment

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

We could add python to the list of executables that are tried. But where? At the beginning or at the end?

I like adding it but do not have a strong opinion where to add it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added it to the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test has a high chance of flaking on random Windows systems until we have #2117 resolved (i.e. sometime after this PR) since a 'python.exe' in the path on Windows has a reasonable chance of being the Store-launcher version (which we would hopefully reject if it was chosen...) or simply not existing at all.

We could also try 'py.exe' after 'python.exe' which would give you some PEP-514 version of Python, if possible. I just noticed on my machine that it picked up the Python 3.13.0rc1 experimental free-threaded build in preference to either the traditional 3.13.0rc1, or (released) 3.12, so yeah... it's a lexicographical version ordering. Surprise!

But I don't think we try 'py.exe' now, so I don't think that change would make sense in this refactoring anyway.

Is there value in marking this test "May fail due to system configuration on Windows, see #2117" or the like? (I am assuming that the implementation for #2117 will work for both py use env and get_compatible_python...)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but from what I understood, this PR is just refactoring, not bug fixing. If there are bugs that were present previously, we can take care of those in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's why my suggestion was to simply annotate the test, since it's a newly distinguished function in this refactoring, and the new test for it has flake-potential in rational-but-uncontrolled environments, and a possibly-related existing bug report where we can track and resolve (or at least improve) that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment as suggested.

Copy link
Contributor

@TBBle TBBle Sep 23, 2024

Choose a reason for hiding this comment

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

Just looking at the comment, I think it might have conflated a few things:

  • The Store-launcher that is bundled in Windows puts python.exe in the path. I expect that's rejected by logic that checks the version of the discovered Python binary, but would pop up the store UI in the meantime.
  • A store-installed Python will put functioning python.exe and python3.exe in the path (apparently, I didn't test this), causing this test to work correctly, and giving the most POSIX-like experience in this respect.
  • A python.org installer may or may not put python.exe in the path (not sure of the default off-hand, I always have it off personally), but definitely won't put python3.exe in the path.
  • A python.org installer would normally install a py.exe which behaves link the normal /usr/bin/python symlink on POSIX machines, as far as py -c goes, anyway.

So I think

    # Note: This test may fail on Windows systems using Python from the Microsoft Store,
    # as the executable is named `py.exe`, which is not currently recognized by
    # Python.get_compatible_python. This issue will be resolved in #2117.

maybe should be

    # Note: This test may fail on Windows systems using Python from the python.org installer,
    # as the included `python.exe` may not be on the PATH, and hence not seen by
    # Python.get_compatible_python. This issue will be resolved in #2117.
    # Note: On Windows systems with nothing else putting a `python.exe` in the PATH,
    # this test may open the Microsoft Store, see
    # https://learn.microsoft.com/windows/python/faqs#why-does-running-python-exe-open-the-microsoft-store-

(I added the second note because #2117 won't address that, so it'll have to be left behind when #2117 is done and the first note is removed. There's basically nothing we can do for that case except special case that exact executable, perhaps by full path in _full_python_path, and that's something we can fuss about with later if it's interesting.)

That said, looking at the rest of the comment, if we can guarantee that the Poetry-running Python executable will be selected in this case, maybe the comment could be more-succinctly summarised to just that, or even asserted somehow? Although with a quick look, I don't see how this happens? get_compatible_python doesn't appear to try the result of get_system_python before it goes searching the PATH with _full_python_path, so if our currently-executing python binary isn't in the PATH, it won't be found, I think.

@Secrus Secrus merged commit 37772f8 into python-poetry:main Sep 23, 2024
75 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.

4 participants