[PR PATCH] [RFC] option to skip 03-rewrite-python-shebang

[PR PATCH] [RFC] option to skip 03-rewrite-python-shebang

[tornaria]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

There is a new pull request by tornaria against master on the void-packages repository

https://github.com/tornaria/void-packages rewrite-python-shebang
https://github.com/void-linux/void-packages/pull/34053

[RFC] option to skip 03-rewrite-python-shebang
For #34030 we need to disable `pre-pkg/03-rewrite-python-shebang`, since sagemath ships its own python and rewriting shebangs breaks it (we will work on un-vendoring python, but that's a different story).

This PR adds an option `no_python_shebang=yes` to skip running this hook, similar to `nocross=yes`.

If this goes through, it will have to be added to `Manual.md` and to `xlint`, but I want to get feedback before.

Maybe, as an alternative, a way to skip hooks by name would be more generally useful? Something like `skip_hook="pre-pkg/03-rewrite-python-shebang"` with a space separated list of hooks to skip. That shouldn't be too hard to implement in `run_pkg_hooks()`.

A patch file from https://github.com/void-linux/void-packages/pull/34053.patch is attached

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: github-pr-rewrite-python-shebang-34053.patch --]
[-- Type: text/x-diff, Size: 878 bytes --]

From a1734b6383b915fce0d296324bf0c4d6cda3e930 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gonzalo=20Tornar=C3=ADa?= <tornaria@cmat.edu.uy>
Date: Thu, 11 Nov 2021 09:39:20 -0300
Subject: [PATCH] hooks: option to skip 03-rewrite-python-shebang

---
 common/hooks/pre-pkg/03-rewrite-python-shebang.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/common/hooks/pre-pkg/03-rewrite-python-shebang.sh b/common/hooks/pre-pkg/03-rewrite-python-shebang.sh
