Canonicalize Python shebangs #9314

ghost · 2019-09-11T14:56:48Z

Motivation and Context

/usr/bin/env python3 is the suggested shebang for Python in general (likewise for python2) and is conventional across platforms. This eases development on systems where python is not installed in /usr/bin (FreeBSD for example) and makes it possible to develop in virtual environments (venv) for isolating dependencies.

Many packaging guidelines discourage the use of /usr/bin/env, but since this is the canonical way of writing shebangs in the Python community, many packaging scripts are already equipped to handle substituting the appropriate absolute path to python automatically.

Some RPM package builders lacking brp-mangle-shebangs need a small fallback mechanism in the package spec to stamp the appropriate shebang on installed Python scripts.

Description

Shebangs in Python scripts are changed to use /usr/bin/env and the Makefile.am sed replacements for python3 to python2 are adjusted accordingly.

The RPM shebang mangling exception for test-runner.py is also removed, which probably should have been done in 3ba8cd6.

How Has This Been Tested?

We have been running this through the automated tests for a while in the FreeBSD PR.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.

rlaager

This is reasonable.

gdevenyi · 2019-09-11T16:49:28Z

I don't think this follows https://www.python.org/dev/peps/pep-0394/#for-python-script-publishers

ghost · 2019-09-11T17:18:38Z

I don't think this follows https://www.python.org/dev/peps/pep-0394/#for-python-script-publishers

Why not? I believe this is the part that applies:

In cases where the script is expected to be executed outside virtual environments, [...]
developers may either use a Python package installation tool that rewrites shebang lines for the installed environment, provide instructions on updating shebang lines interactively, or else use more specific shebang lines that are tailored to the target environment.

The packaging tools we use rewrite the shebang to target the specific environment the package is being built for. This is separate from changing python3 to python2. The shebangs are further mangled for packaging to something like /usr/bin/python3 or /usr/bin/python2 or whatever is appropriate for the system being targeted.

ghost · 2019-09-11T17:26:36Z

Updated to remove brp mangle exception for test-runner.py, which probably should have been done in 3ba8cd6

ghost · 2019-09-11T18:46:00Z

The three early test failures appear to be systems lacking /usr/lib/rpm/redhat/brp-mangle-shebangs. That should be detectable in the RPM spec so a fallback can be used instead. I will fix this in the next push.

gdevenyi · 2019-09-11T18:49:56Z

I missed the rewrite code, sorry.

gmelikov

LGTM

gmelikov

Oops, there are problems in our test builders, e.g. centos 7.

codecov · 2019-09-11T23:32:28Z

Codecov Report

Merging #9314 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #9314      +/-   ##
==========================================
- Coverage   79.14%   79.11%   -0.04%     
==========================================
  Files         401      401              
  Lines      122077   122074       -3     
==========================================
- Hits        96617    96573      -44     
- Misses      25460    25501      +41

Flag	Coverage Δ
#kernel	`79.78% <ø> (+0.01%)`	⬆️
#user	`66.92% <ø> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d666206...6ba8734. Read the comment docs.

/usr/bin/env python3 is the suggested[1] shebang for Python in general (likewise for python2) and is conventional across platforms. This eases development on systems where python is not installed in /usr/bin (FreeBSD for example) and makes it possible to develop in virtual environments (venv) for isolating dependencies. Many packaging guidelines discourage the use of /usr/bin/env, but since this is the canonical way of writing shebangs in the Python community, many packaging scripts are already equipped to handle substituting the appropriate absolute path to python automatically. Some RPM package builders lacking brp-mangle-shebangs need a small fallback mechanism in the package spec to stamp the appropriate shebang on installed Python scripts. [1]: https://docs.python.org/3/using/unix.html?#miscellaneous Signed-off-by: Ryan Moeller <[email protected]>

ghost · 2019-09-12T13:08:45Z

