Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#24025 closed enhancement (fixed)

Update simple autotools packages to use sdh_configure and related helpers

Reported by: Erik Bray 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, GitHub, GitLab) Commit:
Dependencies: #24014, #23781, #21083, #24092, #24042 Stopgaps:

Status badges

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 5 years ago by Erik Bray

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 5 years ago by Erik Bray

Dependencies: #24014 #23781

This is also based on a couple other pending tickets.

comment:3 Changed 5 years ago by Jeroen Demeyer

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

comment:4 Changed 5 years ago by John Palmieri

The man page for cp on OS X says:

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

comment:5 Changed 5 years ago by Jeroen Demeyer

Dependencies: #24014 #23781#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 5 years ago by Erik Bray

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 5 years ago by Erik Bray

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 5 years ago by git

Commit: 6c655a6952a94ec8242383fe7bd11a80ce0cfc14d823cb2f0bed9aa4a0e8367e13ff8a374cb2a3b8

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 5 years ago by Erik Bray

Status: newneeds_review

comment:10 Changed 5 years ago by Erik Bray

Status: needs_reviewneeds_work

Oops, rebase is messed up...

comment:11 Changed 5 years ago by git

Commit: d823cb2f0bed9aa4a0e8367e13ff8a374cb2a3b88b7a87f311cd69febda9da04c0d9211f3b37b2a7

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 5 years ago by Erik Bray

Status: needs_workneeds_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 5 years ago by Erik Bray

Status: needs_reviewneeds_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 5 years ago by Erik Bray

Status: needs_workneeds_review

Didn't mean to change the status.

comment:15 Changed 5 years ago by François Bissey

Status: needs_reviewneeds_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 5 years ago by Erik Bray

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 5 years ago by git

Commit: 8b7a87f311cd69febda9da04c0d9211f3b37b2a7cbb5950251d15d9b08ccc75bbf971cbc3a9fb7bb

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 5 years ago by Erik Bray

Dependencies: #24014, #23781, #21083#24014, #23781, #21083, #24092

Fully incorporated fix from #24092.

comment:19 Changed 5 years ago by Erik Bray

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 5 years ago by Erik Bray

Status: needs_workneeds_review

comment:21 Changed 5 years ago by Jeroen Demeyer

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 5 years ago by Jeroen Demeyer (previous) (diff)

comment:22 in reply to:  21 Changed 5 years ago by Erik Bray

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 5 years ago by git

Commit: cbb5950251d15d9b08ccc75bbf971cbc3a9fb7bbdba2433f349835d616a06429fb17a63e0f8ea4c4

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 5 years ago by git

Commit: dba2433f349835d616a06429fb17a63e0f8ea4c4df303f9ee3f8357db95736f0f7875124794e1860

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

df303f9Bump cddlib package version

comment:25 Changed 5 years ago by Erik Bray

Dependencies: #24014, #23781, #21083, #24092#24014, #23781, #21083, #24092, #24042

Also merged in #24042.

comment:26 Changed 5 years ago by Erik Bray

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 Changed 5 years ago by git

Commit: df303f9ee3f8357db95736f0f7875124794e1860d09460226e0494a8c2f14ca40132dbd4ded77f54

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 5 years ago by Erik Bray

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 5 years ago by François Bissey

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 5 years ago by Erik Bray

Priority: majorcritical

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 5 years ago by git

Commit: d09460226e0494a8c2f14ca40132dbd4ded77f54838c42a8c88e5dfab3ec81dbbc317038840f0ed8

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 5 years ago by Erik Bray

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 5 years ago by Jeroen Demeyer

Branch: u/embray/build/sdh-autotools-trivialu/jdemeyer/build/sdh-autotools-trivial

comment:34 Changed 5 years ago by Jeroen Demeyer

Commit: 838c42a8c88e5dfab3ec81dbbc317038840f0ed85641b85437769718d2ed2121151acd5ad901f09c
Reviewers: Jeroen Demeyer
Status: needs_reviewpositive_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 Changed 5 years ago by Erik Bray

Status: positive_reviewneeds_info

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

comment:36 Changed 5 years ago by Erik Bray

Branch: u/jdemeyer/build/sdh-autotools-trivialu/embray/build/sdh-autotools-trivial
Commit: 5641b85437769718d2ed2121151acd5ad901f09c8e441b158d9861e1c87932519a2fb24c28680486

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 5 years ago by Jeroen Demeyer

Reviewers: Jeroen Demeyer

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 5 years ago by Jeroen Demeyer

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 Changed 5 years ago by Erik Bray

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 5 years ago by git

Commit: 8e441b158d9861e1c87932519a2fb24c286804862c1152ff7c8494f32581b49a32021bf3f5152f43

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 ; Changed 5 years ago by Jeroen Demeyer

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 5 years ago by Erik Bray

Status: needs_infopositive_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 5 years ago by Erik Bray (previous) (diff)

comment:43 in reply to:  41 Changed 5 years ago by Erik Bray

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 5 years ago by Erik Bray

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 5 years ago by Frédéric Chapoton

reviewer name, please

comment:46 Changed 5 years ago by François Bissey

Reviewers: Jeroen Demeyer

comment:47 Changed 5 years ago by Volker Braun

Status: positive_reviewneeds_work

merge conflict

comment:48 Changed 5 years ago by Jeroen Demeyer

Branch: u/embray/build/sdh-autotools-trivialu/jdemeyer/build/sdh-autotools-trivial

comment:49 Changed 5 years ago by Jeroen Demeyer

Commit: 2c1152ff7c8494f32581b49a32021bf3f5152f431174282f66a55285c54fa6eeef4d812f2ef80a32
Status: needs_workpositive_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 5 years ago by Volker Braun

Branch: u/jdemeyer/build/sdh-autotools-trivial1174282f66a55285c54fa6eeef4d812f2ef80a32
Resolution: fixed
Status: positive_reviewclosed

comment:51 Changed 5 years ago by Jeroen Demeyer

Commit: 1174282f66a55285c54fa6eeef4d812f2ef80a32

Great!

comment:52 Changed 5 years ago by Dima Pasechnik

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

comment:53 Changed 5 years ago by Dima Pasechnik

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

Note: See TracTickets for help on using tickets.