Opened 2 years ago

Closed 2 months ago

Last modified 2 months ago

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

Status badges

Description (last modified by mkoeppe)

This ticket adds an spkg-configure.m4 for linbox, in order to use it from a system package if possible (see #27330).

Note for packagers: by default, linbox produces a buggy linbox.pc, and @LINBOXSAGE_LIBS@ must be removed from their linbox.pc.in (see #27205).

No more problem after that.

Attachments (1)

spkg-configure.m4 (223 bytes) - added by gh-thierry-FreeBSD 2 years ago.
To be put under build/pkgs/linbox

Download all attachments as: .zip

Change History (68)

Changed 2 years ago by gh-thierry-FreeBSD

To be put under build/pkgs/linbox

comment:1 Changed 2 years ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 2 years ago by mjo

  • 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 gh-thierry-FreeBSD

pkgconf linbox --libs

sets LINBOX_LIB, and we could grep that the required libs are there?

comment:4 Changed 2 years ago by mjo

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 mjo

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 mjo

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 mjo

  • Authors changed from gh-thierry-FreeBSD to gh-thierry-FreeBSD, Michael Orlitzky
  • 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:

72291a0Trac #29631: don't force SHELL=bash when installing linbox.
787b5dfTrac #29631: drop fplll from linbox's dependencies.
e4a14c0Trac #29631: disable additional unused features of the linbox SPKG.

comment:8 Changed 2 years ago by mjo

This page has some information on how to checkout and push new branches to our trac server:

http://doc.sagemath.org/html/en/developer/manual_git.html

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: Changed 22 months ago by mkoeppe

  • 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 mkoeppe

  • Authors changed from gh-thierry-FreeBSD, Michael Orlitzky to Thierry Thomas, Michael Orlitzky

comment:11 Changed 21 months ago by mkoeppe

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 mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:13 Changed 15 months ago by mkoeppe

  • 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 slelievre

  • Authors changed from Thierry Thomas, Michael Orlitzky to Thierry Thomas, Michael Orlitzky, Samuel Lelièvre
  • 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 slelievre

  • Branch changed from u/mjo/ticket/29631 to u/slelievre/29361
  • Commit changed from e4a14c0e9de40be16c5515aaf0a2ab4e877a5204 to 6864ae576dc56e4373a6b509ce21b7fd40c26dc5

New commits:

0e7917eTrac #29631: don't force SHELL=bash when installing linbox.
c238f84Trac #29631: drop fplll from linbox's dependencies.
bb670d8Trac #29631: disable additional unused features of the linbox SPKG.
6864ae529361: Add Fedora and Gentoo linbox package info

comment:16 Changed 14 months ago by mkoeppe

  • 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 mkoeppe

  • 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 mjo

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

19b93ebTrac #29631: don't force SHELL=bash when installing linbox.
6568cfbTrac #29631: drop fplll from linbox's dependencies.
e5c12caTrac #29631: disable additional unused features of the linbox SPKG.
83c89a429361: Add Fedora and Gentoo linbox package info
756f9eeTrac #29631: new spkg-configure.m4 for linbox.

comment:19 follow-up: Changed 6 months ago by 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 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 mkoeppe

  • Status changed from needs_review to needs_work

comment:21 Changed 6 months ago by git

  • Commit changed from 756f9eebb650a7ff5b206db770c4121868ae559b to c5d523b3641361739743b2ecb34d7e041bd2eafd

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

c5d523bTrac #29631: new spkg-configure.m4 for linbox.

comment:22 in reply to: ↑ 19 ; follow-up: Changed 6 months ago by mjo

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 like fplll. Therefore, if we build our own fplll, we need to avoid using system linbox.

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 mjo

  • Status changed from needs_work to needs_review

comment:24 in reply to: ↑ 22 ; follow-up: Changed 6 months ago by mkoeppe

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: Changed 6 months ago by mjo

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: Changed 6 months ago by 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.

comment:27 in reply to: ↑ 26 Changed 6 months ago by mjo

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 mkoeppe

  • 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 git

  • Commit changed from c5d523b3641361739743b2ecb34d7e041bd2eafd to 7072119d27d091b3894860547673c2f6b224f61a

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

85b19db29361: Add Fedora and Gentoo linbox package info
7072119Trac #29631: new spkg-configure.m4 for linbox.

comment:30 Changed 6 months ago by mjo

  • Status changed from needs_work to needs_review

As you wish.

comment:31 follow-up: Changed 6 months ago by 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

comment:32 in reply to: ↑ 31 Changed 6 months ago by mjo

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 mkoeppe

Ah, thanks.

comment:34 Changed 6 months ago by mkoeppe

Looking good then, but this will need platform testing on GH Actions

comment:35 Changed 6 months ago by tornaria

After fixing the system linbox package (as in #27205) this works for me on void linux.

comment:36 Changed 6 months ago by dimpase

  • 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

rebased over the latest beta


New commits:

4758635Trac #29631: don't force SHELL=bash when installing linbox.
3becab5Trac #29631: drop fplll from linbox's dependencies.
b51dba429361: Add Fedora and Gentoo linbox package info
794713eTrac #29631: new spkg-configure.m4 for linbox.

comment:37 Changed 6 months ago by git

  • Commit changed from 794713e88ee188d516e4956832a23f985a73917a to f1fdff2025189aa99a5824288fa1695ad80f7bf6

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

2240cc1build/pkgs/pyzmq: Update to 22.3.0
fc11612build/pkgs/zeromq: Update to 4.3.4, add upstream_url
a8fe9dabuild/pkgs/zeromq/patches/getrandom.patch: Remove
078e37bbuild/pkgs/setuptools: Update to 59.1.0, remove patch
ee4f6b4Merge tag '9.5.beta7' into t/32828/pyzmq__build_errors
b430bcfbuild/pkgs/setuptools: Update to 59.2.0
c986098Merge branch 'u/mkoeppe/pyzmq__build_errors' of trac.sagemath.org:sage into u/mjo/ticket/29631
53cfadfupdate cysignals to 1.11.0
5ed4717update cysignals to 1.11.1
f1fdff2Merge branch 'public/32576' of trac.sagemath.org:sage into u/mjo/ticket/29631

comment:38 Changed 6 months ago by git

  • Commit changed from f1fdff2025189aa99a5824288fa1695ad80f7bf6 to 00a28d5765174f41c444f4945e87933a855d3d01

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

3795599Trac #8450: update the output from a few plotting doctests.
518349eTrac #8450: pass imaginary tolerance down through plot.options.
5b46a44Trac #8450: remove imag_tol default from FastCallablePlotWrapper().
b47adf4Trac #8450: use CDF for symbolic expression fast-callables.
8b43bb8Trac #8450: catch more cases in setup_for_eval_on_grid.
77ed41cTrac #8450: fix complex type error in real_nth_root().
5a70ea6Trac #8450: add an example using "imaginary_tolerance" in plot().
7899e73Merge remote-tracking branch 'trac/develop' into u/mjo/ticket/8450
2c086e2add missing imports
00a28d5Merge branch 'u/dimpase/rebased/mjo/ticket/8450' of trac.sagemath.org:sage into u/mjo/ticket/29631

comment:39 Changed 6 months ago by dimpase

  • Reviewers changed from Dima Pasechnik to Dima Pasechnik, https://github.com/sagemath/sagetrac-mirror/actions/runs/1538684506

comment:40 follow-up: Changed 6 months ago by dimpase

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 git

  • Commit changed from 00a28d5765174f41c444f4945e87933a855d3d01 to 7e16ea979d4bfa1a26ef3b08a29ca626b83b6ff5

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

7e16ea9correct the package name: `-`->`_`

comment:42 Changed 6 months ago by dimpase

the last commit should not be forgotten while cutting the final branch here...

comment:43 Changed 6 months ago by dimpase

also need

comment:44 Changed 6 months ago by dimpase

and

comment:45 Changed 5 months ago by slelievre

  • Branch changed from u/dimpase/rebased/mjo/ticket/29631 to u/slelievre/29631
  • Commit changed from 7e16ea979d4bfa1a26ef3b08a29ca626b83b6ff5 to 60e69eddc1760ec54c279b2d84c3b7e33843a4b7

New commits:

60e69ed29631: add fplll and libm4ri debian distro info

comment:46 Changed 5 months ago by slelievre

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 mkoeppe

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 mkoeppe

In light of #33042, let's also set an upper bound for the linbox version

comment:49 Changed 5 months ago by slelievre

  • Dependencies set to #8450, #32576, #32828
  • Status changed from needs_review to needs_work

comment:50 Changed 5 months ago by git

  • Commit changed from 60e69eddc1760ec54c279b2d84c3b7e33843a4b7 to 56aba2fb60135e02d916bf1d69125bcfc28f52dc

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

56aba2f29631: upper bound for linbox version

comment:51 follow-up: Changed 5 months ago by slelievre

  • Status changed from needs_work to needs_review

Like that?

comment:52 in reply to: ↑ 51 Changed 5 months ago by mjo

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: Changed 5 months ago by slelievre

I mimicked build/pkgsfplll/spkg-configure.m4.

Is anything else needed here?

comment:54 Changed 5 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

comment:55 in reply to: ↑ 53 Changed 5 months ago by mjo

Replying to slelievre:

Is anything else needed here?

I think we need #8450 to be merged to clear up the merge conflict but otherwise the commits for this ticket are good to go.

comment:56 Changed 4 months ago by mjo

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: Changed 4 months ago by slelievre

#8450 made it into Sage 9.6.beta0.

comment:58 in reply to: ↑ 57 Changed 4 months ago by mjo

Replying to slelievre:

#8450 made it into Sage 9.6.beta0.

Merge failed though D:

comment:59 Changed 4 months ago by mjo

  • Branch changed from u/slelievre/29631 to u/mjo/ticket/29631
  • Commit changed from 56aba2fb60135e02d916bf1d69125bcfc28f52dc to fc083c2420e13c0dfebe0b04c1966ab615e3ea21
  • Dependencies #8450, #32576, #32828 deleted

New commits:

ab58ae4Trac #29631: don't force SHELL=bash when installing linbox.
94d7717Trac #29631: drop fplll from linbox's dependencies.
7afd36129361: Add Fedora and Gentoo linbox package info
8128f66Trac #29631: new spkg-configure.m4 for linbox.
48eab64add missing imports
91aee3bcorrect the package name: `-`->`_`
6ee694229631: add fplll and libm4ri debian distro info
fc083c229631: upper bound for linbox version

comment:60 Changed 3 months ago by git

  • Commit changed from fc083c2420e13c0dfebe0b04c1966ab615e3ea21 to bc7a6f8d2b2b79094998dbc63d33adb01fc021cf

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

9a8bf64Trac #29631: don't force SHELL=bash when installing linbox.
9f788f5Trac #29631: drop fplll from linbox's dependencies.
796b0c329361: Add Fedora and Gentoo linbox package info
9b7d1ecTrac #29631: new spkg-configure.m4 for linbox.
a5d8c72correct the package name: `-`->`_`
4d4135629631: add fplll and libm4ri debian distro info
bc7a6f829631: upper bound for linbox version

comment:61 Changed 2 months ago by mjo

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 dimpase

  • 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 mjo

Thanks! Now I guess I have to test the maxima ticket on github.

comment:64 Changed 2 months ago by mkoeppe

  • 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 mkoeppe

  • 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 vbraun

  • 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 dimpase

  • Commit bc7a6f8d2b2b79094998dbc63d33adb01fc021cf deleted

I've missed that build/pkgs/linbox/distros/fedora.txt also needs linbox-devel

Note: See TracTickets for help on using tickets.