Updated to provide a fallback method for mangling the Python shebangs on RPM builders lacking brp-mangle-shebangs. This should fix the three failed test runners.

/usr/bin/env python3 is the suggested[1] shebang for Python in general (likewise for python2) and is conventional across platforms. This eases development on systems where python is not installed in /usr/bin (FreeBSD for example) and makes it possible to develop in virtual environments (venv) for isolating dependencies. Many packaging guidelines discourage the use of /usr/bin/env, but since this is the canonical way of writing shebangs in the Python community, many packaging scripts are already equipped to handle substituting the appropriate absolute path to python automatically. Some RPM package builders lacking brp-mangle-shebangs need a small fallback mechanism in the package spec to stamp the appropriate shebang on installed Python scripts. [1]: https://docs.python.org/3/using/unix.html?#miscellaneous Reviewed-by: Richard Laager <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: George Melikov <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#9314

/usr/bin/env python3 is the suggested[1] shebang for Python in general (likewise for python2) and is conventional across platforms. This eases development on systems where python is not installed in /usr/bin (FreeBSD for example) and makes it possible to develop in virtual environments (venv) for isolating dependencies. Many packaging guidelines discourage the use of /usr/bin/env, but since this is the canonical way of writing shebangs in the Python community, many packaging scripts are already equipped to handle substituting the appropriate absolute path to python automatically. Some RPM package builders lacking brp-mangle-shebangs need a small fallback mechanism in the package spec to stamp the appropriate shebang on installed Python scripts. [1]: https://docs.python.org/3/using/unix.html?#miscellaneous Reviewed-by: Richard Laager <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: George Melikov <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes #9314

ghost requested a review from rlaager September 11, 2019 14:56

rlaager approved these changes Sep 11, 2019

View reviewed changes

ghost force-pushed the python-shebangs branch from 4ee5a00 to 29b5826 Compare September 11, 2019 17:23

gmelikov approved these changes Sep 11, 2019

View reviewed changes

gmelikov suggested changes Sep 11, 2019

View reviewed changes

behlendorf added the Status: Code Review Needed Ready for review and testing label Sep 11, 2019

ghost force-pushed the python-shebangs branch from 29b5826 to 6ba8734 Compare September 12, 2019 13:07

behlendorf approved these changes Sep 12, 2019

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 12, 2019

behlendorf merged commit 4f342e4 into openzfs:master Sep 12, 2019

ghost deleted the python-shebangs branch September 13, 2019 03:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Canonicalize Python shebangs #9314

Canonicalize Python shebangs #9314

ghost commented Sep 11, 2019 •

edited by ghost

Loading

rlaager left a comment

gdevenyi commented Sep 11, 2019

ghost commented Sep 11, 2019 •

edited by ghost

Loading

ghost commented Sep 11, 2019

ghost commented Sep 11, 2019

gdevenyi commented Sep 11, 2019

gmelikov left a comment

gmelikov left a comment

codecov bot commented Sep 11, 2019 •

edited

Loading

ghost commented Sep 12, 2019 •

edited by ghost

Loading

Canonicalize Python shebangs #9314

Canonicalize Python shebangs #9314

Conversation

ghost commented Sep 11, 2019 • edited by ghost Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

rlaager left a comment

Choose a reason for hiding this comment

gdevenyi commented Sep 11, 2019

ghost commented Sep 11, 2019 • edited by ghost Loading

ghost commented Sep 11, 2019

ghost commented Sep 11, 2019

gdevenyi commented Sep 11, 2019

gmelikov left a comment

Choose a reason for hiding this comment

gmelikov left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 11, 2019 • edited Loading

Codecov Report

ghost commented Sep 12, 2019 • edited by ghost Loading

ghost commented Sep 11, 2019 •

edited by ghost

Loading

ghost commented Sep 11, 2019 •

edited by ghost

Loading

codecov bot commented Sep 11, 2019 •

edited

Loading

ghost commented Sep 12, 2019 •

edited by ghost

Loading