index 07162ad2c69b..0ac9d7dda65a 100644
--- a/common/hooks/pre-pkg/03-rewrite-python-shebang.sh
+++ b/common/hooks/pre-pkg/03-rewrite-python-shebang.sh
@@ -4,6 +4,10 @@
 hook() {
 	local pyver= shebang= off=
 
+	if [ -n "$no_python_shebang" ]; then
+		return 0
+	fi
+
 	if [ -d ${PKGDESTDIR}/usr/lib/python* ]; then
 		pyver="$(find ${PKGDESTDIR}/usr/lib/python* -prune -type d | grep -o '[[:digit:]]\.[[:digit:]]\+$')"
 	fi

Re: [RFC] option to skip 03-rewrite-python-shebang

[ericonr]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

New comment by ericonr on void-packages repository

https://github.com/void-linux/void-packages/pull/34053#issuecomment-968073629

Comment:
My position here is that sage shouldn't be merged into void as long as it's using built-in python.

Most of the hooks exist for good reason, and I don't think this change has enough value.

A variable to skip specific hooks is also fragile: sometimes we need to change the order they run at, and that means changing their name.

Re: [RFC] option to skip 03-rewrite-python-shebang

[dkwo]

[-- Attachment #1: Type: text/plain, Size: 253 bytes --]

New comment by dkwo on void-packages repository

https://github.com/void-linux/void-packages/pull/34053#issuecomment-968262536

Comment:
Will this be necessary after https://trac.sagemath.org/ticket/30766 is completed?
Namely, also for python modules?

Re: [RFC] option to skip 03-rewrite-python-shebang

[leahneukirchen]

[-- Attachment #1: Type: text/plain, Size: 217 bytes --]

New comment by leahneukirchen on void-packages repository

https://github.com/void-linux/void-packages/pull/34053#issuecomment-968301077

Comment:
I agree we should use system Python, but it's targetted for Sage 9.5.

Re: [RFC] option to skip 03-rewrite-python-shebang

[tornaria]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

New comment by tornaria on void-packages repository

https://github.com/void-linux/void-packages/pull/34053#issuecomment-968304770

Comment:
> My position here is that sage shouldn't be merged into void as long as it's using built-in python.

Fair enough, I tend to think the same. Hopefully it will work with python 3.10 by the time they release sagemath-9.5. If it doesn't, though, it may mean another 6 months wait for sagemath-9.6.

> Most of the hooks exist for good reason, and I don't think this change has enough value.

Doesn't scanning and replacing all python shebangs cause issues in any other situations? In this case we are mostly replacing "#! /usr/bin/env python3" or worse "#! /usr/bin/env sage-python" (which complains about lack of python version and aborts).

In particular this is NOT about hardcoding paths to the python interpreter but allowing it to run through env.

It might be argued that rewriting "#! /usr/bin/env sage-python" is incorrect.

Also: for packages with lots of files (and/or huge text files?) this hook takes its time... AFAICT it's not only checking in /usr/bin or +x files but every single non-binary file is scanned.

A similar situation is 11-pkglint-elf-in-usrshare, but for that case we have "ignore_elf_dirs=". Example package pari-galpol contains 14681 files and using `nostrip=true` plus `ignore_elf_dirs=/usr/share/pari` saves ~ 3m in install step. It may not sound much for CI, but when testing changes locally it's a workflow killer.

> A variable to skip specific hooks is also fragile: sometimes we need to change the order they run at, and that means changing their name.

That's a good point. It could be made different, e.g. `skip_hooks="pre-pkg:rewrite-python-shebang"` where the first part matches a stage exactly and the second part matches partially, so this is robust to change in the numbers. But I understand this gets a bit hairy...

Would one of the following be acceptable:
 a. fix the pattern in 03-rewrite-python-shebang so it doesn't match "#!/usr/bin/env sage-python".
 b. add an option "ignore_python_shebang_dirs" similar to "ignore_elf_dirs"

Re: [RFC] option to skip 03-rewrite-python-shebang

[ericonr]

[-- Attachment #1: Type: text/plain, Size: 2316 bytes --]

New comment by ericonr on void-packages repository

https://github.com/void-linux/void-packages/pull/34053#issuecomment-968341587

Comment:
> Doesn't scanning and replacing all python shebangs cause issues in any other situations?

There used to be a bug with replacing it without taking into account the flags passed to the interpreter in the shebang, but that's been fixed.

> In particular this is NOT about hardcoding paths to the python interpreter but allowing it to run through env.

That is the exact purpose of the hook. It means system packages aren't affected by alternative `python` executables in `PATH`, and that the `xbps-alternatives` selected python doesn't change what's used for a given script. Since an alternative python will usually look for modules in a different place from `/usr/lib/python${py3_ver}` as well, this means that programs which depend on packaged modules continue working. (way back when, installing conda provided Python on ubuntu completely hosed a bunch of GUI programs for me, since they could no longer import `gi`, for example)

> It might be argued that rewriting "#! /usr/bin/env sage-python" is incorrect.

How does this break? Where is `sage-python` located?

> Also: for packages with lots of files (and/or huge text files?) this hook takes its time... AFAICT it's not only checking in /usr/bin or +x files but every single non-binary file is scanned.

Yup, there are packages with scripts under `/usr/share`, for example.

> A similar situation is 11-pkglint-elf-in-usrshare, but for that case we have "ignore_elf_dirs=".

`ignore_elf_dirs` is about policy: that directory can have ELF files because they aren't arch specific or because they *have* to go there. It's not about speedup, and it bothers me slightly that `pari-galpol` is using it for that purpose. The purpose of hooks is to always be there to catch things that we may miss; manually adding a bunch of skips for speed rather than policy feels wrong to me.

>  a. fix the pattern in 03-rewrite-python-shebang so it doesn't match "#!/usr/bin/env sage-python".

I can kinda see the case for that, since it isn't one of the two "official" python interpreters.

>  b. add an option "ignore_python_shebang_dirs" similar to "ignore_elf_dirs"

I don't think this is a valid option.

Re: [PR PATCH] [Closed]: [RFC] option to skip 03-rewrite-python-shebang

[tornaria]

[-- Attachment #1: Type: text/plain, Size: 863 bytes --]

There's a closed pull request on the void-packages repository

[RFC] option to skip 03-rewrite-python-shebang
https://github.com/void-linux/void-packages/pull/34053

Description:
For #34030 we need to disable `pre-pkg/03-rewrite-python-shebang`, since sagemath ships its own python and rewriting shebangs breaks it (we will work on un-vendoring python, but that's a different story).

This PR adds an option `no_python_shebang=yes` to skip running this hook, similar to `nocross=yes`.

If this goes through, it will have to be added to `Manual.md` and to `xlint`, but I want to get feedback before.

Maybe, as an alternative, a way to skip hooks by name would be more generally useful? Something like `skip_hook="pre-pkg/03-rewrite-python-shebang"` with a space separated list of hooks to skip. That shouldn't be too hard to implement in `run_pkg_hooks()`.

Re: [RFC] option to skip 03-rewrite-python-shebang

[tornaria]

[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]

New comment by tornaria on void-packages repository

https://github.com/void-linux/void-packages/pull/34053#issuecomment-968435224

Comment:
@ericonr thanks for your detailed answer. Let me add a few things:
- I misread the code and in fact `#!/usr/bin/env sage-python` is not rewritten. The first grep will match that, but there is a more precise regexp in the loop that will only match python when preceded by space or / and followed by numbers.
- the hook was failing for sagemath because some shebangs refer to `python` without a version. That's not a bug: these are standalone binaries intended to run with whatever python is already installed in the system, regardless of version (sage will force install python3 if only python2 is available; these binaries may run before that).
- adding `python_version=3` fixes that breakage in a proper way.
- allowing xbps-src to rewrite the shebangs doesn't cause terrible breakage (maybe some things are broken, I don't know yet, but if so they are relatively minor). Maybe sage is not relying on shebangs to run python files that must be run on its own venv with its own python.

I think we can close this PR for now and I'll probably drop the commit from the sagemath branch as well.

If / when we discover issues with some of the rewritten shebangs, we can try to find workarounds.