Opened 2 years ago

Closed 2 years ago

Last modified 23 months ago

#24025 closed enhancement (fixed)

Update simple autotools packages to use sdh_configure and related helpers

Reported by: embray Owned by:
Priority: critical Milestone: sage-8.1
Component: build Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 1174282 (Commits) Commit:
Dependencies: #24014, #23781, #21083, #24092, #24042 Stopgaps:

Description

These are packages that use standard autoconf builds (configure && make && make install) that could be easily updated to use the corresponding helper functions, as explained in #24024.

This ticket updates a large number of packages, but the change to all of them is nearly identical.

Change History (53)

comment:1 Changed 2 years ago by embray

I'm going to check to see if there are any other open tickets this conflicts with before setting it ready for review.

comment:2 Changed 2 years ago by embray

  • Dependencies set to #24014 #23781

This is also based on a couple other pending tickets.

comment:3 follow-up: Changed 2 years ago by jdemeyer

Note: we need to check the portability of cp -L on OS X which does not have GNU tools.

comment:4 Changed 2 years ago by jhpalmieri

The man page for cp on OS X says:

-L    If the -R option is specified, all symbolic links are followed.

comment:5 follow-up: Changed 2 years ago by jdemeyer

  • Dependencies changed from #24014 #23781 to #24014, #23781, #21083

Looks good in principle, but I'm sure that there are details to be checked and fixed.

comment:6 in reply to: ↑ 3 Changed 2 years ago by embray

Replying to jdemeyer:

Note: we need to check the portability of cp -L on OS X which does not have GNU tools.

I did check that. Also that comes from #23781. This ticket depends on 3 others so it would be best for those to be merged before reviewing this one. This ticket only changes the spkg- scripts of some packages. Note, you can always manually do a diff as well.

comment:7 in reply to: ↑ 5 Changed 2 years ago by embray

Replying to jdemeyer:

Looks good in principle, but I'm sure that there are details to be checked and fixed.

Are you? I'll note that these changes are all extracted from #22509 which I'm working on breaking up into multiple smaller steps, but the branch in #22509 has already gone through quite a lot of testing.

The only thing I'm sure of is it's worth checking what open tickets this conflicts with, of which I'm sure there are some.

comment:8 Changed 2 years ago by git

  • Commit changed from 6c655a6952a94ec8242383fe7bd11a80ce0cfc14 to d823cb2f0bed9aa4a0e8367e13ff8a374cb2a3b8

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

9613485With fmt, preserve existing line breaks, just wrap long lines
bb9b97bUse sdh_die for sdh_check_vars as well
955ab50Split spkg-install for python2/3 into spkg-build and spkg-install as recommended in https://trac.sagemath.org/ticket/21726
940fdfaMove the post-build tests of module imports to spkg-build instead of
7952b5cFix Python build on non-Cygwin systems (namely OSX) with case-insensitive filesystems
938dbc1On OSX DYLD_LIBRARY_PATH should be set a la LD_LIBRARY_PATH to run Python from its build directory
9ae0a6eFix a bug when building python2/3 and there's already a libpython in
11e699bDereference symlinks while copying package build files.
98a5b94Merge python2 and python3 build scripts as they are mostly identical to
d823cb2Convert several autotools packages to using the helper functions for

comment:9 Changed 2 years ago by embray

  • Status changed from new to needs_review

comment:10 Changed 2 years ago by embray

  • Status changed from needs_review to needs_work

Oops, rebase is messed up...

comment:11 Changed 2 years ago by git

  • Commit changed from d823cb2f0bed9aa4a0e8367e13ff8a374cb2a3b8 to 8b7a87f311cd69febda9da04c0d9211f3b37b2a7

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

8b7a87fConvert several autotools packages to using the helper functions for

comment:12 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

Okay, there we go.

This ticket unfortunately touches a large number of files, but I don't see a much better way to do it other than to create separate tickets for each package, which quickly becomes tedious. The subset of packages updated by this ticket was specifically selected because they required no additional changes other than mechanical replacement of ./configure with sdh_configure and so on (and in the case of brial, one use of sdh_pip_install as well).

Some other packages will be addressed in followup tickets.

comment:13 Changed 2 years ago by embray

  • Status changed from needs_review to needs_work

One question remains whether we need to update the patch level on these packages. I think it's a bit excessive, because the point of focusing on this subset of packages is that the replacement by the helper functions results in almost the exact same build scripts as before. But I'm not outright opposed.

comment:14 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

