-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Make get-poetry.py fallback to standard executables #2547
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -197,8 +197,7 @@ def expanduser(path): | |
POETRY_LIB_BACKUP = os.path.join(POETRY_HOME, "lib-backup") | ||
|
||
|
||
BIN = """#!/usr/bin/env python | ||
# -*- coding: utf-8 -*- | ||
BIN = """# -*- coding: utf-8 -*- | ||
import glob | ||
import sys | ||
import os | ||
|
@@ -217,7 +216,7 @@ def expanduser(path): | |
main() | ||
""" | ||
|
||
BAT = u('@echo off\r\npython "{poetry_bin}" %*\r\n') | ||
BAT = u('@echo off\r\n{python_executable} "{poetry_bin}" %*\r\n') | ||
|
||
|
||
PRE_MESSAGE = """# Welcome to {poetry}! | ||
|
@@ -589,23 +588,63 @@ def _make_lib(self, version): | |
finally: | ||
gz.close() | ||
|
||
def _which_python(self): | ||
"""Decides which python executable we'll embed in the launcher script.""" | ||
allowed_executables = ["python", "python3"] | ||
if WINDOWS: | ||
allowed_executables += ["py.exe -3", "py.exe -2"] | ||
|
||
# \d in regex ensures we can convert to int later | ||
version_matcher = re.compile(r"^Python (?P<major>\d+)\.(?P<minor>\d+)\..+$") | ||
fallback = None | ||
for executable in allowed_executables: | ||
try: | ||
raw_version = subprocess.check_output( | ||
executable + " --version", stderr=subprocess.STDOUT, shell=True | ||
).decode("utf-8") | ||
except subprocess.CalledProcessError: | ||
continue | ||
|
||
match = version_matcher.match(raw_version.strip()) | ||
if match: | ||
return executable | ||
|
||
if fallback is None: | ||
# keep this one as the fallback; it was the first valid executable we found. | ||
fallback = executable | ||
|
||
if fallback is None: | ||
raise RuntimeError( | ||
"No python executable found in shell environment. Tried: " | ||
+ str(allowed_executables) | ||
) | ||
|
||
return fallback | ||
|
||
def make_bin(self): | ||
if not os.path.exists(POETRY_BIN): | ||
os.mkdir(POETRY_BIN, 0o755) | ||
|
||
python_executable = self._which_python() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still don't understand the rationnal behind searching ourself for python executable while we are running one. In which situation will |
||
|
||
if WINDOWS: | ||
with open(os.path.join(POETRY_BIN, "poetry.bat"), "w") as f: | ||
f.write( | ||
u( | ||
BAT.format( | ||
python_executable=python_executable, | ||
poetry_bin=os.path.join(POETRY_BIN, "poetry").replace( | ||
os.environ["USERPROFILE"], "%USERPROFILE%" | ||
) | ||
), | ||
) | ||
) | ||
) | ||
|
||
with open(os.path.join(POETRY_BIN, "poetry"), "w", encoding="utf-8") as f: | ||
if WINDOWS: | ||
python_executable = "python" | ||
|
||
f.write(u("#!/usr/bin/env {}\n".format(python_executable))) | ||
f.write(u(BIN)) | ||
|
||
if not WINDOWS: | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdispater - the fact that this prefers python 2 is really unfortunate :-(
Any chance it could look for the newest python first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python
should link to your preferred python version. So it's absolutely understandable that poetry will pick up this one. If yourpython
doesn't link to your preferred python version, please look up how you can change it in your distribution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't buy it.
/usr/bin/python
is python2 for compatibility. Changing this on released distro is wrong. It will never happen. This behaviour prevent using poetry on python3 with most distributions.What's the point of having
python3 get-poetry.py
installing topython2
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering how your comment comply with https://www.python.org/dev/peps/pep-0394 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where it violates PEP394?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdispater actually, the point is not falling back to
python
but simply install poetry for the interpreter running get-poetry. Just likepip
installs a package for the interpreter running pip.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So does
get-pip.py
by default.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bersace The point of the official installer is to pick up the currently activate Python executable so you can easily switch between Python versions without having to install Poetry for each version. If we were to to hardcode the executable used to install Poetry this would defeat this purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By currently activated Python, do you mean pyenv or poetry-managed project python interpreter in a virtualenv ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdispater - what kind of breaking were you seeing when looking for
python3
first?How would use use the
env use
command when runningget-poetry.py
?@bersace - I'm afraid @sdispater may well means
pyenv
. It's such an unfortunate choice to recommend as it's a terrible tool in professionally managed environments wherepyenv
either fails completely because development headers aren't available, or it builds broken python binaries because development headers for "optional" modules likezlib
andncurses
aren't present, or perhaps worse still builds its own python binaries rather than using system-provided builds that have been optimised for the current environment by a sysadmin or build team.