Add sage-apply-patches helper script for use in spkg-install scripts

[embray]

This adds a new script to build/bin called sage-apply-patches which implements the same code that is copy/pasted through many of the spkg-install scripts (albeit often inconsistently). The patches in build/pkgs/PKGNAME/patches are now applied automatically.

I'm not attached to any of the details of how the script works--the implementation was designed mostly to make it easiest to automatically replace the existing repeated code snippet with minimal effort.

Component: build

Author: Erik Bray

Branch: b6b33cd

Reviewer: Jeroen Demeyer

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

[jdemeyer]

comment:2

Seems I have to undust my bash scripting skills...

Why is it an error if there are no patches to apply? I would remove

if [[ -r "${patches[0]}" ]]; then
else
    >&2 echo "No patch files found in $patchdir"
    exit 1
fi

and just apply all patches, which may be the empty set.

[jdemeyer]

comment:3

A minor thing: I would prefer to remove the -pNUM option. It's better if we consistently use -p1 in all cases. That's also how git generates its patches.

[jdemeyer]

comment:4

I don't get this:

and it is assumed that the patches are being applied from
the root of the package source

[jdemeyer]

comment:5

Why not always use the option --no-backup-if-mismatch instead of only for PARI?

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:7

Following up on [comment:2]: it's very useful to support an empty set of patches. For example, when upgrading a package, it can happen that all patches need to be removed. You don't want that to break the build. That is also the reason why many packages have code to apply patches when they don't have patches. I would not remove such code since it allows easily adding a new patch. Ideally, every spkg-install script would call spkg-apply-patches.

[embray]

comment:8

Replying to @jdemeyer:

Following up on [comment:2]: it's very useful to support an empty set of patches. For example, when upgrading a package, it can happen that all patches need to be removed. You don't want that to break the build. That is also the reason why many packages have code to apply patches when they don't have patches. I would not remove such code since it allows easily adding a new patch. Ideally, every spkg-install script would call spkg-apply-patches.

I guess I was thinking it was wasteful to call a process (especially on Windows where process creation is much slower) when it's not needed. Granted, when this was inline in every script it didn't require a process creation (more on that in a followup). I think if someone wants to add a patch to a package they should explicitly add spkg-appy-patches as well. If they forget to do that, well, then they clearly didn't test that their patch applies at build time :)

As for it being an error when no patches are found, I have no strong feelings. I liked that it was explicit, but it could just as will either print a message, or go completely silent.

A minor thing: I would prefer to remove the -pNUM option. It's better if we consistently use -p1 in all cases. That's also how git generates its patches.

That's fine by me. I think there was one case, maybe two where it was needed, but it would be just as easy to update those patch files and keep things consistent throughout.

Why not always use the option --no-backup-if-mismatch instead of only for PARI?

I figured maybe there was a reason to prefer to keep backups in general. Would be fine with me to apply in general though. Previously there was one other case where additional arguments had to be passed to patch, but then I realized that package had no patches anymore.

I don't get this:

and it is assumed that the patches are being applied from
the root of the package source

Meaning the patches are applied after cd-ing into the upstream source, hence the default path to search for patches being ../patches. No reason it has to be that way but it was the most minimal change from the existing code.

[embray]

comment:9

An alternative I considered to adding a new script was to create a little library of functions that is sourced in every spkg-install. It would include sage-apply-patches reimplemented as a function, along with a few other bits that are repeated a lot. This would save on process creations too.

I'd still be happy to revisit that idea too if it sounds good, but right now I just wanted to focus on the patch thing since that's what was irking me specifically :)

[jdemeyer]

comment:10

Replying to @embray:

I guess I was thinking it was wasteful to call a process (especially on Windows where process creation is much slower) when it's not needed.

How much slower is it really? Surely it's negligible compared to the time to build Sage.

I think if someone wants to add a patch to a package they should explicitly add spkg-appy-patches as well.

That's really annoying. Please keep the call to spkg-apply-patches even when there are no patches.

[embray]

comment:11

If you feel that strongly about it. I think it's pointless. Especially considering that it was already inconsistent. Instead you should be asking me to put sage-apply-patches in every spkg-install that didn't have it before :)

[jdemeyer]

comment:12

Replying to @embray:

Instead you should be asking me to put sage-apply-patches in every spkg-install that didn't have it before :)

Well, that's essentially what I wrote in [comment:7].

And yes, I feel strongly about because I don't want to always add/remove the call to spkg-apply-patches depending on whether or not the number of patches to apply equals zero.

Detail: for consistency, I would prefer the script to be called spkg-apply-patches instead of sage-apply-patches.

[embray]

comment:13

What would that be consistent with? Every other script in build/bin starts with sage-. In fact very nearly every, if not every executable installed by sage starts with sage- which is good and consistent, and completely free of potential for overlap with anything else.

[jdemeyer]

comment:14

Consistent with spkg-install, spkg-check, spkg-src. Basically, anything related to building Sage packages.

[embray]

comment:15

Those scripts are only ever in spkg directories though and are specific to each spkg. They are not in bin/ paths added to $PATH. This is a helper script used in installation like the rest in build/bin, all of which (sensibly) start with sage-.

