Opened 5 years ago

Closed 4 years ago

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

Reported by: Owned by: embray minor sage-7.5 build Erik Bray Jeroen Demeyer N/A b6b33cd

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.

### comment:1 Changed 5 years ago by embray

• Status changed from new to needs_review

### comment:2 Changed 5 years ago by jdemeyer

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.

### comment:3 Changed 5 years ago by jdemeyer

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.

### comment:4 Changed 5 years ago by jdemeyer

I don't get this:

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

### comment:5 Changed 5 years ago by jdemeyer

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

### comment:6 Changed 5 years ago by jdemeyer

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

### comment:7 follow-up: ↓ 8 Changed 5 years ago by jdemeyer

Following up on 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.

Last edited 5 years ago by jdemeyer (previous) (diff)

### comment:8 in reply to: ↑ 7 ; follow-up: ↓ 10 Changed 5 years ago by embray

Following up on 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.

### comment:9 follow-up: ↓ 39 Changed 5 years ago by 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 :)

### comment:10 in reply to: ↑ 8 Changed 5 years ago by jdemeyer

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.

### comment:11 follow-up: ↓ 12 Changed 5 years ago by embray

If you feel that strongly about it. I think it's pointless.

Version 0, edited 5 years ago by embray (next)

### comment:12 in reply to: ↑ 11 Changed 5 years ago by jdemeyer

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

### comment:13 Changed 5 years ago by embray

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.

### comment:14 Changed 5 years ago by jdemeyer

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

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). ### comment:16 Changed 5 years ago by jdemeyer For the record: I just wanted to comment over the name, I'm not going to reject this patch over such bikeshedding. ### comment:17 follow-up: ↓ 18 Changed 5 years ago by fbissey 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? ### comment:18 in reply to: ↑ 17 ; follow-up: ↓ 19 Changed 5 years ago by jdemeyer Replying to fbissey: 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. ### comment:19 in reply to: ↑ 18 Changed 5 years ago by fbissey Replying to jdemeyer: Replying to fbissey: 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. ### comment:20 Changed 5 years ago by embray 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? ### comment:21 Changed 5 years ago by embray 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.

### comment:22 follow-up: ↓ 23 Changed 5 years ago by fbissey

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?

### comment:23 in reply to: ↑ 22 Changed 5 years ago by embray

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.

### comment:24 Changed 5 years ago by embray

• Dependencies set to #20721

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

### comment:25 Changed 5 years ago by embray

• Dependencies changed from #20721 to #20721 20837

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

### comment:26 Changed 5 years ago by mkoeppe

• Dependencies changed from #20721 20837 to #20721, #20837

### comment:27 Changed 5 years ago by mkoeppe

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

### comment:28 Changed 5 years ago by embray

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.

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

### comment:29 Changed 5 years ago by embray

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

### comment:30 Changed 5 years ago by embray

• Dependencies changed from #20721, #20837 to #20721, #20837, #20933

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

### comment:31 Changed 5 years ago by mkoeppe

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.

### comment:32 follow-up: ↓ 33 Changed 5 years ago by embray

Could we just prefix the patch filenames with numbers?

### comment:33 in reply to: ↑ 32 Changed 5 years ago by fbissey

Could we just prefix the patch filenames with numbers?

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

### comment:34 Changed 5 years ago by git

• Commit changed from 29bd19903b16289c178e9468194318e5d1017dad to 2c1ec4f6d168d43c3cde20b5b3b7fcb39450b717

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

### comment:43 Changed 5 years ago by embray

• Milestone sage-7.3 deleted
• Status changed from needs_work to needs_review

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.

### comment:44 follow-up: ↓ 49 Changed 5 years ago by jdemeyer

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?

### comment:45 Changed 5 years ago by jdemeyer

• Status changed from needs_review to needs_info

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

### comment:46 follow-up: ↓ 50 Changed 5 years ago by jdemeyer

Also, you are deleting rubiks patches...

### comment:47 Changed 5 years ago by jdemeyer

And since you rewrote git history anyway, you might as well squash everything to 1 commit.

### comment:48 Changed 5 years ago by embray

The change in openblas is a mistake made in resolving a merge conflict; will revert.

### comment:49 in reply to: ↑ 44 Changed 5 years ago by embray

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?

I think earlier, before #20837, it was more necessary to have separate arguments. But I still kind of like it as is.

The assumption here is that there is a standard root for patches to a package with may have subdirectories. The reason it's useful to have separate arguments is that spkg-install scripts don't need to care where the patch root is, but sometimes it's useful to select subdirectories within it from which to apply other patches.

I'm open to other ideas though.

### comment:50 in reply to: ↑ 46 Changed 5 years ago by embray

Also, you are deleting rubiks patches...

Looks like that's left over from before #21103--it wasn't intentional. What's odd is that everything worked fine without them so now I question whether they're even necessary but I won't think about that for now.

