Opened 3 years ago

Closed 2 years ago

#23160 closed enhancement (fixed)

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

Reported by: embray Owned by:
Priority: major Milestone: sage-8.1
Component: build Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 48fe6ac (Commits) Commit: 48fe6acdd25929752cba413d8ed629aef436b856
Dependencies: #23179 Stopgaps:

Description

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).

Change History (39)

comment:1 Changed 3 years ago by embray

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.

comment:2 Changed 3 years ago by jdemeyer

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.

comment:3 Changed 3 years ago by jdemeyer

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.

comment:4 follow-up: Changed 3 years ago by embray

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

comment:5 in reply to: ↑ 4 Changed 3 years ago by embray

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.

comment:6 Changed 3 years ago by embray

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.

comment:7 Changed 3 years ago by embray

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

Last edited 3 years ago by embray (previous) (diff)

comment:8 follow-up: Changed 2 years ago by embray

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).

comment:9 in reply to: ↑ 8 Changed 2 years ago by jdemeyer

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.

comment:10 Changed 2 years ago by embray

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.

comment:11 Changed 2 years ago by embray

Have a look at #23179.

comment:12 Changed 2 years ago by git

  • Commit changed from cbe3643b663eae073d9b127764bff4e1e0d75715 to 2b5e33df3bdbe1013f063b7ecf47e7eca44333da

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

1b96a66Make it a mere warning on Cygwin if any of the scripts in build/pkgs are executable, but a hard error on other platforms
b1155ddUse a heredoc instead of a quoted variable, for clarity's sake.
d28f35dAdd an explicit variable indicating if this is an old-style package
4c4941fRestore support old-style packages, for now, by not creating script wrappers
a6163ceUpdated the developer docs to reflect the changes in #23179
4f52331Adds initial version of sage-dist-helpers
f07cb21Use the handful of implemented helpers in 'patch' for demonstration purposes
55b68abAdditional args shouldn't be quoted
a01a1bdAdd support for $SAGE_SUDO in all sdh_make_install calls
2b5e33dAs suggested, rename sdh_check_common_vars to sdh_guard

comment:13 Changed 2 years ago by embray

  • Dependencies set to #23179
  • Status changed from new to needs_review

Rebased on latest version of #23179

comment:14 Changed 2 years ago by git

  • Commit changed from 2b5e33df3bdbe1013f063b7ecf47e7eca44333da to 79ff115067a32af55b9fe94aac6789cc752d70de

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

bb4a3d5Add support for $SAGE_SUDO in all sdh_make_install calls
79ff115As suggested, rename sdh_check_common_vars to sdh_guard

comment:15 Changed 2 years ago by jdemeyer

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.

comment:16 Changed 2 years ago by embray

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?

comment:17 Changed 2 years ago by embray

  • Status changed from needs_review to needs_work

comment:18 Changed 2 years ago by embray

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:

https://git.sagemath.org/sage.git/diff?id2=u/embray/build/script-wrappers&id=u/embray/build/helpers

That should make reviewing this much easier.

comment:19 Changed 2 years ago by jdemeyer

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

comment:20 Changed 2 years ago by embray

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.

comment:21 Changed 2 years ago by git

  • Commit changed from 79ff115067a32af55b9fe94aac6789cc752d70de to 31151404b28cd23b5256f616d524d7414cdb1c1a

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

c447855Use a heredoc instead of a quoted variable, for clarity's sake.
f53ee4dUpdated the developer docs to reflect the changes in #23179
6e322b8Set some variables in the wrapper script itself.
caf64bfFix typos in this comment
1d10cc2Add an explicit variable indicating if this is an old-style package
aeb2459Adds initial version of sage-dist-helpers
1173561Use the handful of implemented helpers in 'patch' for demonstration purposes
4a4fd78Add support for $SAGE_SUDO in all sdh_make_install calls
3115140As suggested, rename sdh_check_common_vars to sdh_guard

comment:22 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

comment:23 Changed 2 years ago by jdemeyer

This introduces again PKG_OLD_STYLE. Is this a mistake in the branch or do you really think it is needed?

Other than that, I have no objections by looking at the patch. I still need to test, but I will do that after you fix the PKG_OLD_STYLE stuff.

comment:24 Changed 2 years ago by jdemeyer

  • Dependencies #23179 deleted
  • Status changed from needs_review to needs_work

comment:25 follow-up: Changed 2 years ago by 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.

comment:26 Changed 2 years ago by embray

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.

comment:27 in reply to: ↑ 25 Changed 2 years ago by embray

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.

comment:28 Changed 2 years ago by embray

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.

comment:29 Changed 2 years ago by git

  • Commit changed from 31151404b28cd23b5256f616d524d7414cdb1c1a to 54da5200650d59a240cd3f12ea3abe1c0b74e7c1

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

b7370f7Adds initial version of sage-dist-helpers
97203f2Use the handful of implemented helpers in 'patch' for demonstration purposes
c7c92aaAdd support for $SAGE_SUDO in all sdh_make_install calls
54da520As suggested, rename sdh_check_common_vars to sdh_guard

comment:30 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

comment:31 Changed 2 years ago by jdemeyer

  • Dependencies set to #23179
  • Status changed from needs_review to needs_work

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.

comment:32 Changed 2 years ago by embray

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

comment:33 Changed 2 years ago by git

  • Commit changed from 54da5200650d59a240cd3f12ea3abe1c0b74e7c1 to 48fe6acdd25929752cba413d8ed629aef436b856

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

48fe6acInclude the PKG_NAME, PKG_BASE, and PKG_VER variables directly in the

comment:34 follow-up: Changed 2 years ago by 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...

comment:35 in reply to: ↑ 34 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

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.

comment:36 Changed 2 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

Works for me...

comment:37 Changed 2 years ago by vbraun

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

comment:38 Changed 2 years ago by jdemeyer

  • Milestone set to sage-8.1

comment:39 Changed 2 years ago by vbraun

  • Branch changed from u/embray/build/helpers to 48fe6acdd25929752cba413d8ed629aef436b856
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.