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

Allow passing python version number for install --python, use venv for Python 3 #142

Merged

Conversation

reorx
Copy link
Contributor

@reorx reorx commented Apr 16, 2018

  1. Allow passing python version number for install --python

    e.g. pipsi install --python 2, this will find the python2 executable in $PATH, and use it to create virtualenv for installing package. This concept is borrowed from pipenv which has the similar usage like pipenv --python 2, makes it easier to specify python interpreter, because getting the full path is always a pain.

  2. Add PIPSY_PYTHON as env var binding for --python, makes it possible to set the default python interpreter from shell rc file.

  3. Use venv if python interpreter is python 3. setup.py also determines install_requires by python version, and virtualenv will only be required for python 2. Though virtualenv is OK, it's quite clear that venv is the more official and standard way for python 3. Besides, it's good to see a dependency could be removed, glory to Python 3 :D

Some other subtle changes including:

  • tox.ini: py35 -> py36
  • Add Pipfile and Pipfile.lock

Could be removed if you think it's not necessary for the project.

Xiao added 14 commits April 16, 2018 17:30
`venv` for python 3 has the problem that `venv` cannot add pip in the created virtualenv if it is executed under a virtualenv,
try to find the real path (real python executable of the virtualenv python) before virtualenv is created.
When testing under py33, travis will raise error below:

    ERROR: unknown environment 'py33'

It seems py33 is not supported in the latest tox, or whatever the reason,
this is a CI environment problem. Besides, python 3.3 is too old
to have value for testing nowadays.
try using `python2`, `python3` as the binary name according
to the python major version. Because in some cases,
the python executable under real_prefix is not directly `python`.
get-pipsi.py Outdated
@@ -5,6 +5,7 @@
import sys
from subprocess import call
import textwrap
from pipsi import get_real_python
Copy link
Contributor

Choose a reason for hiding this comment

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

that import is not available for the intended use of that script, its only working in a checkout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, you're right, just forgot that 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in the latest commit

Xiao and others added 2 commits April 17, 2018 17:19
also fixed get_real_python not raise error when all possible paths not exist
@RonnyPfannschmidt
Copy link
Contributor

i just resolved the conflicts using the GH UI

reorx added 2 commits May 31, 2018 17:18
I don't see any value in matching `pipsi --version` output
with `sys.executable`, because:
1. If the virtual env is created by `venv` in python3, the python
   path would be like `/venv/path/bin/python3`, but suffix of
   `sys.executable` might be `python3.6`, which will fail the test
   but actually nothing is wrong
2. the suffix of python path has nothing to do with the actual version
   of python. In the future maybe `venv` or `virtualenv` will change
   the executable path to be `/venv/path/bin/python`, then this test
   would fail again.
3. This is just a test again venv or virtualenv's internal logic.
   It compares current python `sys.executable` with a virtualenv
   `sys.executable` which is created by current python,
   uh, it's really nothing to do with pipsi.
As long as this PR involves `venv` package, this part should be necessary.
@reorx
Copy link
Contributor Author

reorx commented May 31, 2018

Add another two commits to solve the ci tests.

setup.py Outdated

if sys.version_info.major == 2:
requires.extend([
'virtualenv'
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, that one is always required - else one would be dependent on the system one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but that's the purpose of this PR right? To use builtin package venv instead of virtualenv in Python 3. And since the code here https://github.com/mitsuhiko/pipsi/pull/142/files#diff-bc98b82c8d093ef76c369fe573d188cbR343 has already handled this situation, virtualenv is not event imported when using Python 3. So I think it could be directly removed from the requires. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@reorx creating a python 2 env will require virtualenv - so until python 2 support is dropped, its a required dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got you, but there maybe a bit confusing here, let's make it clear by enumerate different situations:

  1. If get-pipsi.py is executed by python 2, then virtualenv will be included in requires.

    • pipsi install foo, this will use ~/.local/venvs/pipsi/bin/python to create python 2 virtualenv for foo, works ok
    • pipsi install --python /some/other/python2 foo, in this situation whether virtualenv for foo can be created depends on /some/other/python2 has virtualenv installed or not, nothing to do with pipsi requires virtualenv or not.
    • pipsi install --python /some/python3 foo, in this situation pipsi will call /some/python3 -m venv to create virtualenv for foo, works ok
  2. If get-pipsi.py is executed by python 3, then virtualenv will not be included in requires.

    • pipsi install foo, this will call ~/.local/venvs/pipsi/bin/python -m venv to create python 3 virtualenv for foo, works ok
    • pipsi install --python /some/python2 foo, in this situation whether virtualenv for foo can be created depends on /some/python2 has virtualenv installed or not, nothing to do with pipsi requires virtualenv or not.
    • pipsi install --python /some/other/python3 foo, in this situation pipsi will call /some/other/python3 -m venv to create virtualenv for foo, works ok

I think these are all the situations pipsi can meet, each shows that "ignoring virtualenv for pipsi in Python 3" will not have bad influence on how pipsi works. What's your idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

i get where you are coming from

i summarize my stance as requiring a external virtualenv for the pipsi use-case is not acceptable ^^
since it would require changes to all target pythons if they are python2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey I hope I didn't get it wrong, by saying "requiring a external virtualenv for the pipsi use-case is not acceptable" did you mean pipsi should not use virtualenv if it's possible? Which is the same idea with mine.

Copy link
Contributor

Choose a reason for hiding this comment

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

pipsi should depend on virtualenv since ithas to use it for python2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK so I finally find out what's going on here, the main reason we are not speaking the same voice is because, I changed args = [sys.executable, '-m', 'virtualenv', '-p', python or sys.executable, venv_path] to [python, '-m', 'virtualenv', venv_path] previously, without much thoughts it became the subconscious when I was thinking about whether virtualenv is required or not, and that makes things all wrong.

The right way is to only use venv if target python is version 3, otherwise virtualenv should always be used to create for python 2. Whether pipsi is installed in python 2 or 3 makes no difference here (before realized this, I thought a virtualenv installed in python 2 can not create python 3 virtualenv, vice versa).

I've updated the PR with new commits, please help to see if I do it right this time.

@cs01
Copy link
Contributor

cs01 commented Jun 2, 2018

Regarding Pipfile and Pipfile.lock

1.) Under what circumstances would these be used? If the answer is unclear, maybe it should not be part of pipsi. If the answer is clear, can it be added to documentation somewhere?
2.) Who is responsible for updating the .lock file, and when should it be done?

@RonnyPfannschmidt
Copy link
Contributor

@cs01 thanks for the note

@reorx please remove those 2 from the project

@reorx
Copy link
Contributor Author

reorx commented Jun 2, 2018

@cs01 actually they are not part of the pipsi package, that's why they are not included in MANIFEST.in file. It's intended to be used for developing purpose, by running pipenv shell you can quickly make a virtual environment with dependencies (specified in Pipfile) installed.

For question 2, Pipfile.lock is only updated when pipenv update/lock is called, or dependencies are modified by pipenv install/uninstall. Whoever wants to change developing tools (tox, pytest etc), or change the real dependencies of pipsi itself, is responsible for updating the lock file (or even Pipfile). This should be done by the time the changes happen.

It's indeed a kind of personal choice, so remove them is OK, maybe better for sticking out the main purpose of this PR.

reorx and others added 4 commits June 2, 2018 14:01
since they are not necessary for this branch
previously this branch makes virtualenv being created by
target python instead of `sys.executable` for all situations.
this commit turns it back, now only when target python version is 3
will the virtualenv being created by target python itself and `venv`
@RonnyPfannschmidt
Copy link
Contributor

will merge after green

@reorx
Copy link
Contributor Author

reorx commented Jun 6, 2018

@RonnyPfannschmidt great, is the current status considered green now?

@RonnyPfannschmidt RonnyPfannschmidt merged commit aa3b496 into mitsuhiko:master Jun 6, 2018
@RonnyPfannschmidt
Copy link
Contributor

thanks for the ping,i got sidetracked

@taion
Copy link

taion commented Jul 5, 2018

Would it be possible to cut a new release with this fix? It would be nice not to have to install from master.

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.

None yet

4 participants