Didn't mean to change the status.

comment:15 follow-up: Changed 2 years ago by fbissey

  • Status changed from needs_review to needs_work

That will clash with #24092 (which you happily reviewed). I am sorry, can you make it yet another dependency of this ticket?

comment:16 in reply to: ↑ 15 Changed 2 years ago by embray

Replying to fbissey:

That will clash with #24092 (which you happily reviewed). I am sorry, can you make it yet another dependency of this ticket?

Sure, of course. Thanks for pointing it out. I'm trying to track down any other currently active tickets this might clash with.

comment:17 Changed 2 years ago by git

  • Commit changed from 8b7a87f311cd69febda9da04c0d9211f3b37b2a7 to cbb5950251d15d9b08ccc75bbf971cbc3a9fb7bb

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

6dd69fdAdd pip to brial's dependencies, replace the use of PIP_INSTALL as per #24014.
cbb5950Merge branch 'u/fbissey/brial_pip' into u/embray/build/sdh-autotools-trivial

comment:18 Changed 2 years ago by embray

  • Dependencies changed from #24014, #23781, #21083 to #24014, #23781, #21083, #24092

Fully incorporated fix from #24092.

comment:19 Changed 2 years ago by embray

Okay--hard to be exactly sure, but I don't think there are any other imminent tickets this conflicts with, so the sooner it can be applied the better, if indeed it's acceptable.

comment:20 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

comment:21 follow-up: Changed 2 years ago by jdemeyer

I would rather prefer to see all those packages receiving a version bump, to avoid trouble.

Also, some random quick comments:

  1. libfplll: conflicts with #24042: just postpone libfplll to a new ticket.
  1. libpng: I don't want to think about $DESTDIR for now, please revert that.
  1. mpfr: one ./configure command is intentionally not checked I believe.
  1. ppl: just use ./configure instead of sdh_configure if you don't want the functionality of sdh_configure.
  1. zlib: use ZLIB_CONFIGURE instead of CONFIGUREFLAGS to be consistent with other packages.

If you disagree with any of the above, I would much rather prefer to postpone the given package to a new ticket and only deal on this ticket with the packages where we agree. Otherwise we will never make progress.

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

comment:22 in reply to: ↑ 21 Changed 2 years ago by embray

Replying to jdemeyer:

I would rather prefer to see all those packages receiving a version bump, to avoid trouble.

Okay--that will probably introduce more potential conflicts though; e.g. with #23700. I could postpone some of those, or help get those tickets done more quickly if possible...

Also, some random quick comments:

  1. libfplll: conflicts with #24042: just postpone libfplll to a new ticket.

I don't see any reason to postpone it to a new ticket. In particular, since #24042 is already closed I can just merge it into this.

  1. libpng: I don't want to think about $DESTDIR for now, please revert that.

Oops--yes, my intention was to leave out anything to do with DESTDIR. I missed that one.

  1. mpfr: one ./configure command is intentionally not checked I believe.

Right--I think originally I had that correct, but when I was checking this patch over I initially thought it was something I missed and updated it. Now you've reminded me that it was intentionally allowed to fail.

  1. ppl: just use ./configure instead of sdh_configure if you don't want the functionality of sdh_configure.

The functionality of sdh_configure isn't just the error checking. But I'll see if there's a different approach I can take to that case that doesn't require a sub-shell. Maybe I'll leave ppl and mpfr out of this ticket for now.

  1. zlib: use ZLIB_CONFIGURE instead of CONFIGUREFLAGS to be consistent with other packages.

+1

If you disagree with any of the above, I would much rather prefer to postpone the given package to a new ticket and only deal on this ticket with the packages where we agree. Otherwise we will never make progress.

Agreed--with the exception of libfplll where I'm not sure why you propose a separate ticket.

comment:23 Changed 2 years ago by git

  • Commit changed from cbb5950251d15d9b08ccc75bbf971cbc3a9fb7bb to dba2433f349835d616a06429fb17a63e0f8ea4c4

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

2b0b4abUpgrade fplll and fpylll
cc77c4dMerge branch 'u/jdemeyer/upgrade_fplll_and_fpylll' into u/embray/build/sdh-autotools-trivial
9b595aaRevert changes to curl for now so as to not conflict with #23991
f0c5c91Revert changes to gc for now so as to not conflict with #23700
72edfacRevert change to libpng that is unrelated to this ticket
ef3b470Revert change to mpfr that requires a little additional thought
60d7130Revert change to ppl that requires a little additional thought
9dbe51eCONFIGUREFLAGS => ZLIB_CONFIGURE
dba2433Bump patch levels of affected packages