### comment:51 follow-up: ↓ 54 Changed 5 years ago by jdemeyer

As you probably know, I absolutely hate needless complexity. I see no reason to have two arguments doing essentially the same thing. In your current branch, both kinds of arguments are used, but all use cases could be done with either the -d patch-subdir argument or the patch-dir argument. So just pick one.

### comment:52 Changed 5 years ago by git

• Commit changed from 5343f3bef75da86de5bf2d9d8fffc1ab39f76de3 to b058fae6f1be3ca0a85007a3a3eb48daff97dab7

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

 ​b058fae Adds a new sage-apply-patches script.

### comment:53 Changed 5 years ago by embray

I reverted the accidental changes and squashed. I also want to add documentation about this to the developer docs for maintaining packages.

### comment:54 in reply to: ↑ 51 Changed 5 years ago by embray

As you probably know, I absolutely hate needless complexity. I see no reason to have two arguments doing essentially the same thing. In your current branch, both kinds of arguments are used, but all use cases could be done with either the -d patch-subdir argument or the patch-dir argument. So just pick one.

I see what you mean about using it inconsistently--well really there's one case of that in singular. There's a reason for that though. The way the spkg-install for singular is written (talk about "needless complexity") is it has a bunch of sub-routine that executes in a loop. Before each subroutine it cd's in to the src/ directory (of the singular sources, so from the point of view of spkg-install that's src/src which is different from the default assumption made in sage-apply-patches.

Really if I were using sage-apply-patches as I intended here, it should call sage-apply-patches -d debug "$PATCH" :) (though it looks like I cd .. first, so I guess the$PATCH part can go away, maybe...)

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

### comment:55 Changed 5 years ago by git

• Commit changed from b058fae6f1be3ca0a85007a3a3eb48daff97dab7 to f92d0eff9c063c6f418208ac25ac9f42bf4502a9

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

 ​f92d0ef Use the -d argument to sage-apply-patches instead

### comment:56 follow-up: ↓ 58 Changed 5 years ago by embray

As for the arguments I think I'll leave them as is. As of now the patch-dir argument is not used explicitly which is good, as it implies uniformity across the spkg-installs (previously there was less uniformity and I think both use cases were used).

That said, it would be more work now to remove it than to leave it as is, as it is completely harmless. And not having it limits the script's applicability to other problems that may arise in the future. I do not think it is "needless complexity".

### comment:57 Changed 5 years ago by embray

• Status changed from needs_info to needs_review

### comment:58 in reply to: ↑ 56 Changed 5 years ago by embray

• Status changed from needs_review to needs_work

As for the arguments I think I'll leave them as is. As of now the patch-dir argument is not used explicitly which is good, as it implies uniformity across the spkg-installs (previously there was less uniformity and I think both use cases were used).

That said, it would be more work now to remove it than to leave it as is, as it is completely harmless. And not having it limits the script's applicability to other problems that may arise in the future. I do not think it is "needless complexity".

