Add a library of common helper functions for use in spkg-install

[embray]

This idea has come up before but not been implemented. My implementation simply adds a collection of shell functions with names prefixed by sdh_ for Sage distribution helpers (this is somewhat inspired by, but much simpler than, the dh_ programs from the debhelper package in Debian).

These functions are exported by sage-spkg, and so are immediately usable by any spkg-build, spkg-install, or similar scripts executed from sage-spkg (these are still not required to be shell scripts, but as most are having such a library is convenient).

One piece of boilerplate I replaced with a function is the check in most spkg-installs for $SAGE_LOCAL. A downside to this is that if the script is run outside the correct environment, then rather than getting a useful error message we just get that sdh_check_vars is undefined. I have two thoughts on this:

1. In general these scripts shouldn't be run directly in the first place. In the case that they are (such as testing) the person developing the package should have some awareness of the fact that these scripts are being run from sage-spkg and assume sage-env has been sourced.

2. Most of the time these scripts are run, it's after they've been copied to a temporary build directory for the package. If nothing else, sage-spkg could create a wrapper around these scripts that automatically sets some default assumptions (such as sourcing sage-env and the new sage-dist-helpers).

In general, though, I think having a library of helper functions will be a big improvement to the consistency and legibility of theses scripts.

This ticket also updates the spkg-install for patch for demonstration purposes only. I don't intend with this to go on a mass rewriting of install scripts, but having this will be helpful for some other work (e.g. #22509, #22510).

Depends on #23179

Component: build

Author: Erik Bray

Branch/Commit: 48fe6ac

Reviewer: Jeroen Demeyer

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

[embray]

comment:1

This could also replace the $PIP_INSTALL variable that's used for Python packages with an sdh_pip_install helper, serving the same purpose, but that could be done separately.

[jdemeyer]

comment:2

The whole point of this

if [ -z "$SAGE_LOCAL" ]; then
    echo >&2 "Error: SAGE_LOCAL undefined - exiting..."
    echo >&2 "Maybe run 'sage -sh'?"
    exit 1
fi

is to abort when the variable is not defined. A plain sdh_check_common_vars does not do that. At least replace it by sdh_check_common_vars || exit $? or something like that.

[jdemeyer]

comment:3

I would also rename sdh_check_common_vars to something like sdh_guard (or sdh_begin or sdh_check). It's not important what sdh_check_common_vars does, only that it guards against accidental executing of spkg-install when the necessary variables are not defined.

[embray]

comment:4

I'm not sure I understand. It's already defined to exit the script immediately.

[embray]

comment:5

Replying to @embray:

I'm not sure I understand. It's already defined to exit the script immediately.

Oh, you mean if it's not defined in the first place.

[embray]

comment:6

One point I will make though, is that if more of the build functionality is moved into helper functions there's also less effect a script can have if it's run outside the sage environment. For example if sdh_make_install is not defined, then nothing will be installed. That's no a silver bullet though, so I can still see the usefulness of having this guard in place.

[embray]

comment:7

Okay, I'd be fine with something like sdh_guard || exit $?. It's still boilerplate but at least now it's only one line.

[embray]

comment:8

That said, this brings me back around to the idea I was mulling earlier, that maybe the spkg-* scripts that live in the package directories shouldn't be executable in the first place. That as, they would not have executable permissions, and not have a shebang line.

Only when the script is copied to the temporary install directory does it get marked executable, and gets a header automatically written which, among other things, ensures that the correct sage-env is sourced. So there's generally no danger in running it outside the correct sage environment.

This might limit it to being a shell script, but it could still be a wrapper for a non-shell executable if desired (there are only a couple cases of this currently, such as atlas).

[jdemeyer]

comment:9

Replying to @embray:

Only when the script is copied to the temporary install directory does it get marked executable, and gets a header automatically written which, among other things, ensures that the correct sage-env is sourced.

My gut feeling is that this is a bit too much magic. At least the sdh_guard || exit $? boilerplate is easy to understand with little surprises.

[embray]

comment:10

That was my feeling too at first, which is why I dropped the idea initially. But now that I've tried it I think it has a number of advantages, including the fact that it automatically makes these scripts more robust without their authors having to think about details that aren't immediately related to building/installing the package.

[embray]

comment:11

Have a look at #23179.

[sagetrac-git]

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

1b96a66	Make it a mere warning on Cygwin if any of the scripts in build/pkgs are executable, but a hard error on other platforms
b1155dd	Use a heredoc instead of a quoted variable, for clarity's sake.
d28f35d	Add an explicit variable indicating if this is an old-style package
4c4941f	Restore support old-style packages, for now, by not creating script wrappers
a6163ce	Updated the developer docs to reflect the changes in #23179
4f52331	Adds initial version of sage-dist-helpers
f07cb21	Use the handful of implemented helpers in 'patch' for demonstration purposes
55b68ab	Additional args shouldn't be quoted
a01a1bd	Add support for $SAGE_SUDO in all sdh_make_install calls
2b5e33d	As suggested, rename sdh_check_common_vars to sdh_guard

