Automatically wrap spkg-install scripts with boilerplate

[embray]

I mentioned this idea in this ticket, but to summarize the purpose of this is to make the spkg-install scripts (as well as spkg-build and spkg-check when they exist) not directly executable on their own outside of package build directories. This is to prevent them from being accidentally executed in any other context. Further, they are wrapped to source the sage-env script from the $SAGE_ROOT the package is being installed into. So if one manually enters a package build directory and runs the spkg-install it will automatically ensure that the correct Sage environment is loaded.

This obviates the need for the guard boilerplate that checks for $SAGE_LOCAL, though it can't hurt to keep, in case sage-env itself is broken somehow.

This ticket changes a ton of files, so let me just summarize the changes briefly:

• Modifies sage-spkg to write a wrapper containing the boilerplate around each sage-build/install/check script after it is copied into a package build directory.

• For all existing sage-build/install/check scripts, the executable bit is removed from the copies in the source tree in build/pkgs/, and the shebang lines are removed.

• For the handful of scripts that were written in Python, the spkg-install becomes just exec sage-python23 sage-install.py, where the original Python-based install script is moved to sage-install.py.

I think it would be good to do something like this, but it's just an idea and not strictly required for any future work.

CC: @vbraun

Component: build

Author: Erik Bray

Branch: 9dfaeeb

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/23179

[sagetrac-git]

Changed commit from 16a51f0 to 176356a

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

176356a

Remove this check as it is no longer needed

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f39f099	Mark all spkg-build|install|check as not executable
94d2626	Create wrapper script around spkg-build, spkg-check, and spkg-install
7a7424b	In accordance with the new restrictions, strips shebank lines from all spkg-install, spkg-build, and spkg-check
d30624b	Remove this check as it is no longer needed

[sagetrac-git]

Changed commit from 176356a to d30624b

[embray]

comment:4

Rebased.

[jdemeyer]

comment:5

I don't really like this idea.

1. Having those scripts executable can be useful for debugging.

2. I would still want to keep the check if [ "$SAGE_LOCAL" = "" ]; then (possibly in a different form) to guard against accidental execution outside of a Sage shell. This is important, since there can be severe consequences if $SAGE_LOCAL is not set.

3. Too much magic makes the logic harder to understand.

[embray]

comment:6

Replying to @jdemeyer:

I don't really like this idea.

1. Having those scripts executable can be useful for debugging.

I'm not aware of any scenario that's impeded by this. Could you give me an example?

2. I would still want to keep the check if [ "$SAGE_LOCAL" = "" ]; then (possibly in a different form) to guard against accidental execution outside of a Sage shell. This is important, since there can be severe consequences if $SAGE_LOCAL is not set.

Right, as I wrote in the ticket I would still keep this guard, and the goal is not to remove it. That said, I don't see why this is a big deal. I'm not aware of any other packaging system that worries about this. If the consequences can be "severe" that's maybe a bigger problem.

In any case, checking $SAGE_LOCAL isn't always completely sufficient either, to prevent unwanted side-effects. What I'm proposing I believe is a lot safer because it discourages execution of these scripts outside of the appropriate context.

3. Too much magic makes the logic harder to understand.

I've tried giving this some thought, but to be honest I really don't know what you mean by this at all.

[jdemeyer]

comment:7

I'm not aware of any other packaging system that worries about this.

Well, other packaging systems aren't written in a very fragile language like bash :-)

Anyway, let me reconsider this ticket. I am starting to understand better why it's useful.

[embray]

comment:8

True, but as far as I can tell the problem here is not that what language the packaging system itself, however you define that, is written in.

The way I see it, on most systems, the danger of accidentally installing outside SAGE_LOCAL is mitigated by permissions--you wouldn't be running these scripts with sudo. And if you are doing anything with sudo it sort of becomes your responsibility what happens.

I think at the end of the day this makes our scripts safer because they are no executable by default (if you really wanted to you could still run them with bash <filename>), and the wrapped scripts will always source the correct sage-env for the Sage that you're installing into.

[jdemeyer]

comment:9

1. Instead of this:

if [ -f "/usr/local/src/sage-config/src/bin/sage-env" ]; then
    . "/usr/local/src/sage-config/src/bin/sage-env"
else
    echo >&2 "Error: sage-env not found; is /usr/local/src/sage-config the correct SAGE_ROOT?"
    exit 1
fi

I would prefer something more like

source "/usr/local/src/sage-config/src/bin/sage-env"
if [ $? -ne 0 ]; then
    echo >&2 "Error: failed to source sage-env; is /usr/local/src/sage-config the correct SAGE_ROOT?"
    exit 1
fi

At the very least, you need to check for errors when sourcing sage-env.

2. If you really want to make the spkg-install script be usable outside of a Sage shell, starting with a cd statement into the correct directory would be good too. Otherwise you get

$ /usr/local/src/sage-config/local/var/tmp/sage/build/patch-2.7.5/spkg-install 
/usr/local/src/sage-config/local/var/tmp/sage/build/patch-2.7.5/spkg-install: line 45: ./configure: No such file or directory
Error configuring GNU patch

