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

get-poetry.py: Use correct python when installed with non-default python #1042

Closed

Conversation

terminalmage
Copy link

@terminalmage terminalmage commented Apr 18, 2019

On CentOS 6, /usr/bin/env python is Python 2.6. This results in poetry being unusable since 2.7 is required. Poetry should still work when an alternative Python is used to install it. That is, you should be able to run python2.7 get-poetry.py and end up with a usable installation of poetry.

This will use the python executable used to run get-poetry.py in the shebang, ensuring that a valid python is used.

NOTE: I didn't see a place to add tests for this, the "installation" dir seems to refer to installing python packages using poetry, not installing poetry itself via get-poetry.py. Any help in this respect would be much appreciated.

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

@rafalkrupinski
Copy link

rafalkrupinski commented May 3, 2019

Same problem on OSX

@terminalmage
Copy link
Author

I have submitted and asked, per the PR template. It has been 7 weeks, can anyone offer some insight as to how we can move forward with this?

@terminalmage
Copy link
Author

I've rebased to resolve a merge conflict.

Any chance I can get some answers regarding testing?

@brycedrennan brycedrennan added the kind/bug Something isn't working as expected label Aug 17, 2019
@brycedrennan
Copy link
Contributor

@terminalmage thanks for your contribution. sorry we've been swamped. I'm still getting up to speed so I can't make promises as to when we'll be able to look at this.

Would you be able to fix the formatting issue? (run black on the code)

terminalmage and others added 2 commits August 19, 2019 09:01
On CentOS 6, `/usr/bin/env python` is Python 2.6. This results in poetry
being unusable since 2.7 is required. Poetry should still work when an
alternative Python is installed. That is, you should be able to run
`python2.7 get-poetry.py` and end up with a usable installation of
poetry.

This will use the python executable used to run get-poetry.py in the
shebang, ensuring that a valid python is used.
@terminalmage
Copy link
Author

@brycedrennan Formatting should be taken care of now.

Copy link
Contributor

@brycedrennan brycedrennan left a comment

Choose a reason for hiding this comment

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

This seems like it still won't fix the issue. Examples:

  • my system python is 2.6, but I run python get-poetry.py in my 3.6 virtual environment, we'll still have the same issue.
  • my system python is 2.6 but I run /path/to/python3/python get-poetry.py

We could alternatively change this fix to use the full path to the python executable, but then I worry about someone running this inside of a virtualenv and then later deleting a virtualenv. Perhaps we could error out if someone runs get-poetry.py inside a virtualenv and then use the full path?

@michielboekhoff
Copy link

@brycedrennan -

Thanks for your excellent points. Disallowing Poetry from running within a virtualenv seems unnecessary perhaps. I think the rationale for this is not being able to set the path to the Python executable to use for Poetry, which perhaps can be made configurable.

The way this can be done is to add a little script that checks a configuration value and uses that to start Poetry with the correct Python interpreter. This would allow people to change the Python version used as well. If this is populated with the system interpreter (e.g. /usr/bin/python), it would allow people to force Python 3 by writing something along the lines of poetry config python $(which python3). It's the most flexible option I can discern, what do you think?

@terminalmage
Copy link
Author

This seems like it still won't fix the issue. Examples:

* my system python is 2.6, but I run `python get-poetry.py` in my 3.6 virtual environment, we'll still have the same issue.

If your system Python is 2.6, and you run python get-poetry.py, get-poetry.py will fail to run since it uses 2.7-specific syntax, as you can see in the below commands (run from a git clone of poetry):

% docker run --rm -it -v $PWD:/poetry centos:6 bash
Unable to find image 'centos:6' locally
6: Pulling from library/centos
Digest: sha256:dec8f471302de43f4cfcf82f56d99a5227b5ea1aa6d02fa56344986e1f4610e7
Status: Downloaded newer image for centos:6
[root@68c7efa4c7bd /]# python /poetry/get-poetry.py
  File "/poetry/get-poetry.py", line 173
    return value in {"true", "1", "y", "yes"}
                           ^
SyntaxError: invalid syntax
[root@68c7efa4c7bd /]#
* my system python is 2.6 but I run `/path/to/python3/python get-poetry.py`