comment:24 Changed 2 years ago by git

  • Commit changed from dba2433f349835d616a06429fb17a63e0f8ea4c4 to df303f9ee3f8357db95736f0f7875124794e1860

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

df303f9Bump cddlib package version

comment:25 Changed 2 years ago by embray

  • Dependencies changed from #24014, #23781, #21083, #24092 to #24014, #23781, #21083, #24092, #24042

Also merged in #24042.

comment:26 Changed 2 years ago by embray

Jeroen, how does this sound to you? I think I addressed all your comments--I excluded ppl and mpfr from this ticket for now, until I decide what to do there, and I have also removed updates to gc and curl in order to avoid conflicts, for now, with other tickets that update them.

comment:27 follow-up: Changed 2 years ago by git

  • Commit changed from df303f9ee3f8357db95736f0f7875124794e1860 to d09460226e0494a8c2f14ca40132dbd4ded77f54

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

d094602Force the libdir option to $SAGE_LOCAL/lib for all packages that call sdh_configure

comment:28 Changed 2 years ago by embray

The problem here was a missing change to the definition of sdh_configure that I originally included in #22509. This is part of the problem with taking a large commit that works fine and trying to chop it down piecemeal...

comment:29 in reply to: ↑ 27 Changed 2 years ago by fbissey

Replying to git:

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

d094602Force the libdir option to $SAGE_LOCAL/lib for all packages that call sdh_configure

I am wondering if this could be split and included quickly. That way, separately from a big push for all packages I and other can already start migrating when upgrading packages.

For example I am thinking on working on #22836 - it will conflict with this ticket. But instead of just upgrading I think I could use the opportunity to migrate. It would be more useful if the above commit was already available.

comment:30 Changed 2 years ago by embray

  • Priority changed from major to critical

Let me rebase it and figure out what the issue is with building flint, which is still broken for some reason. But I agree this should be high priority to merge.

comment:31 Changed 2 years ago by git

  • Commit changed from d09460226e0494a8c2f14ca40132dbd4ded77f54 to 838c42a8c88e5dfab3ec81dbbc317038840f0ed8

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

7305765Revert changes to curl for now so as to not conflict with #23991
ecc846fRevert changes to gc for now so as to not conflict with #23700
d6f16c1Revert change to libpng that is unrelated to this ticket
7664749Revert change to mpfr that requires a little additional thought
186b6daRevert change to ppl that requires a little additional thought
034392dCONFIGUREFLAGS => ZLIB_CONFIGURE
a944e3cBump patch levels of affected packages
475730cBump cddlib package version
fd3a3b1Force the libdir option to $SAGE_LOCAL/lib for all packages that call sdh_configure
838c42aRoll back changes to the flint package, which is not actually a real autoconf package; it has a hand-written configure script for some reason.

comment:32 Changed 2 years ago by embray

It turns out that flint is not actually a real autoconf'd package, and has a hand-written custom configure script which doesn't support --libdir.

I might do something about that, but in the meantime it should be dropped from this ticket.

comment:33 Changed 2 years ago by jdemeyer

  • Branch changed from u/embray/build/sdh-autotools-trivial to u/jdemeyer/build/sdh-autotools-trivial

comment:34 Changed 2 years ago by jdemeyer

  • Commit changed from 838c42a8c88e5dfab3ec81dbbc317038840f0ed8 to 5641b85437769718d2ed2121151acd5ad901f09c
  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

Squashed most commits together and reverted zlib.


New commits:

63324f4Convert several autotools packages to using the helper functions for
5641b85Force the libdir option to $SAGE_LOCAL/lib for all packages that call sdh_configure

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

  • Status changed from positive_review to needs_info

Why "reverted zlib"? I don't see any problems there.

comment:36 Changed 2 years ago by embray

  • Branch changed from u/jdemeyer/build/sdh-autotools-trivial to u/embray/build/sdh-autotools-trivial
  • Commit changed from 5641b85437769718d2ed2121151acd5ad901f09c to 8e441b158d9861e1c87932519a2fb24c28680486

Please don't unilaterally replace my branch with yours without discussion if it's making non-trivial changes.


New commits:

e6603e0Force the libdir option to $SAGE_LOCAL/lib for all packages that call sdh_configure
8e441b1Convert several autotools packages to using the helper functions for