I'm probably also going to add a sage-spkg-python (or something, I don't care what it's called) that will be exec'd by the spkg-install for most Python packages since the vast majority of them are cookie-cutter anyways (especially if you also want to have every one of them calling whatever-apply-patches by default). This would be to provide a solution for #16897 which also calls out this fact. (I think it's important for every package to have a file called spkg-install, but it's fine if its sole function is to call some other generic script).

[jdemeyer]

comment:16

For the record: I just wanted to comment over the name, I'm not going to reject this patch over such bikeshedding.

[kiwifb]

comment:17

While I am OK with the stuff as it is, I have a bold suggestion for further work: may be it should be put in sage-spkg and always be run. We already leave it when there is no patches anymore. What would be the overhead of always running it?

[jdemeyer]

comment:18

Replying to @kiwifb:

may be it should be put in sage-spkg and always be run.

Not easily. We need to guess in which source directory to run the script. Usually, it's src but not always.

[kiwifb]

comment:19

Replying to @jdemeyer:

Replying to @kiwifb:

may be it should be put in sage-spkg and always be run.

Not easily. We need to guess in which source directory to run the script. Usually, it's src but not always.

Tricky. I didn't remember we had those outliers.

[embray]

comment:20

Let me think about that a bit more though. If it were done in sage-spkg it would do away with the need for a separate script entirely, and would cut down that much more on the duplication which is what I want.

One possibility would be to rewrite the patch files to include the source directory in the paths, but that could be annoying and error-prone. The other would be to standardize src. I knew there were outliers but I never looked into why. Is there really no way those could be changed?

[embray]

comment:21

sage-spkg contains the following:

     # Strip file extension from the archive file and hope (as per
     # automake standards) that this is the same as the directory name
     # inside; move the directory to "src".  (This goes wrong for
     # upstream package archives that have been renamed to appease some
     # other Sage script, such as "latte-int", whose archive is renamed
     # to "latte_int".)
     mv "$(echo "$PKG_NAME_UPSTREAM" | sed 's/\.zip$//g;s/\.tgz$//g;s/\.tar\.    gz$//g;s/\.tar\.xz$//g;s/\.tar\.lz$//g;s/\.tar\.bz2$//g;s/\.tar$//g')"* src

Maybe this should be redone. There are a number of ways it might be done differently. One possibility is that it could be made sage-uncompress-spkg's job to always extract to src. It could handle checking for the top-level directory in the archive and extracting it instead as src. For those handful of packages (are there any? I suspect probably...) that don't have a top-level directory it would create src and extract all the archive contents into it.

[kiwifb]

comment:22

This is getting out of the original scope (and I am sorry to have started it) but it would be a good idea to have something more robust for unpacking in a standard folder. May be we should just get this in now and move that idea to another ticket?

[embray]

comment:23

Replying to @kiwifb:

This is getting out of the original scope (and I am sorry to have started it) but it would be a good idea to have something more robust for unpacking in a standard folder. May be we should just get this in now and move that idea to another ticket?

I was going to suggest the same. Though I might address the subject of extracting to a standard location before coming back to and reworking this ticket. I've gone through the list of spkgs that don't wind up with their sources in src and in each case there's no good reason really, except that the hack above doesn't work for it (or the last time the file was touched predates that hack).

I think a patch to sage-uncompress-spkg would be a good idea and I'll put something to that effect in a separate ticket.

[embray]

Dependencies: #20721

[jdemeyer]

comment:84

I have no further objections. I'm currently building Sage from scratch with this branch. If this works fine, I will set this to positive_review (hoping that there are no conflicts).

[embray]

comment:85

Awesome, thanks!

[vbraun]

comment:87

Merge conflict

[sagetrac-git]

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

ad5dcd6	Adds a new sage-apply-patches script.
2858b5f	Added some updated documentation about how to apply patches
52a8ca0	Reduce verbosity--no need for the 'Applying' message, and no real need to use pushd/popd either since sage-apply-patches does not change directories.

[sagetrac-git]

Changed commit from addd3ee to 52a8ca0

[embray]

comment:89

Rebased.

[jdemeyer]

Changed branch from u/embray/patch-cleanup to u/jdemeyer/patch-cleanup

[jdemeyer]

comment:91

I fixed a minor bikeshed: I prefer no newline after ./configure in

./configure ...

if [ $? -ne 0 ]; then
    ...
fi

It makes it more clear that the if really belongs with the ./configure.

I don't know if you intentionally added a newline there in build/pkgs/cysignals/spkg-install, but I removed it now. I also squashed the commits together.

---

New commits:

b6b33cd

Adds a new sage-apply-patches script.

[jdemeyer]

Changed commit from 52a8ca0 to b6b33cd

[vbraun]

Changed branch from u/jdemeyer/patch-cleanup to b6b33cd

[jdemeyer]

comment:93

Finally merged, thanks Erik!

[jdemeyer]

Changed commit from b6b33cd to none

[embray]

comment:94

Hooray!
pours you a champagne