3. grep -q -x -m 1: I have some doubts about the portability of these options. GNU grep may have them, but we should support other grep implementations too.

4. For readability, assign ".tmp-$script" to a variable: tmpscript=".tmp-$script".

5. In sage-spkg, there is a check for a spkg-install implemented in Python. That is obsolete now.

[jdemeyer]

comment:11

6. I find it a bit strange that you are treating a #! line in the spkg-install file as a hard error, while the executable bit is only a warning. I think the executability is something that we explicitly do not want, so that should be an error.

[embray]

comment:12

Replying to @jdemeyer:

6. I find it a bit strange that you are treating a #! line in the spkg-install file as a hard error, while the executable bit is only a warning. I think the executability is something that we explicitly do not want, so that should be an error.

That can be a problem on Cygwin where, depending on how permission emulation is configured, all files may be treated as executable even if they aren't really (annoying, yes). My configuration isn't like that but it's not guaranteed.

[embray]

comment:13

Replying to @jdemeyer:

3. grep -q -x -m 1: I have some doubts about the portability of these options. GNU grep may have them, but we should support other grep implementations too.

These options are available on BSD grep as well, including on OSX. If someone can point out a specific implementation where they're not then I can change it.

[jdemeyer]

comment:14

Replying to @embray:

If someone can point out a specific implementation where they're not then I can change it.

Since you challenged me, OpenBSD grep does not have -m. But really, there is no reason to use -m, you can use head -1 | grep. And instead of using -x, you can match a whole line with ^regex$. And instead of using -q, use > /dev/null.

[embray]

comment:15

Replying to @jdemeyer:

Replying to @embray:

If someone can point out a specific implementation where they're not then I can change it.

Since you challenged me, OpenBSD grep does not have -m. But really, there is no reason to use -m, you can use head -1 | grep. And instead of using -x, you can match a whole line with ^regex$. And instead of using -q, use > /dev/null.

Okay, that's fine--obviously all of these alternatives are possible just annoying.

[embray]

comment:16

Replying to @embray:

Replying to @jdemeyer:

6. I find it a bit strange that you are treating a #! line in the spkg-install file as a hard error, while the executable bit is only a warning. I think the executability is something that we explicitly do not want, so that should be an error.

That can be a problem on Cygwin where, depending on how permission emulation is configured, all files may be treated as executable even if they aren't really (annoying, yes). My configuration isn't like that but it's not guaranteed.

Perhaps for Cygwin I could just output a warning, but make it an error everywhere else, where presumably we can rely on permissions being sane.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

d5399f7	Don't use not entirely portable GNU grep features
3c34582	Instead of looking before we leap, always try sourcing sage-env, and handle any errors that might occur
38647dc	Pass write_script_wrapper the absolute path to the script it's writing
8cd0049	Use a variable for the path to the temporary file for clarity's sake
01e3c03	Remove this check from sage-spkg since spkg-installs will no longer be written in Python

[sagetrac-git]

Changed commit from d30624b to 01e3c03

[embray]

Reviewer: Jeroen Demeyer

[embray]

comment:18

I think this addresses all your comments so far. If this needs any further tweaks that's fine, but even if not please set this back to needs work so that I can work on updating the developer documentation, assuming there are no further objections to this in principle.

[embray]

comment:19

Sorry, I think there's a bug somewhere.

[sagetrac-git]

Changed commit from 6e322b8 to caf64bf

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

caf64bf

Fix typos in this comment

[kiwifb]

comment:57

For info I have marked #15674 in your list as duplicate since it was superseded by #22817 which was merged.

[jdemeyer]

comment:59

Setting to blocker to get the attention of the release manager.

Volker: since this touches a lot of files, it would be best to merge this early in Sage 8.1 to avoid conflicts.

[vbraun]

comment:61

Successfully installed scipy-0.17.1.p0
Running the test suite for scipy-0.17.1.p0...
/mnt/disk/home/buildslave-sage/slave/sage_git/build/local/bin/python2: can't open file 'sage-check.py': [Errno 2] No such file or directory

[jdemeyer]

comment:62

Typo: sage-check.py -> spkg-check.py

[sagetrac-git]

Changed commit from caf64bf to 9dfaeeb

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

9dfaeeb

Fix obvious typo

[embray]

comment:64

This was just a typo as confirmed by Jeroen so I'll set it back to positive review.

[vbraun]

Changed branch from u/embray/build/script-wrappers to 9dfaeeb

[kiwifb]

comment:66

I just flagged #23498 which was merged at the same time for follow up. Do we have a ticket for mopping up any new/upgrade packages that will trickle in until people are fully aware of the new system? We may have to do a bit of spotting and pre-emptive reviewing for a while.

[kiwifb]

Changed commit from 9dfaeeb to none

[embray]

comment:67

I mean, I made a big list of tickets that are affected by this: #23179 comment:45 But #23498 slipped through the cracks.

[kiwifb]

comment:68

Yes the list of "older" ticket is fine. I already warned one upgrade (arb) on rebasing on this ticket before 8.1.beta1 was released. But apart from those one, we need to be careful of new package/upgrade ticket for a while as well.