(FWIW if I were to choose one to remove it would be patch-dir since now it's not used.)

Also setting back to needs_work because I still need to update the documentation.

### comment:59 Changed 4 years ago by git

• Commit changed from f92d0eff9c063c6f418208ac25ac9f42bf4502a9 to ed68c76ed23b611db471f60aac37b5222b4f740b

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

 ​928528c Adds a new sage-apply-patches script. ​5f76324 Use the -d argument to sage-apply-patches instead ​ed68c76 Added some updated documentation about how to apply patches

### comment:60 Changed 4 years ago by embray

• Status changed from needs_work to needs_review

Rebased again, and updated the docs. Can we please move forward on merging this before I have to rebase again? If you really want to change something about sage-apply-patches we can do that separately, but this works now as-is.

### comment:61 Changed 4 years ago by jdemeyer

Note to self: double-check fricas, frobby, gap3, gfan, scipoptsuite

Last edited 4 years ago by jdemeyer (previous) (diff)

### comment:62 Changed 4 years ago by jdemeyer

• Dependencies changed from #21103 to #17254
• Status changed from needs_review to needs_work

This conflicts with #17254. I suggest that you merge that branch.

### comment:63 Changed 4 years ago by jdemeyer

Also, since you have to change this branch anyway: could you remove the line echo "Applying Sage-specific patches" from sage-spkg? That is not needed since sage-apply-patches is sufficiently verbose. That message could actually be confusing if there are no patches to apply.

### comment:64 Changed 4 years ago by jdemeyer

Another detail: the pushd/popd is also verbose:

[gsl-2.1] Finished extraction
[gsl-2.1] Applying Sage-specific patches
[gsl-2.1] /usr/local/src/sage-git/local/var/tmp/sage/build/gsl-2.1/src /usr/local/src/sage-git/local/var/tmp/sage/build/gsl-2.1
[gsl-2.1] No patch files found in ../patches
[gsl-2.1] /usr/local/src/sage-git/local/var/tmp/sage/build/gsl-2.1
[gsl-2.1] ****************************************************

This should be silenced

### comment:65 Changed 4 years ago by jdemeyer

• Dependencies changed from #17254 to #17254, #21721

Some packages still use non-standard patching: #21721

### comment:66 Changed 4 years ago by jdemeyer

In build/pkgs/scipoptsuite/spkg-install, you are still using both directory arguments:

sage-apply-patches -d scip $SPKGDIR/patches || exit 1 This could be sage-apply-patches "$SPKGDIR/patches/scip" || exit $? ### comment:67 Changed 4 years ago by jdemeyer • Dependencies changed from #17254, #21721 to #17254, #21721, #21722 gap3 had more issues, so I created a separate ticket: #21722 ### comment:68 Changed 4 years ago by jdemeyer • Dependencies changed from #17254, #21721, #21722 to #21721, #21722 • Description modified (diff) ### comment:69 Changed 4 years ago by jdemeyer • Dependencies #21721, #21722 deleted All dependencies are merged now, so please rebase to latest 7.5.beta1. ### comment:70 Changed 4 years ago by jdemeyer • Dependencies set to #21676, #21792 ### comment:71 follow-up: ↓ 72 Changed 4 years ago by embray All dependencies are not merged now.... ### comment:72 in reply to: ↑ 71 Changed 4 years ago by jdemeyer Replying to embray: All dependencies are not merged now.... No, because I added two dependencies which would otherwise conflict. ### comment:73 Changed 4 years ago by jdemeyer You could just merge those two dependencies in this branch. ### comment:74 Changed 4 years ago by mkoeppe needs rebasing ### comment:75 Changed 4 years ago by jdemeyer • Dependencies #21676, #21792 deleted ### comment:76 Changed 4 years ago by git • Commit changed from ed68c76ed23b611db471f60aac37b5222b4f740b to 0968ec08336fe9d90df740a514c39e91495a9930 Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:  ​f187f9c Adds a new sage-apply-patches script. ​0968ec0 Added some updated documentation about how to apply patches ### comment:77 Changed 4 years ago by embray • Status changed from needs_work to positive_review Rebased again. ### comment:78 Changed 4 years ago by embray • Status changed from positive_review to needs_work ### comment:79 Changed 4 years ago by embray • Status changed from needs_work to needs_review Sorry--misclicked. ### comment:80 Changed 4 years ago by jdemeyer • Status changed from needs_review to needs_work Please fix 63, 64, 66. Those are trivial things which can easily be fixed. ### comment:81 Changed 4 years ago by embray The way sage-apply-patches is being invoked in scipoptsuite/spkg-install is actually exactly how those options are meant to be used but whatever. ### comment:82 Changed 4 years ago by git • Commit changed from 0968ec08336fe9d90df740a514c39e91495a9930 to addd3ee9804571bcca63da0667100a7bce4fffe0 Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:  ​e842689 Adds a new sage-apply-patches script. ​02e8b04 Added some updated documentation about how to apply patches ​addd3ee 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. ### comment:83 Changed 4 years ago by embray • Status changed from needs_work to needs_review New commits:  ​e842689 Adds a new sage-apply-patches script. ​02e8b04 Added some updated documentation about how to apply patches ​addd3ee 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. ### comment:84 Changed 4 years ago by jdemeyer 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). ### comment:85 Changed 4 years ago by embray Awesome, thanks! ### comment:86 Changed 4 years ago by jdemeyer • Milestone set to sage-7.5 • Status changed from needs_review to positive_review ### comment:87 Changed 4 years ago by vbraun • Status changed from positive_review to needs_work Merge conflict ### comment:88 Changed 4 years ago by git • Commit changed from addd3ee9804571bcca63da0667100a7bce4fffe0 to 52a8ca0bafb373c7af7955f9b979fa9438b9d391 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. ### comment:89 Changed 4 years ago by embray • Status changed from needs_work to positive_review Rebased. ### comment:90 Changed 4 years ago by jdemeyer • Branch changed from u/embray/patch-cleanup to u/jdemeyer/patch-cleanup ### comment:91 Changed 4 years ago by jdemeyer • Commit changed from 52a8ca0bafb373c7af7955f9b979fa9438b9d391 to b6b33cdbb33ba7eb856cb283c296250976e1638c 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.
Last edited 4 years ago by jdemeyer (previous) (diff)

### comment:92 Changed 4 years ago by vbraun

• Branch changed from u/jdemeyer/patch-cleanup to b6b33cdbb33ba7eb856cb283c296250976e1638c
• Resolution set to fixed
• Status changed from positive_review to closed

### comment:93 Changed 4 years ago by jdemeyer

• Commit b6b33cdbb33ba7eb856cb283c296250976e1638c deleted

Finally merged, thanks Erik!

### comment:94 Changed 4 years ago by embray

Hooray! *pours you a champagne*

Note: See TracTickets for help on using tickets.