#29631 closed enhancement (fixed)
spkg-configure.m4 for linbox
Reported by: | gh-thierry-FreeBSD | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.6 |
Component: | build: configure | Keywords: | linbox; spkg-configure; system packages |
Cc: | mkoeppe, dimpase, fbissey, isuruf, embray, arojas, slelievre | Merged in: | |
Authors: | Thierry Thomas, Michael Orlitzky, Samuel Lelièvre | Reviewers: | Dima Pasechnik |
Report Upstream: | N/A | Work issues: | |
Branch: | bc7a6f8 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
Attachments (1)
Change History (68)
Changed 2 years ago by
comment:1 Changed 2 years ago by
- Description modified (diff)
comment:2 Changed 2 years ago by
- Cc mkoeppe dimpase fbissey isuruf embray arojas added
We also need iml, ntl, flint, fplll, and mpfr in the DEPCHECK. Linbox can be built without some of those libraries with, for example, --without-ntl
, so we may need to check for the parts that are actually used in sage. The sage-on-gentoo ebuild passes --without-fplll
, so that dependency is probably superfluous, and maybe others are as well? In any case, to check which libraries linbox actually supports... after the PKG_CHECK_MODULES
call, I guess we could look for things like -lntl
in LINBOX_LIBS
?
I don't see any distros that have a 1.6.x version other than 1.6.3, so it would probably be pointless to lower the version to 1.6.0. The real question is whether or not we can use the v1.5.2 that e.g. older Debian/Fedora? ship.
comment:3 Changed 2 years ago by
pkgconf linbox --libs
sets LINBOX_LIB, and we could grep that the required libs are there?
comment:4 Changed 2 years ago by
Hmm, looking at the spkg-install for linbox, we already pass --without-fplll
:
# Disable fplll as version 5.x is not supported by linbox <= 1.5.0. # This is harmless as no functionality using fplll is exposed in Sage. # See trac ticket #21221. LINBOX_CONFIGURE="--without-fplll $LINBOX_CONFIGURE"
So, the existing dependency on fplll (in build/pkgs/linbox/dependencies
) is wrong. I'm going to build a system copy of linbox with all of the other stuff disabled and see what goes wrong. Maybe we don't care if the other stuff is enabled, either.
comment:5 Changed 2 years ago by
For posterity, this also appears to be obsolete,
# Need to use 'bash' for configure, see # https://trac.sagemath.org/ticket/23451 if [ -z "$CONFIG_SHELL" ]; then export CONFIG_SHELL=`command -v bash` fi
as I've been compiling linbox-1.6.3 this whole time with /bin/sh -> /bin/dash
.
comment:6 Changed 2 years ago by
Even if you pass --without-foo
for all of the optional linbox dependencies, flint still gets pulled in automagically, so it's going to be required either way. I'm doing a full sage build / ptestlong now with the rest (ntl, fplll, mpfr, iml) disabled to see if any of them are actually important.
comment:7 Changed 2 years ago by
- Branch set to u/mjo/ticket/29631
- Commit set to e4a14c0e9de40be16c5515aaf0a2ab4e877a5204
Everything is fine if we disable ntl, iml, and mpfr in linbox, so I created a branch with those dependencies eliminated. That means that we only need to add "flint" to the DEPCHECK.
New commits:
72291a0 | Trac #29631: don't force SHELL=bash when installing linbox.
|
787b5df | Trac #29631: drop fplll from linbox's dependencies.
|
e4a14c0 | Trac #29631: disable additional unused features of the linbox SPKG.
|
comment:8 Changed 2 years ago by
This page has some information on how to checkout and push new branches to our trac server:
You should be able to checkout my branch, add a commit on top of it, and then push a new branch of your own (which you would then stick in the "branch" field of this ticket). That's the best way to collaborate since it will keep your commit info and author information in-tact. If you don't feel like dealing with it, though, I can just add the spkg-configure.m4 onto my branch and re-push.
After adding flint to the DEPCHECK, I think only two questions remains:
- Do we need to check for flint support in the spkg-configure check? Given that flint support is "automagically" enabled, I don't think it's necessary on source-based distros, who should have flint support enabled unconditionally to avoid problems. If none of the binary distros ship a flint-less linbox, then we can skip it.
- Should we try to set the version bound to 1.5.2? Someone would need to test that.
comment:9 follow-up: ↓ 14 Changed 22 months ago by
- Cc slelievre added
These questions can be resolved by adding system package information to build/pkgs/linbox/distros
and then testing...
comment:10 Changed 22 months ago by
comment:11 Changed 21 months ago by
The duplicate ticket #29484 reminds us to "(attempt to) lower the bound on fflas-ffpack in its spkg-configure.m4. If we can accept an older system linbox that itself uses an older system fflas-ffpack, that would be nice."
comment:12 Changed 19 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:13 Changed 15 months ago by
- Milestone changed from sage-9.3 to sage-9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:14 in reply to: ↑ 9 Changed 14 months ago by
- Status changed from new to needs_review
Replying to mkoeppe:
These questions can be resolved by adding system package information to
build/pkgs/linbox/distros
and then testing...
Here we go.
comment:15 Changed 14 months ago by
- Branch changed from u/mjo/ticket/29631 to u/slelievre/29361
- Commit changed from e4a14c0e9de40be16c5515aaf0a2ab4e877a5204 to 6864ae576dc56e4373a6b509ce21b7fd40c26dc5
comment:16 Changed 14 months ago by
- Status changed from needs_review to needs_work
The branch does not do what ticket title and description promise
comment:17 Changed 10 months ago by
- Milestone changed from sage-9.4 to sage-9.5
Setting a new milestone for this ticket based on a cursory review.
comment:18 Changed 6 months ago by
- Branch changed from u/slelievre/29361 to u/mjo/ticket/29631
- Commit changed from 6864ae576dc56e4373a6b509ce21b7fd40c26dc5 to 756f9eebb650a7ff5b206db770c4121868ae559b
- Status changed from needs_work to needs_review
let's unforget about this
New commits:
19b93eb | Trac #29631: don't force SHELL=bash when installing linbox.
|
6568cfb | Trac #29631: drop fplll from linbox's dependencies.
|
e5c12ca | Trac #29631: disable additional unused features of the linbox SPKG.
|
83c89a4 | 29361: Add Fedora and Gentoo linbox package info
|
756f9ee | Trac #29631: new spkg-configure.m4 for linbox.
|
comment:19 follow-up: ↓ 22 Changed 6 months ago by
+# Disable all additional features of linbox that aren't needed by sage. +# This helps keep the dependency list small, and makes it more likely +# that we can use the system copy of linbox (which may be missing some +# of these features).
Note that the purpose of the SPKG_DEPCHECK
logic is as follows: The system package MIGHT link to an optional dependency like fplll
. Therefore, if we build our own fplll
, we need to avoid using system linbox
.
It does not help at all to remove features from our build of the linbox
spkg.
comment:20 Changed 6 months ago by
- Status changed from needs_review to needs_work
comment:21 Changed 6 months ago by
- Commit changed from 756f9eebb650a7ff5b206db770c4121868ae559b to c5d523b3641361739743b2ecb34d7e041bd2eafd
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c5d523b | Trac #29631: new spkg-configure.m4 for linbox.
|
comment:22 in reply to: ↑ 19 ; follow-up: ↓ 24 Changed 6 months ago by
Replying to mkoeppe:
+# Disable all additional features of linbox that aren't needed by sage. +# This helps keep the dependency list small, and makes it more likely +# that we can use the system copy of linbox (which may be missing some +# of these features).Note that the purpose of the
SPKG_DEPCHECK
logic is as follows: The system package MIGHT link to an optional dependency likefplll
. Therefore, if we build our ownfplll
, we need to avoid using systemlinbox
.
Ideally we would detect that optional linkage using AC_SEARCH_LIBS
, so that we only reject a system linbox if it actually links to something on the system that is also installed (and linked into sage) as an SPKG. But DEPCHECK
is much easier than digging through the source looking for public functions hidden behind e.g. HAVE_IML
, so I've just gone ahead and used DEPCHECK
for now.
It does not help at all to remove features from our build of the
linbox
spkg.
Not having those features in the SPKG means that we don't have to insist that they be present in the system copy.
comment:23 Changed 6 months ago by
- Status changed from needs_work to needs_review
comment:24 in reply to: ↑ 22 ; follow-up: ↓ 25 Changed 6 months ago by
Replying to mjo:
It does not help at all to remove features from our build of the
linbox
spkg.Not having those features in the SPKG means that we don't have to insist that they be present in the system copy.
We don't have to insist on it either way. No need to remove the feature.
comment:25 in reply to: ↑ 24 ; follow-up: ↓ 26 Changed 6 months ago by
Replying to mkoeppe:
Not having those features in the SPKG means that we don't have to insist that they be present in the system copy.
We don't have to insist on it either way. No need to remove the feature.
You want the linbox SPKG to have mpfr, ntl, and iml support... but also accept a system linbox that doesn't? And will crash the moment anyone tries to use those optional features?
comment:26 in reply to: ↑ 25 ; follow-up: ↓ 27 Changed 6 months ago by
Replying to mjo:
You want the linbox SPKG to have mpfr, ntl, and iml support... but also accept a system linbox that doesn't? And will crash the moment anyone tries to use those optional features?
The same is also true when a user uses a system linbox that provides the optional features.
comment:27 in reply to: ↑ 26 Changed 6 months ago by
Replying to mkoeppe:
Replying to mjo:
You want the linbox SPKG to have mpfr, ntl, and iml support... but also accept a system linbox that doesn't? And will crash the moment anyone tries to use those optional features?
The same is also true when a user uses a system linbox that provides the optional features.
A user can write his own code that depends on his own special system copy of linbox, but he won't get it committed to sage, which is the risk we run retaining the optional features in the SPKG. And the general trend is from-SPKG-to-system-package -- not the other way around -- minimizing the risk that someone will shoot himself in the foot that way. Finally, we're not doing "two wrong make a right." Avoiding the one potential problem is a win. Sage doesn't need those features enabled. Why slow down the build, bloat the install, and invite future bug reports?
comment:28 Changed 6 months ago by
- Status changed from needs_review to needs_work
Removing the features is neither an improvement, nor a prerequisite for the goal of the ticket. So it's not appropriate for this ticket.
comment:29 Changed 6 months ago by
- Commit changed from c5d523b3641361739743b2ecb34d7e041bd2eafd to 7072119d27d091b3894860547673c2f6b224f61a
comment:31 follow-up: ↓ 32 Changed 6 months ago by
Thanks. The branch still has this change though:
--- a/build/pkgs/linbox/dependencies +++ b/build/pkgs/linbox/dependencies @@ -1,4 +1,4 @@ -$(MP_LIBRARY) ntl givaro mpfr fplll iml flint fflas_ffpack +$(MP_LIBRARY) ntl givaro mpfr iml flint fflas_ffpack
comment:32 in reply to: ↑ 31 Changed 6 months ago by
Replying to mkoeppe:
Thanks. The branch still has this change though:
--- a/build/pkgs/linbox/dependencies +++ b/build/pkgs/linbox/dependencies @@ -1,4 +1,4 @@ -$(MP_LIBRARY) ntl givaro mpfr fplll iml flint fflas_ffpack +$(MP_LIBRARY) ntl givaro mpfr iml flint fflas_ffpack
Its spkg-install explicitly disables fplll.
comment:33 Changed 6 months ago by
Ah, thanks.
comment:34 Changed 6 months ago by
Looking good then, but this will need platform testing on GH Actions
comment:35 Changed 6 months ago by
After fixing the system linbox package (as in #27205) this works for me on void linux.
comment:36 Changed 6 months ago by
- Branch changed from u/mjo/ticket/29631 to u/dimpase/rebased/mjo/ticket/29631
- Commit changed from 7072119d27d091b3894860547673c2f6b224f61a to 794713e88ee188d516e4956832a23f985a73917a
- Reviewers set to Dima Pasechnik
comment:37 Changed 6 months ago by
- Commit changed from 794713e88ee188d516e4956832a23f985a73917a to f1fdff2025189aa99a5824288fa1695ad80f7bf6
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
2240cc1 | build/pkgs/pyzmq: Update to 22.3.0
|
fc11612 | build/pkgs/zeromq: Update to 4.3.4, add upstream_url
|
a8fe9da | build/pkgs/zeromq/patches/getrandom.patch: Remove
|
078e37b | build/pkgs/setuptools: Update to 59.1.0, remove patch
|
ee4f6b4 | Merge tag '9.5.beta7' into t/32828/pyzmq__build_errors
|
b430bcf | build/pkgs/setuptools: Update to 59.2.0
|
c986098 | Merge branch 'u/mkoeppe/pyzmq__build_errors' of trac.sagemath.org:sage into u/mjo/ticket/29631
|
53cfadf | update cysignals to 1.11.0
|
5ed4717 | update cysignals to 1.11.1
|
f1fdff2 | Merge branch 'public/32576' of trac.sagemath.org:sage into u/mjo/ticket/29631
|
comment:38 Changed 6 months ago by
- Commit changed from f1fdff2025189aa99a5824288fa1695ad80f7bf6 to 00a28d5765174f41c444f4945e87933a855d3d01
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
3795599 | Trac #8450: update the output from a few plotting doctests.
|
518349e | Trac #8450: pass imaginary tolerance down through plot.options.
|
5b46a44 | Trac #8450: remove imag_tol default from FastCallablePlotWrapper().
|
b47adf4 | Trac #8450: use CDF for symbolic expression fast-callables.
|
8b43bb8 | Trac #8450: catch more cases in setup_for_eval_on_grid.
|
77ed41c | Trac #8450: fix complex type error in real_nth_root().
|
5a70ea6 | Trac #8450: add an example using "imaginary_tolerance" in plot().
|
7899e73 | Merge remote-tracking branch 'trac/develop' into u/mjo/ticket/8450
|
2c086e2 | add missing imports
|
00a28d5 | Merge branch 'u/dimpase/rebased/mjo/ticket/8450' of trac.sagemath.org:sage into u/mjo/ticket/29631
|
comment:39 Changed 6 months ago by
- Reviewers changed from Dima Pasechnik to Dima Pasechnik, https://github.com/sagemath/sagetrac-mirror/actions/runs/1538684506
comment:40 follow-up: ↓ 47 Changed 6 months ago by
currently the branch includes some positively reviewed tickets - most of them to increase chances of more systems passing.
comment:41 Changed 6 months ago by
- Commit changed from 00a28d5765174f41c444f4945e87933a855d3d01 to 7e16ea979d4bfa1a26ef3b08a29ca626b83b6ff5
Branch pushed to git repo; I updated commit sha1. New commits:
7e16ea9 | correct the package name: `-`->`_`
|
comment:42 Changed 6 months ago by
the last commit should not be forgotten while cutting the final branch here...
comment:43 Changed 6 months ago by
also need
-
build/pkgs/m4ri/distros/debian.txt
a b 1 libm4ri-dev
comment:44 Changed 6 months ago by
and
-
new file uild/pkgs/fplll/distros/debian.txt
- + 1 libfplll-dev
comment:45 Changed 5 months ago by
- Branch changed from u/dimpase/rebased/mjo/ticket/29631 to u/slelievre/29631
- Commit changed from 7e16ea979d4bfa1a26ef3b08a29ca626b83b6ff5 to 60e69eddc1760ec54c279b2d84c3b7e33843a4b7
New commits:
60e69ed | 29631: add fplll and libm4ri debian distro info
|
comment:46 Changed 5 months ago by
Added the changes proposed in @dimpase's last two comments: comment:43, comment:44.
comment:47 in reply to: ↑ 40 Changed 5 months ago by
Replying to dimpase:
currently the branch includes some positively reviewed tickets - most of them to increase chances of more systems passing.
These should be listed in the Dependencies field
comment:48 Changed 5 months ago by
In light of #33042, let's also set an upper bound for the linbox version
comment:49 Changed 5 months ago by
- Dependencies set to #8450, #32576, #32828
- Status changed from needs_review to needs_work
comment:50 Changed 5 months ago by
- Commit changed from 60e69eddc1760ec54c279b2d84c3b7e33843a4b7 to 56aba2fb60135e02d916bf1d69125bcfc28f52dc
Branch pushed to git repo; I updated commit sha1. New commits:
56aba2f | 29631: upper bound for linbox version
|
comment:51 follow-up: ↓ 52 Changed 5 months ago by
- Status changed from needs_work to needs_review
Like that?
comment:52 in reply to: ↑ 51 Changed 5 months ago by
Replying to slelievre:
Like that?
LGTM. The linbox team doesn't really do maintenance releases, so even if we set the bound to < 1.7.0
, it would have the same effect. sage_spkg_install_linbox=no
is the default so it's not really necessary but it won't hurt anything.
comment:53 follow-up: ↓ 55 Changed 5 months ago by
I mimicked build/pkgsfplll/spkg-configure.m4
.
Is anything else needed here?
comment:54 Changed 5 months ago by
- Milestone changed from sage-9.5 to sage-9.6
comment:55 in reply to: ↑ 53 Changed 5 months ago by
comment:56 Changed 4 months ago by
I don't think #8450 is going to make it into v9.5, maybe time to rebase this with just the linbox commits?
comment:57 follow-up: ↓ 58 Changed 4 months ago by
#8450 made it into Sage 9.6.beta0.
comment:58 in reply to: ↑ 57 Changed 4 months ago by
comment:59 Changed 4 months ago by
- Branch changed from u/slelievre/29631 to u/mjo/ticket/29631
- Commit changed from 56aba2fb60135e02d916bf1d69125bcfc28f52dc to fc083c2420e13c0dfebe0b04c1966ab615e3ea21
- Dependencies #8450, #32576, #32828 deleted
New commits:
ab58ae4 | Trac #29631: don't force SHELL=bash when installing linbox.
|
94d7717 | Trac #29631: drop fplll from linbox's dependencies.
|
7afd361 | 29361: Add Fedora and Gentoo linbox package info
|
8128f66 | Trac #29631: new spkg-configure.m4 for linbox.
|
48eab64 | add missing imports
|
91aee3b | correct the package name: `-`->`_`
|
6ee6942 | 29631: add fplll and libm4ri debian distro info
|
fc083c2 | 29631: upper bound for linbox version
|
comment:60 Changed 3 months ago by
- Commit changed from fc083c2420e13c0dfebe0b04c1966ab615e3ea21 to bc7a6f8d2b2b79094998dbc63d33adb01fc021cf
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9a8bf64 | Trac #29631: don't force SHELL=bash when installing linbox.
|
9f788f5 | Trac #29631: drop fplll from linbox's dependencies.
|
796b0c3 | 29361: Add Fedora and Gentoo linbox package info
|
9b7d1ec | Trac #29631: new spkg-configure.m4 for linbox.
|
a5d8c72 | correct the package name: `-`->`_`
|
4d41356 | 29631: add fplll and libm4ri debian distro info
|
bc7a6f8 | 29631: upper bound for linbox version
|
comment:61 Changed 2 months ago by
Ping, this one should be easy now:
- Very few lines changed
- We require the one exact version of linbox that sage itself ships
- It already went through Github CI
comment:62 Changed 2 months ago by
- Reviewers changed from Dima Pasechnik, https://github.com/sagemath/sagetrac-mirror/actions/runs/1538684506 to Dima Pasechnik
- Status changed from needs_review to positive_review
lgtm
comment:63 Changed 2 months ago by
Thanks! Now I guess I have to test the maxima ticket on github.
comment:64 Changed 2 months ago by
- Status changed from positive_review to needs_work
This ticket makes no sense because we have no spkg-configure.m4 for the dependency fflas_ffpack
comment:65 Changed 2 months ago by
- Status changed from needs_work to positive_review
Sorry for the noise - I was wrong about that (misled by a typo that I made)
comment:66 Changed 2 months ago by
- Branch changed from u/mjo/ticket/29631 to bc7a6f8d2b2b79094998dbc63d33adb01fc021cf
- Resolution set to fixed
- Status changed from positive_review to closed
comment:67 Changed 2 months ago by
- Commit bc7a6f8d2b2b79094998dbc63d33adb01fc021cf deleted
I've missed that build/pkgs/linbox/distros/fedora.txt
also needs linbox-devel
To be put under build/pkgs/linbox