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

Fix the detection of the system environment with custom installer #4433

Merged
merged 6 commits into from
Sep 13, 2021

Conversation

sdispater
Copy link
Member

@sdispater sdispater commented Aug 24, 2021

After several attempts at fixing this, I think this is the best approach: differentiate the custom Poetry virtual environment – by putting a special file – from standard ones so that there is no confusion about what Poetry needs to do when detecting the system environment.

Resolves #4427
Resolves #4414
Resolves #4369
Resolves #4254

Pull Request Check List

  • Added tests for changed code.
  • Updated documentation for changed code.

@sdispater sdispater added kind/bug Something isn't working as expected area/venv Related to virtualenv management labels Aug 24, 2021
@sdispater sdispater added this to the 1.2 milestone Aug 24, 2021
@sdispater sdispater force-pushed the revert-system-env-changes branch 4 times, most recently from 0aec865 to af920c5 Compare August 27, 2021 12:40
Comment on lines 1097 to 1104
python_executables = sorted(
[
p.name
for p in self._bin_dir.glob("python*")
if re.match(r"python(?:\d+(?:\.\d+)?)?(?:\.exe)?$", p.name)
]
)
self._executable = python_executables[0].rstrip(".exe")
Copy link
Member

Choose a reason for hiding this comment

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

If I understand the code correct, it should work if there is only one python interpreter in the environment. But what happens if we have a python2, python3.6, python3.7? python2 will be selected. But we cannot be sure that this is the intended interpreter.

Can we make the following assumptions?:

  • in a venv there will be only on interpreter
  • in the system env there can be multiple interpreter

If so, we could look for python executables of the format pythonX.Y. If we receive exactly one, take it. If we get multiple, take the one with the same version as poetry is running.

BTW: rstrip() doesn't strip .exe from the end of the string. It removes any of the given character from the end until it hits a character not listed. A trap I run into regularly 😄

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 updated the code to look for the executable of the child environment (if given). Otherwise the previous logic is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that I intend to rewrite the environment management at some point but it will be after the release of the 1.2 version.

@datacubed
Copy link

How would I test this for myself ?
Is it going to be merged at some point by chance ?

@sdispater sdispater force-pushed the revert-system-env-changes branch 3 times, most recently from 963cfeb to 37b6c37 Compare September 13, 2021 22:17
@sdispater sdispater merged commit 61301a1 into master Sep 13, 2021
@sdispater sdispater deleted the revert-system-env-changes branch September 13, 2021 22:30
@datacubed
Copy link

hey @sdispater it looks like this is merged into master ? I used the latest curl -sSL https://raw.githubusercontent.com/python-poetry/poetry/master/install-poetry.py | python3.7 (from the release notes page, not the main site's documentation)
but im still getting the error where poetry cant find "python" though it should be using "python3" ?

@finswimmer
Copy link
Member

Hey @datacubed,

the install-poetry.py script always installs the latest stable release. To install from master use https://raw.githubusercontent.com/python-poetry/poetry/master/install-poetry.py | python3.7 - --git https://github.com/python-poetry/poetry.git@master instead.

fin swimmer

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/venv Related to virtualenv management kind/bug Something isn't working as expected
Projects
None yet
3 participants