comment:37 Changed 2 years ago by jdemeyer

  • Reviewers Jeroen Demeyer deleted

First of all, I think it is a bad idea to change a ticket back to positive_review just because of that question. As I explained in 21, we have the most chances of actually making progress if we try to merge what we can and postpone more difficult packages.

Giving positive_review to a part of a ticket which makes sense by itself is something that I have done before and I honestly don't see the problem.

My goal is to actually merge this ticket and I don't understand why you are going against that.

comment:38 in reply to: ↑ 35 Changed 2 years ago by jdemeyer

Replying to embray:

Why "reverted zlib"?

There looks to be an issue with quoting (" quotes nested inside " quotes). I thought I commented on this already, but I see now that I haven't. Anyway, I'm done with reviewing this ticket.

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

Giving positive_review to a part of a ticket which makes sense by itself is something that I have done before and I honestly don't see the problem.

Because you made changes and then set it to "positive review" without consulting. If anything you should have then set it to "needs review" so that I could review your changes. Otherwise something that I didn't intend might end up getting merged. It's just presumptuous.

I don't know what issue with zlib you're talking about. I can't reproduce it. Maybe if there really is an issue it would be simpler for me to fix it than to just revert the change entirely.

comment:40 Changed 2 years ago by git

  • Commit changed from 8e441b158d9861e1c87932519a2fb24c28680486 to 2c1152ff7c8494f32581b49a32021bf3f5152f43

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

2c1152fConvert several autotools packages to using the helper functions for

comment:41 in reply to: ↑ 39 ; follow-up: Changed 2 years ago by jdemeyer

Replying to embray:

Because you made changes and then set it to "positive review" without consulting.

I wouldn't call it making changes. Essentially, you propose A, B, C and D and I agree with A, B and C but not with D. So instead of having a discussion about D, I simply say: let's merge A, B and C right now and postpone D. I don't see any problem here.

comment:42 Changed 2 years ago by embray

  • Status changed from needs_info to positive_review

Ah, I see--you're talking about the double quotes inside the assignment of ZLIB_CONFIGURE. You could've just said so. Although it didn't seem to cause any harm it was obviously a mistake; a simple bug. I've removed the --includedir flag entirely from that line since it was superfluous anyways.


New commits:

2c1152fConvert several autotools packages to using the helper functions for
Last edited 2 years ago by embray (previous) (diff)

comment:43 in reply to: ↑ 41 Changed 2 years ago by embray

Replying to jdemeyer:

Replying to embray:

Because you made changes and then set it to "positive review" without consulting.

I wouldn't call it making changes. Essentially, you propose A, B, C and D and I agree with A, B and C but not with D. So instead of having a discussion about D, I simply say: let's merge A, B and C right now and postpone D. I don't see any problem here.

There'd be less problem if some reasonable explanation were given.

comment:44 Changed 2 years ago by embray

It's also sneaky since the change was rolled into a squashed commit. In this case it could be very easy for me to miss, since I already have other branches where I've done further work on this.

comment:45 Changed 2 years ago by chapoton

reviewer name, please

comment:46 Changed 2 years ago by fbissey

  • Reviewers set to Jeroen Demeyer

comment:47 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

merge conflict

comment:48 Changed 2 years ago by jdemeyer

  • Branch changed from u/embray/build/sdh-autotools-trivial to u/jdemeyer/build/sdh-autotools-trivial

comment:49 Changed 2 years ago by jdemeyer

  • Commit changed from 2c1152ff7c8494f32581b49a32021bf3f5152f43 to 1174282f66a55285c54fa6eeef4d812f2ef80a32
  • Status changed from needs_work to positive_review

New commits:

63324f4Convert several autotools packages to using the helper functions for
5641b85Force the libdir option to $SAGE_LOCAL/lib for all packages that call sdh_configure
1174282Merge tag '8.1.rc4' into t/24025/build/sdh-autotools-trivial

comment:50 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/build/sdh-autotools-trivial to 1174282f66a55285c54fa6eeef4d812f2ef80a32
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:51 Changed 2 years ago by jdemeyer

  • Commit 1174282f66a55285c54fa6eeef4d812f2ef80a32 deleted

Great!

comment:52 Changed 23 months ago by dimpase

I am confused - where are all these sdh_make, sdh_confugure etc scripts located ?

comment:53 Changed 23 months ago by dimpase

facepalm - sdh==sage-dist-helpers, see #23160

Note: See TracTickets for help on using tickets.