We could alternatively change this fix to use the full path to the python executable, but then I worry about someone running this inside of a virtualenv and then later deleting a virtualenv. Perhaps we could error out if someone runs get-poetry.py inside a virtualenv and then use the full path?

Very good points.

@brycedrennan
Copy link
Contributor

@michielboekhoff To be clear, I don't want to prevent poetry from running in virtualenvs, I want to prevent the global, system-wide installation of poetry in a virtualenv. This would allow us to solve the problem you're having while not creating a new situation that people will get into trouble with.

@terminalmage in my first example the python being run is 3.6 from my virtualenv.

@terminalmage
Copy link
Author

Ahh yes, thanks for clarifying.

@brycedrennan
Copy link
Contributor

@michielboekhoff

The way this can be done is to add a little script that checks a configuration value and uses that to start Poetry with the correct Python interpreter. This would allow people to change the Python version used as well. If this is populated with the system interpreter (e.g. /usr/bin/python), it would allow people to force Python 3 by writing something along the lines of poetry config python $(which python3). It's the most flexible option I can discern, what do you think?

Can't poetry only be started from interpreters it's been installed in? So the configuration would only have one valid option. That feels not a great fit for a setting. Also wouldn't poetry config not be runnable at all in python 2.6?

@michielboekhoff
Copy link

@brycedrennan - This is true, it wouldn't fix it for Python 2.6 (and other unsupported versions of Python).

The only reason I mention this is this: say, you're a user who only has Python 2 installed. You upgrade your things to Python 3, potentially even removing Python 2 altogether, only to find Poetry still uses 2 and now fails to start.

The way I understand it, it's all just Python scripts invoking the interpreter as defined by #!/usr/bin/env python (as per the ~/.poetry/bin/poetry file), especially considering it includes all of its dependencies within it. I was able to change the ~/.poetry/bin/poetry file and use a different Python interpreter (3.6) without having to install it with Python 3.

There very well might be a much nicer mechanism to handle this.

@brycedrennan
Copy link
Contributor

interesting. thanks I'll have to think about this

@brycedrennan
Copy link
Contributor

Okay I think we should solve this with two changes:

  • create create a function that tries to pick a good version of python
  • add a command line argument to the installer that lets you manually specify a python binary

arguments can be added like this:
curl -sSL https://raw.githubusercontent.com/sdispater/poetry/master/get-poetry.py | python - --python-binary /path/to/python

This would silently fix the problem for many people while also giving a configuration option to address any other edge cases. What do you guys think?

Psuedocode:

def find_compatible_python_binary():
    if command_line_binary_specified:
        return command_line_args['python_binary']
    # default to trying to use python3 if its available
    if command_exists('python3'):
        return 'python3'
    if version_of_binary('python') < (2,7,0):
        raise RuntimeError("Poetry only works with Python >=2.7.  You can manually specify a python binary for poetry to use with the `--python-binary` argument.")

@michielboekhoff
Copy link

@brycedrennan - I like it! If no one else has any time, I can pick this up later this evening.

@terminalmage
Copy link
Author

@michielboekhoff I will be very busy for the next two weeks, so if you are free and have the time, I would be very interested to see what you come up with.

@bersace
Copy link

bersace commented Sep 12, 2019

The only reason I mention this is this: say, you're a user who only has Python 2 installed. You upgrade your things to Python 3, potentially even removing Python 2 altogether, only to find Poetry still uses 2 and now fails to start.

@michielboekhoff that's the purpose of package management. Custom script installation defeat the functionnality. I think it's better to fail if user removed required python, rather than picking random interpreter from either virtual_env or interpreter-version-of-the-day.

If one choose to install poetry with /usr/bin/python3, it will still works when upgrading from 3.7 to 3.8. But it should effectively fails if one installs poetry with /usr/bin/python3.7 and drops python3.7.

In the face of ambiguity, refuse the temptation to guess.

I think that this /usr/bin/env is actually a guess that should be removed by what the user uses when running get-poetry.py.

my 2c. :-)

@michielboekhoff
Copy link

Sorry it took me so long to get around to this. I've taken a stab at this in #1518. @brycedrennan - I couldn't (easily) figure out how to detect the version of a given Python parser - without running python --version on it. I didn't think that would be too big a problem. If it is, let me know and I'll make that a part of the PR as well.

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
kind/bug Something isn't working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants