Merge lp:~savoirfairelinux-openerp/lp-community-utils/pep394 into lp:lp-community-utils

Status: Needs review
Proposed branch: lp:~savoirfairelinux-openerp/lp-community-utils/pep394
Merge into: lp:lp-community-utils
Diff against target: 39 lines (+4/-4)
4 files modified
clone_mp_to_community.py (+1/-1)
merge_mp.py (+1/-1)
openerp-nag (+1/-1)
replay_missing.py (+1/-1)
To merge this branch: bzr merge lp:~savoirfairelinux-openerp/lp-community-utils/pep394
Reviewer Review Type Date Requested Status
Leonardo Pistone Approve
Joël Grand-Guillaume @ camptocamp Abstain
Yannick Vaucher @ Camptocamp Abstain
Alexandre Fayolle - camptocamp political opinion Disapprove
Stefan Rijnhart (Opener) Abstain
Ronald Portier (Therp) (community) code review and partial testing Approve
Maxime Chambreuil (http://www.savoirfairelinux.com) Approve
Review via email: mp+204535@code.launchpad.net

Description of the change

Adapted to crushbangs to PEP394 specifications.

The reason for this is added portability since not all distributions use python2 as the default python.
The biggest distro to use python3 as the target of the python symlink is Arch Linux.

`/usr/bin/env python` should only be used when code can be run in both python2 and python3

To post a comment you must log in.
Revision history for this message
Maxime Chambreuil (http://www.savoirfairelinux.com) (max3903) :
review: Approve
Revision history for this message
Ronald Portier (Therp) (rportier1962) wrote :

I doubt wether this change is really a good idea.

When running python from a virtualenv, in many cases this relies on a virtualenv specific python command to be the first in the path. When using python2, suddenly the whole virtualenv will be ignored and the script will revert to using the system python.

Therefore the change will break most virtualenv installations, which I think will be much more common than Arch Linux installs.

Or do I overlook something?

review: Needs Information
Revision history for this message
Ronald Portier (Therp) (rportier1962) wrote :

Just to add to my previous comment:

I do think it is a good idea to use #!/usr/bin/env but it should be followed by python. For the - at this moment - rare occasions where python3 is the default, it will be necesarry to manipulate the path to use python2 instead.

Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

@rportier1962
As you pointed out, /usr/bin/env will take the python from the virtualenv.
If you do a ls -l, you'll notice that python is a symlink to the python2 or python3 executable, depending on if you used virtualenv2 or virtualenv3.
My changes will, in fact, not break any virtualenv installations for that reason.
Now, if we use /usr/bin/env python instead of /usr/bin/env python2, we are potentially breaking the shebang of anyone not in a python2 virtualenv.
I reiterate that /usr/bin/env python2 should be used as is specified in http://www.python.org/dev/peps/pep-0394/.

Revision history for this message
Ronald Portier (Therp) (rportier1962) wrote :

Hello Sandy,

Ik rechecked the effect of the different commands on our virtualenv installation running on debian testing.

As can be seen /usr/bin/env python2 is using the system python, even when running from a virtualenv. I think you are using a much newer version of virtualenv, that is creating the symbolic links you are referring to. My point is that a lot of existing - even relatively recent and up to date - installations, do not have those symlinks.

rportier@vanaheim:~$ virtualenv --python=python2.7 test_usr_bin_env
Running virtualenv with interpreter /usr/bin/python2.7
New python executable in test_usr_bin_env/bin/python2.7
Also creating executable in test_usr_bin_env/bin/python
Installing distribute.............................................................................................................................................................................................done.
Installing pip...............done.
rportier@vanaheim:~$ ls -l
total 24
-rw------- 1 rportier people 1643 Jan 3 15:59 <email address hidden>
-rw------- 1 rportier people 1070 Jan 3 15:59 <email address hidden>
-rw------- 1 rportier people 1751 Jan 3 15:57 <email address hidden>
-rw------- 1 rportier people 4448 Jan 3 16:00 <email address hidden>
drwxr-xr-x 6 rportier people 4096 Feb 7 10:46 test_usr_bin_env
rportier@vanaheim:~$ . test_usr_bin_env/bin/activate
(test_usr_bin_env)rportier@vanaheim:~$ python
Python 2.7.3 (default, Jan 2 2013, 13:56:14)
[GCC 4.7.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.prefix
'/home/rportier/test_usr_bin_env'
>>> quit()
(test_usr_bin_env)rportier@vanaheim:~$ /usr/bin/env python
Python 2.7.3 (default, Jan 2 2013, 13:56:14)
[GCC 4.7.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.prefix
'/home/rportier/test_usr_bin_env'
>>> quit()
(test_usr_bin_env)rportier@vanaheim:~$ /usr/bin/env python2
Python 2.7.3 (default, Jan 2 2013, 13:56:14)
[GCC 4.7.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.prefix
'/usr'

Revision history for this message
Virgil Dupras (hsoft) wrote :

Interesting, I don't get the same results (I'm on Arch). It must be virtualenv's version. In any case, it looks like it indeed isn't safe to rely on /usr/bin/env python2.

vdupras@vdupras-thinkpad-x201:~/src/openerp$ . env/bin/activate
(env)vdupras@vdupras-thinkpad-x201:~/src/openerp$ python
Python 2.7.4 (default, Apr 19 2013, 18:28:01)
[GCC 4.7.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.prefix
'/home/vdupras/src/openerp/env'
>>>
(env)vdupras@vdupras-thinkpad-x201:~/src/openerp$ /usr/bin/env python
Python 2.7.4 (default, Apr 19 2013, 18:28:01)
[GCC 4.7.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.prefix
'/home/vdupras/src/openerp/env'
>>>
(env)vdupras@vdupras-thinkpad-x201:~/src/openerp$ /usr/bin/env python2
Python 2.7.4 (default, Apr 19 2013, 18:28:01)
[GCC 4.7.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.prefix
'/home/vdupras/src/openerp/env'
>>>
(env)vdupras@vdupras-thinkpad-x201:~/src/openerp$ deactivate
vdupras@vdupras-thinkpad-x201:~/src/openerp$ virtualenv2 --version
1.11.2
vdupras@vdupras-thinkpad-x201:~/src/openerp$

Revision history for this message
Ronald Portier (Therp) (rportier1962) wrote :

Just on the virtualenv version.

This is ours:

rportier@vanaheim:~$ virtualenv --version
1.7.1.2
rportier@vanaheim:~$ virtualenv2 --version
-su: virtualenv2: command not found

Revision history for this message
Ronald Portier (Therp) (rportier1962) wrote :

/usr/bin/env python2 on debian stable from a virtualenv picks up the system python, as noted before, but it will be working on debian testing (jessie):

openeyedev@laptop01:~/tmp/buildout_test$ . sandbox/bin/activate
(sandbox)openeyedev@laptop01:~/tmp/buildout_test$ which python
/home/openeyedev/tmp/buildout_test/sandbox/bin/python
(sandbox)openeyedev@laptop01:~/tmp/buildout_test$ virtualenv --version
1.11.2
(sandbox)openeyedev@laptop01:~/tmp/buildout_test$ virtualenv2 --version
bash: virtualenv2: opdracht niet gevonden
(sandbox)openeyedev@laptop01:~/tmp/buildout_test$ /usr/bin/env python2
Python 2.7.6 (default, Jan 11 2014, 17:06:02)
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.prefix
'/home/openeyedev/tmp/buildout_test/sandbox'

Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

Ronald, I cannot replicate the behaviour:

scarter ~ % virtualenv --version
1.11.2
scarter ~ % python -c "import sys; print(sys.prefix); print(sys.version)"
/usr
3.3.3 (default, Nov 26 2013, 13:33:18)
[GCC 4.8.2]
scarter ~ % python2 -c "import sys; print(sys.prefix); print(sys.version)"
/usr
2.7.6 (default, Nov 26 2013, 12:52:49)
[GCC 4.8.2]
scarter ~ % /usr/bin/env python -c "import sys; print(sys.prefix); print(sys.version)"
/usr
3.3.3 (default, Nov 26 2013, 13:33:18)
[GCC 4.8.2]
scarter ~ % /usr/bin/env python2 -c "import sys; print(sys.prefix); print(sys.version)"
/usr
2.7.6 (default, Nov 26 2013, 12:52:49)
[GCC 4.8.2]
scarter ~ % virtualenv --python=python2.7 test_usr_bin_env
Running virtualenv with interpreter /usr/bin/python2.7
New python executable in test_usr_bin_env/bin/python2.7
Also creating executable in test_usr_bin_env/bin/python
Installing setuptools, pip...done.
scarter ~ % . test_usr_bin_env/bin/activate
(test_usr_bin_env)scarter ~ % python -c "import sys; print(sys.prefix); print(sys.version)"
/home/scarter/test_usr_bin_env
2.7.6 (default, Nov 26 2013, 12:52:49)
[GCC 4.8.2]
(test_usr_bin_env)scarter ~ % python2 -c "import sys; print(sys.prefix); print(sys.version)"
/home/scarter/test_usr_bin_env
2.7.6 (default, Nov 26 2013, 12:52:49)
[GCC 4.8.2]
(test_usr_bin_env) scarter ~ % /usr/bin/env python -c "import sys; print(sys.prefix); print(sys.version)"
/home/scarter/test_usr_bin_env
2.7.6 (default, Nov 26 2013, 12:52:49)
[GCC 4.8.2]
(test_usr_bin_env)scarter ~ % /usr/bin/env python2 -c "import sys; print(sys.prefix); print(sys.version)"
/home/scarter/test_usr_bin_env
2.7.6 (default, Nov 26 2013, 12:52:49)
[GCC 4.8.2]
(test_usr_bin_env)scarter ~ % deactivate

Whether or not we use /usr/bin/env or not doesn't really matter to me. Most of the shebangs in this MP don't even use it. I am just following the official specifications of PEP394.
What matters to me is python2 over python.
A script should not expect the user to change the /usr/bin/python symlink.

Revision history for this message
Virgil Dupras (hsoft) wrote :

So, from what I understand, using a standard shebang line would prevent people still on debian stable and under a virtualenv from executing those scripts directly. They would now have to execute them as "python script.py".

That doesn't sound like a big deal to me, and even less a reason to keep using a non-standard shebang line.

Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

They would be better off using `python2 script.py` ;)

Revision history for this message
Virgil Dupras (hsoft) wrote :

I doubt that debian stable has "python2" right now. I don't think OpenERP will even *exist* anymore the day debian stable symlinks "python" to python3.

Revision history for this message
Ronald Portier (Therp) (rportier1962) wrote :

As the unmodified scripts are breaking virtualenv anyway, and as oldstyle virtualenvs - like on debian stable - can use the scripts in the way Virgil suggested (python script.py), I think taking everything into consideration that the merge request is really OK.

review: Approve (code review and partial testing)
Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

I confirm that virtualenv creates the local bin/python2 symlink in later versions. I suspect it's this commit[1], which would correspond to release 1.8.3. Current version in stable is 1.7.1. This is also the same version in the latest Ubuntu server LTS, so I would suggest we could stick to '/usr/bin/env python' for now instead of '/usr/bin/env python2'. What do you think?

[1] https://github.com/pypa/virtualenv/commit/03969cbcf00a016a29516ca02fb796845f33fbee

review: Needs Information
Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

If we do use python in lieu of python2, I must urge us to make all scripts python2 and python3 compatible [1]:

> The more general python command should be installed whenever any version of Python is installed and should invoke the same version of Python as either python2 or python3.

[1] http://www.python.org/dev/peps/pep-0394/#recommendation

Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

> [Scripts that are deliberately written to be source compatible with both Python 2.x and 3.x] may continue to use python on their shebang line without affecting their portability.

Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

Yes, there are valid counterarguments I suppose. I'll abstain because I don't pretend to be able to decide between the pros and cons.

review: Abstain
Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

I'm not favorable to the use of /usr/bin/python2 in an ecosystem where /usr/bin/python is used everywhere to mean python 2.6 (which is the current compatibility focus for the OpenERP ecosystem).

Users of distributions using python3.x as default /usr/bin/python have to work around this already. I'm in favor of of changing #!/usr/bin/python to #!/usr/bin/env python but not python2.

This is a personal opinion statement, not a technical judgement.

review: Disapprove (political opinion)
Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

@alexandre-fayolle-c2c I urge you to reconsider.
We, users of distros using python3 don't usually have to do any workaround since using python2 is standard in most python project.
Even openerp using the buildout recipe using the anybox recipe will put the following python2 shebang:
#!/usr/bin/python2

The whole reason I did this MP is, being a python3 default user, these scripts surprised me when they did not work the same way any other python script I use, locally and from external sources thanks to the PEP394 convention.

Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

> Even openerp using the buildout recipe using the anybox recipe will put the following python2 shebang:

FYI: the anybox recipy, by means of the buildout scripts mechanism will
put in the python interpreter with which the buildout is being build.

Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

For those who want /usr/bin/env, but have do not necessarily agree with using python2, you can approve the following MP:
https://code.launchpad.net/~savoirfairelinux-openerp/lp-community-utils/pep394-env/+merge/210217

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) :
review: Abstain
Revision history for this message
Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c) wrote :

Hi,

Honestly no idea what's better here, so I go for abstain as well..

review: Abstain
Revision history for this message
Leonardo Pistone (lepistone) wrote :

If I understand correctly, we are trading a small smoothness advantage for old distributions vs. more up-to-date ones that received pep394.

I lean a bit for calling python2 explicitly. I prefer those with old distributions to go through some (easy) hoops to comply with future-proof pep's that the contrary.

In any case, these scripts are used by people that can figure out how to work around the problem in both cases, so I won't fight over this.

review: Approve

Unmerged revisions

24. By Sandy Carter (http://www.savoirfairelinux.com)

[IMP] PEP 394 fix hashbangs

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'clone_mp_to_community.py'
2--- clone_mp_to_community.py 2013-09-22 14:36:16 +0000
3+++ clone_mp_to_community.py 2014-02-03 16:51:43 +0000
4@@ -1,4 +1,4 @@
5-#!/usr/bin/python
6+#!/usr/bin/env python2
7 # -*- coding: utf-8 -*-
8 #
9 #
10
11=== modified file 'merge_mp.py'
12--- merge_mp.py 2013-09-22 14:36:16 +0000
13+++ merge_mp.py 2014-02-03 16:51:43 +0000
14@@ -1,4 +1,4 @@
15-#!/usr/bin/python
16+#!/usr/bin/env python2
17 # -*- coding: utf-8 -*-
18 #
19 #
20
21=== modified file 'openerp-nag'
22--- openerp-nag 2013-09-22 14:36:16 +0000
23+++ openerp-nag 2014-02-03 16:51:43 +0000
24@@ -1,4 +1,4 @@
25-#!/usr/bin/env python
26+#!/usr/bin/env python2
27 # Copyright 2012 Canonical Ltd.
28 # Written by:
29 # Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
30
31=== modified file 'replay_missing.py'
32--- replay_missing.py 2013-09-22 14:36:16 +0000
33+++ replay_missing.py 2014-02-03 16:51:43 +0000
34@@ -1,4 +1,4 @@
35-#!/usr/bin/python
36+#!/usr/bin/env python2
37 # -*- coding: utf-8 -*-
38 #
39 #

Subscribers

People subscribed via source and target branches