Opened 3 months ago

Closed 3 weeks ago

#13505 closed Patch - Feature (fixed)

Rewrite python shebangs during 'make install' step

Reported by: rcrdnalor Owned by: Bill Meek
Priority: minor Milestone: 31.0
Component: Bindings - Python Version: Master Head
Severity: medium Keywords: python shebang
Cc: Ticket locked: no

Description

This patch is a follow up to ticket #13504: Feature patch - clean up (dead) QMAKE_COPY_DIR code

As outlined in #13504, we need to set the python shebangs according to the configured python version for each installation.

Python's PEP 394 already defines the usage of shebangs in respect of the configured python version:

On recent linux distributions, the shebang #!/usr/bin/env python may not exist anymore and package install scripts already warn about 'ambiguous python shebang' during packaging.

This patch rewrites the shebang of python scripts during the 'make install' step of mythtv's 'metadata' scripts located in 'mythtv/programs/scripts' according the given python version. It ignores scripts that are set intentionally to python2.

It can be easily expanded to other folders like 'hardwareprofile' or 'internetcontent'.

Please note: This script looks like somehow 'quick and dirty', but i tried other solutions like creating an 'intermediate target' for the python scripts, and they are much more complicated than this one, albeit, I don't speak QMAKE well.

Attachments (4)

0001-Rewrite-python-shebangs-during-make-install-step.patch (5.5 KB) - added by rcrdnalor 3 months ago.
Patch that rewrites python shebangs not pointing to a specific python version
program-scripts-python-shebang-fixup.patch (11.8 KB) - added by Gary Buhrmaster 4 weeks ago.
program-scripts-python-shebang-fixup-V2.patch (14.0 KB) - added by Gary Buhrmaster 3 weeks ago.
program-scripts-python-shebang-fixup-V3.patch (14.0 KB) - added by Gary Buhrmaster 3 weeks ago.

Download all attachments as: .zip

Change History (17)

Changed 3 months ago by rcrdnalor

Patch that rewrites python shebangs not pointing to a specific python version

comment:1 in reply to:  description Changed 3 months ago by Gary Buhrmaster

Replying to rcrdnalor:

This patch rewrites the shebang of python scripts during the 'make install' step of mythtv's 'metadata' scripts located in 'mythtv/programs/scripts' according the given python version.

I will note that some packaging systems (Fedora) have a tool included that recursively fixes python shebangs called pathfix.py which implements something similar, but almost certainly slightly different. It might be worth reviewing how it does its work to see how/if it can be utilized. Docs are at: https://fedoraproject.org/wiki/Changes/Make_ambiguous_python_shebangs_error#Using_pathfix.py_to_fix_shebangs

FWIW, my rpm spec files all use pathfix.py to adjust the python to either python2 or python3 depending on target python version.

comment:2 Changed 3 months ago by rcrdnalor

Maybe I was not precise enough in the description of this ticket:

QMAKE allows a custom step only at the beginning of the INSTALLS target step: This is defined by the .extra stanza and gets called *before* the files/folders are actually installed. Running a script like 'pathfix.py' at this time over the source paths will modify the git sources unconditionally. This has to be avoided, because it is impractical.

I simply do not know how to run a script over the installed files *at the end* of the QMAKE INSTALLS target.

I would really prefer to have a standardized step here instead of a custom copy/rewrite script.

But as I said already, I do not know QMAKE well. Help is appreciated.

comment:3 Changed 4 weeks ago by Gary Buhrmaster

But as I said already, I do not know QMAKE well.

Discussion:

I am not sure many do (I know I do not know it well either (I mostly just tolerate qmake), maybe one of the MythTV devs would be considered an expert).  That there are so many websites dedicated to the hidden and undocumented features and capabilities probably says something important about qmake itself.  It should be noted that qt company (and its predecessors) itself created qbs to try to deal with some of the qmake issues, but has more recently deprecated even qbs, putting more resources into cmake as a cross platform build option moving forward based on customer feedback.

The qmake way to deal with script modification would seem to be using the QMAKE_EXTRA_COMPILER stanza to convert the files to the eventual target content (as the transformation is something akin to a compilation), but while I initially considered do so (had some code fragments doing it), the advantages did not seem to outweigh the added complexity (and even more under documented stanzas needed for providing your own "compiler"), so I agree with your approach that using the extra member seems more straightforward (as the least bad option).

That said, there are a couple of things that I noted when I looked at both the existing pro file, and at the proposed patches in this ticket, some of which were inherited from the original .pro file, and some of which are from the proposed patches:  (a) make uninstall does not work, (b) one should not use #!/usr/bin/env for a packaged binary(1), (c) the shebang does not specify the actual binary (related to, but different, from the improper use of /usr/bin/env)(2), (d) A shell script may not work in certain non posix environments, (e) the installed scripts are not properly installed based on bindings (installing scripts that are not executable due to dependencies not installed (the perl scripts require the perl bindings, the python scripts require the python bindings)).  Except for (b) none of which are probably critical to address, but once one looks at code, more and more issues tends to be seen.

