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

Make get-poetry.py fallback to standard executables #2547

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

sdispater
Copy link
Member

This is the proper version of #2426 after trying to fix it in #2535.

The changes made in #2426 (and #1878) can break systems that want to use Python 2 (via python) with a python3 executable available. This is the setup we have in our own CI for Python 2.7.

This PR fixes this by selecting the first executable available. Note that it will still look for python3 is python does not exist.

Also, the original implementation was no longer writing the shebang to the poetry script on Windows which broke the bash shell on Windows

Pull Request Check List

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

Co-authored-by: Jonathan Piché <jonapich@users.noreply.github.com>
@abn abn merged commit 00c9e49 into master Jun 11, 2020
@abn abn deleted the improve-installer-script branch June 11, 2020 14:32
@ddaspit
Copy link

ddaspit commented Jun 12, 2020

Unfortunately, this doesn't fix issue #2106. For systems that have both Python 2 (/usr/bin/python) and Python 3 (/usr/bin/python3) installed (many Linux distributions), it will still use the Python 2 executable by default.

def make_bin(self):
if not os.path.exists(POETRY_BIN):
os.mkdir(POETRY_BIN, 0o755)

python_executable = self._which_python()
Copy link

Choose a reason for hiding this comment

The 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 _which_python returns something else that sys.executable ?

# \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:
Copy link

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?

Copy link
Member

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 your python doesn't link to your preferred python version, please look up how you can change it in your distribution.

Copy link

@bersace bersace Jul 3, 2020

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 to python2 ?

Copy link

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 :)

Copy link
Member

Choose a reason for hiding this comment

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

Where it violates PEP394?

If the python command is installed, it is expected to invoke either the same version of Python as the python3 command or as the python2 command.

Copy link

@bersace bersace Jul 3, 2020

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 like pip installs a package for the interpreter running pip.

Copy link

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.

Copy link
Member Author

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.

Copy link

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 ?

Copy link

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 running get-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 where pyenv either fails completely because development headers aren't available, or it builds broken python binaries because development headers for "optional" modules like zlib and ncurses 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.

Copy link

github-actions bot commented Mar 1, 2024

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 Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants