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

[embray]

comment:24

I think #20721, or something like it, should be a prerequisite to this ticket (or any like it depending on how much I end up reworking this).

[embray]

Changed dependencies from #20721 to #20721 20837

[embray]

comment:25

20837 cleans up the handful of spkgs that don't conform to a format where calling patch -p1 < ../patches/*.patch should work from within the source root.

Then we can move forward on appling sage-apply-patches by default on all spkgs.

[mkoeppe]

Changed dependencies from #20721 20837 to #20721, #20837

[mkoeppe]

comment:27

What remains to be done for this ticket? Would be good to use this for #20892.

[embray]

comment:28

Mainly: sage-apply-patches needs to be changed to fail quietly if no patches are found (I think I already did this; might still be on a local branch)

Then remove all, or almost all calls to sage-apply-patches from individual spkg-install scripts, and instead call it on all packages as a standard step in sage-spkg.

I can get this done soon.

[embray]

comment:29

Strange, I thought I rebased this. Maybe I just forgot to push.

[embray]

Changed dependencies from #20721, #20837 to #20721, #20837, #20933

[embray]

comment:30

Turns out the patches for MathJax needed a bit more tweaking, as they were not uniform with the other packages.

[mkoeppe]

comment:31

Should there be a way to control the order in which patches are applied? For example Debian uses a file "series" that controls this. See for example #20901, for which I'd like to lift the patches from debian.

[embray]

comment:32

Could we just prefix the patch filenames with numbers?

[kiwifb]

comment:33

Replying to @embray:

Could we just prefix the patch filenames with numbers?

+1 that's the easiest way to get order.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 29bd199 to 2c1ec4f

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

5891448	Clean up patching of rubiks
18139d8	Adds a new sage-apply-patches script.
a46a816	Fix accidental duplication of patch_subdir.
5458959	Forgot to change this to loop over the full list when I made $patches into a list.
39645a9	Mathjax doesn't have a source directory to cd into, so uses a different default patch path from most packages.
78a151b	None of these packages currently have patches to apply.
d15fb43	Add license notice.
ca832e3	Warn but don't fail if there are no patches to apply
2c1ec4f	Apply sage-apply-patches standard on all packages during spkg-install

[embray]

comment:35

Last I poked at this not all the dependencies were satisfied. Now they are, and I have rebased the branch again on develop.

Just did a test build of this on Linux and everything (modulo optional packages which were not tested) works, but YMMV?

[jdemeyer]

Changed dependencies from #20721, #20837, #20933 to #21103

[jdemeyer]

comment:37

This should still depend on #21103.

[jdemeyer]

comment:38

Note that I might look at #21103 and this ticket, but not right now since I'm working on #20218 and #21256.

[83660e46-0051-498b-a8c1-f7a7bd232b5a]

comment:39

Replying to @embray:

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

+1 (You're not the first suggesting this...)

[83660e46-0051-498b-a8c1-f7a7bd232b5a]

comment:40

P.S.: The main problem to that time was that (legacy) spkgs didn't depend on the Sage version, so we would have had to add fall-backs first (if the script to source from wasn't available).

Error-handling (i.e., the messages) is another thing that should be standardized.

[embray]

comment:41

Sounds good--I'll revisit that later in a separate ticket. As it is this is a step in the right direction since it just standardizes the "apply patches (if any)" step across all spkgs.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

6bee0df	Adds a new sage-apply-patches script.
fc0d294	Fix accidental duplication of patch_subdir.
0e1b32c	Forgot to change this to loop over the full list when I made $patches into a list.
47c69f1	Mathjax doesn't have a source directory to cd into, so uses a different default patch path from most packages.
d2f3eaa	None of these packages currently have patches to apply.
27cc968	Add license notice.
6c9d6d1	Warn but don't fail if there are no patches to apply
229199f	Apply sage-apply-patches standard on all packages during spkg-install
5b00b25	Fixup
5343f3b	Use sage-apply-patches in more packages.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 2c1ec4f to 5343f3b

[embray]

comment:43

Rebased this yet again, incorporating changes for new packages that were added or updated. Did a build/test of this branch on Linux and everything looks fine.

I'm clearing the old milestone--will leave it to the release manager to set an appropriate one.

[jdemeyer]

comment:44

From reading the documentation in sage-apply-patches, I don't quite understand the meaning of the [-d patch-subdir] and [patch-dir] arguments. Are these arguments related? If yes, why is it not just 1 argument?

[jdemeyer]

comment:45

In build/pkgs/openblas/spkg-install, you make a non-trivial change. Why that?

[jdemeyer]

comment:46

Also, you are deleting rubiks patches...