(1) A #!/usr/bin/env shebang has a somewhat storied history, and while there are those that still write it for their own personal use, the general consensus is that use of it is considered (strongly) discouraged for installed executable use because it depends on what the user running the program might have setup in their environment, potentially resulting in subtle, or not subtle, issues, and one should get repeatable results for installed programs.  For some distros, more than discouraged, the /usr/bin/env shebang is banned from installed packages. Typically the python setup modules sets the shebang to the python that it is called with (for this example, let us call it #!/usr/bin/python3) when copying scripts, as it done for the mythpython and mythwikiscripts from the bindings directory.  This is considered correctness.

(2) python distutils defaults to replacing and shebang with the actual executable used for setup.py (or equivalent).

Proposed action:

I will attach my alternative patch proposal, which address the issues I have identified.

Future enhancements:

A number of the python files in the .../programs/scripts/ directory suffer the same issue with having errant shebangs or executable bits that are not correct (similiar to #13539 and #13540 ).  But as they are essentially benign at the moment, I do not plan on prioritizing working up a patch for them.

Changed 4 weeks ago by Gary Buhrmaster

comment:4 Changed 4 weeks ago by Gary Buhrmaster

I accidentally uploaded an attachment of the name "program-scripts-python-shebang-fixup.patch.old" but do not have the privs to delete it. Please delete it if possible. Thanks.

comment:5 Changed 4 weeks ago by Bill Meek

Milestone: needs_triage31.0
Owner: changed from Raymond Wagner to Bill Meek
Status: newaccepted

comment:6 Changed 4 weeks ago by rcrdnalor

@Gary Buhrmaster: Thank You for looking at this ticket and creating a patch!

I tested your patch (program-scripts-python-shebang-fixup.patch) with a clean setup of Ubuntu 19.10 (only python3 available).

I confirm that the following python files have the correct shebang (#!/usr/bin/python3) and the executable flag set after a make install step of todays master (v31-Pre-1659-g1d4d51b0d9) with the patch applied (program-scripts-python-shebang-fixup.patch):

/usr/local/share/mythtv/metadata/Movie/tmdb3.py
/usr/local/share/mythtv/metadata/Television/ttvdb.py
/usr/local/share/mythtv/metadata/Music/mbutils.py
/usr/local/share/mythtv/hardwareprofile/deleteProfile.py
/usr/local/share/mythtv/hardwareprofile/sendProfile.py

Note: All above scripts are called directly from various mythtv programs, therefore, they need the correct shebangs ant the exe flag set.

These python scripts are now compliant to the scripts installed by the Python Bindings:

/usr/local/bin/mythpython
/usr/local/bin/mythwikiscripts

comment:7 Changed 4 weeks ago by Bill Meek

Tested with python2 too with expected results. However distclean reports 265 lines like this (files owned by root.)

rm: cannot remove 'obj/metadata/Movie/tmdb3.py': Permission denied

Easily fixed, but I've always run distclean as myself, not root. Could be handled in the release notes with a comment.

comment:8 Changed 4 weeks ago by Bill Meek

OK, maybe not distclean, still testing.

comment:9 in reply to:  7 ; Changed 4 weeks ago by Gary Buhrmaster

Replying to Bill Meek:

Tested with python2 too with expected results. However distclean reports 265 lines like this (files owned by root.)

Yes, thinking about this, that would likely be the expected result if you ran make install as root (I almost always run it as a normal user), as the obj/.... files are installed as whatever user runs the make. There are likely ways to address that, but they start to get differently ugly due to os specific filesystem requirements (another case where qmake is not sufficiently flexible).

comment:10 in reply to:  9 Changed 3 weeks ago by Gary Buhrmaster

Replying to Gary Buhrmaster:

Yes, thinking about this, that would likely be the expected result if you ran make install as root

An approach will be to do what I had abandoned as even more complex and likely error prone, and that was to look at utilizing the QMAKE_EXTRA_COMPILERS capabilities. I will add it to my list to reconsider, but realistically I am not going to be able to even look at it for quite a few weeks.

comment:11 Changed 3 weeks ago by Gary Buhrmaster

(how time flys when you are looking for a distraction)

Revised patch will be attached which uses QMAKE_EXTRA_COMPILERS to "compile" the python scripts to adjust the shebangs as needed.  This should resolve the issue with permission error messages for a 'make clean' after a 'sudo make install' (at least it works for me).

There is an artifact when doing a 'make clean' with a message about not being able to delete a directory, but it is actually deleted later in the process.  The fix that immediately comes to mind involves doing full subdir .pro file recursion, which does not seem to be worthwhile for a message.

Changed 3 weeks ago by Gary Buhrmaster

comment:12 Changed 3 weeks ago by Gary Buhrmaster

I realized that I had started from a stash with a slightly out-of-date script (which does not matter for this use case, but it could at some future time), so I will attach the V3 variant.

Changed 3 weeks ago by Gary Buhrmaster

comment:13 Changed 3 weeks ago by Gary Buhrmaster <gary.buhrmaster@…>

Resolution: fixed
Status: acceptedclosed

In 706f14596d/mythtv:

Python Bindings: Set shebangs to the configured python version

Replace the undesirable /usr/bin/env format with the
executable itself.

Closes #13505

Signed-off-by: Bill Meek <billmeek@…>

Note: See TracTickets for help on using tickets.