[sagetrac-git]

Changed commit from cbe3643 to 2b5e33d

[embray]

comment:13

Rebased on latest version of #23179

[embray]

Dependencies: #23179

[sagetrac-git]

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

bb4a3d5	Add support for $SAGE_SUDO in all sdh_make_install calls
79ff115	As suggested, rename sdh_check_common_vars to sdh_guard

[sagetrac-git]

Changed commit from 2b5e33d to 79ff115

[jdemeyer]

comment:15

Needs to be rebased on the latest version of #23179. Given that I'll probably wait with a review for this ticket until after the next beta, you could also wait until then and then rebase on 8.1.beta0.

[embray]

comment:16

True, it needs to be rebased now. I don't mind the timing, but why specifically after the next beta? Or is that just the next point at which #23179 will be merged into develop?

[embray]

comment:18

Actually, just to be clear, if you (or anyone else) wants to review this sooner you can still see the diff between this branch and the branch for #23179 like so:

sagemath/sagetrac-mirror@u/embray/build/helpers...u/embray/build/script-wrappers

That should make reviewing this much easier.

[jdemeyer]

comment:19

I'd rather be sure that #23179 really is merged. It wouldn't surprise me if problems would come up with that ticket.

[embray]

comment:20

It wouldn't surprise me either, though not much of this ticket is really strongly dependent on #23179. It only affects how sdh_guard is used. Up to you though.

[sagetrac-git]

Changed commit from 79ff115 to 3115140

[sagetrac-git]

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

c447855	Use a heredoc instead of a quoted variable, for clarity's sake.
f53ee4d	Updated the developer docs to reflect the changes in #23179
6e322b8	Set some variables in the wrapper script itself.
caf64bf	Fix typos in this comment
1d10cc2	Add an explicit variable indicating if this is an old-style package
aeb2459	Adds initial version of sage-dist-helpers
1173561	Use the handful of implemented helpers in 'patch' for demonstration purposes
4a4fd78	Add support for $SAGE_SUDO in all sdh_make_install calls
3115140	As suggested, rename sdh_check_common_vars to sdh_guard

[embray]

comment:26

Oops, that was not intentional. It's possible I pushed from a different computer where I still had that commit in the branch. I'm a little confused by that because normally I would have made sure to sync up first, but it was 3 weeks ago so I don't remember.

[embray]

comment:27

Replying to @jdemeyer:

This may or may not be for a follow-up ticket, but in build/pkgs/lcalc/spkg-install (and others), there is

success() {
    if [ $? -ne 0 ]; then
        echo >&2 "Error building the Lcalc package: '$1'"
        exit 1
    fi
}

I guess a helper function like that would be useful to have.

Yes, I already added something like that in #22509, but it might be worth pulling into a separate ticket. I just added it there because it became increasingly useful.

[embray]

comment:28

I see why I'm confused. This needs to be rebased on the final version of #23179. I don't know why you removed the dependency. I think it's useful to have listed in the ticket even if the dependency has been completed.

[sagetrac-git]

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

b7370f7	Adds initial version of sage-dist-helpers
97203f2	Use the handful of implemented helpers in 'patch' for demonstration purposes
c7c92aa	Add support for $SAGE_SUDO in all sdh_make_install calls
54da520	As suggested, rename sdh_check_common_vars to sdh_guard

[sagetrac-git]

Changed commit from 3115140 to 54da520

[jdemeyer]

comment:31

If you intend that spkg-install can be run without sage-spkg (I assume that was part of the intention of #23179), the following should be done in spkg-install instead of sage-spkg:

# Export package information for use from within the scripts.
export PKG_NAME PKG_BASE PKG_VER

These could be handled analogously to SAGE_ROOT with the values hard-coded in the generated spkg-install file.

[jdemeyer]

Dependencies: #23179

[embray]

comment:32

Agreed. I already fixed that in #22509 here. I should have just fixed that in this branch though.

[sagetrac-git]

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

48fe6ac

Include the PKG_NAME, PKG_BASE, and PKG_VER variables directly in the

[sagetrac-git]

Changed commit from 54da520 to 48fe6ac

[embray]

comment:34

If this otherwise works for you, I'm also going to add some updates to the developer documentation. Of course, I will continue to update the docs as more helpers are added...

[embray]

comment:35

Replying to @embray:

If this otherwise works for you, I'm also going to add some updates to the developer documentation. Of course, I will continue to update the docs as more helpers are added...

On second thought; as it stands with this ticket the helpers that it adds are few in number and trivial enough that there's not enormous advantage to using them. This will change, however, after a few more updates that will add some additional helpers. But maybe the feature isn't worth advertising until it's more complete. In the meantime it will be useful to have the framework in place so that it can be built on.

[jdemeyer]

comment:36

Works for me...

[jdemeyer]

Reviewer: Jeroen Demeyer

[vbraun]

comment:37

You want this to be merged? There is no milestone set...

[vbraun]

Changed branch from u/embray/build/helpers to 48fe6ac