Skip to content

upgrade gsl to version 2.7 #32587

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dimpase opened this issue Sep 29, 2021 · 32 comments
Closed

upgrade gsl to version 2.7 #32587

dimpase opened this issue Sep 29, 2021 · 32 comments

Comments

@dimpase
Copy link
Member

dimpase commented Sep 29, 2021

One API change in GSL 2.7 causes trouble in Sage, see

Some distros, e.g. Gentoo, now have GSL 2.7 as the default.

Depends on #32607

CC: @orlitzky @sagetrac-tmonteil

Component: packages: standard

Keywords: upgrade, gsl

Author: Lorenz Panny, John Palmieri, Matthias Koeppe

Branch/Commit: bb8b92d

Reviewer: Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/32587

@dimpase dimpase added this to the sage-9.5 milestone Sep 29, 2021
@yyyyx4
Copy link
Member

yyyyx4 commented Oct 1, 2021

Branch: public/32587

@yyyyx4
Copy link
Member

yyyyx4 commented Oct 1, 2021

comment:1

Here's an attempt. It seemed to build successfully, but I haven't run any tests so far.


New commits:

e4a133fupdate GSL to 2.7

@yyyyx4
Copy link
Member

yyyyx4 commented Oct 1, 2021

Commit: e4a133f

@yyyyx4
Copy link
Member

yyyyx4 commented Oct 1, 2021

Author: Lorenz Panny

@yyyyx4
Copy link
Member

yyyyx4 commented Oct 1, 2021

comment:2

I just ran make ptestlong with this branch on a fresh Debian 11; all tests passed.

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 1, 2021

comment:3

Do the changes to the library also work with an older GSL?

@yyyyx4
Copy link
Member

yyyyx4 commented Oct 1, 2021

comment:4

They should: The new code simply uses the macros provided by GSL itself (in gsl_complex.h) to work with the gsl_complex struct, and those macros haven't changed (they are only #ifdef'd to something else). That said, I haven't tested it.

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 1, 2021

comment:5

In this case, I would recommend the following. Split this ticket into two:

  • one ticket for updating the library so that it can work with system GSL 2.7.
    Because this ticket will not include a change to packages, the patchbot will run on it.
  • a follow-up ticket that does the upgrade of the package.

With each of the tickets, we can run portability testing on GH Actions (https://doc.sagemath.org/html/en/developer/portability_testing.html) to make sure that it still works with system GSL on all supported platforms. If necessary, we may need to adjust build/pkgs/gsl/spkg-configure.m4. Currently we accept all GSL >= 2.4.

@yyyyx4
Copy link
Member

yyyyx4 commented Oct 2, 2021

Dependencies: #32607

@yyyyx4
Copy link
Member

yyyyx4 commented Oct 2, 2021

comment:7

Alright, all the code changes are now in #32607.

Remaining diff in this branch (after #32607): sagemath/sagetrac-mirror@6633bc4...e4a133f

@dimpase
Copy link
Member Author

dimpase commented Oct 2, 2021

comment:8

I'm a bit confused, as Sage with system's GLS 2.7 (as on Gentoo Linux) appears to work without
any changes in the source.

@orlitzky
Copy link
Contributor

orlitzky commented Oct 2, 2021

comment:9

Replying to @dimpase:

I'm a bit confused, as Sage with system's GLS 2.7 (as on Gentoo Linux) appears to work without
any changes in the source.

Probably the USE=deprecated that is on by default.

@jhpalmieri
Copy link
Member

comment:10

From #31621: can we also make the following changes?

  • change gsl's spkg-install.in as follows:
diff --git a/build/pkgs/gsl/spkg-install.in b/build/pkgs/gsl/spkg-install.in
index 179b78963a..ef156a7a5c 100644
--- a/build/pkgs/gsl/spkg-install.in
+++ b/build/pkgs/gsl/spkg-install.in
@@ -1,5 +1,5 @@
 cd src
 
-sdh_configure LIBS="`pkg-config --libs-only-l cblas` -lm"
+sdh_configure LIBS="`pkg-config --libs-only-l cblas` -lm" LDFLAGS="$LDFLAGS `pkg-config --libs-only-L cblas`"
 sdh_make
 sdh_make_install

As it stands, on OSX + homebrew, ./configure --with-system-gsl=no fails when building gsl with the error

configure: error: in `/Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage-9.5.beta1/local/var/tmp/sage/build/gsl-2.6/src':
configure: error: C compiler cannot create executables

and the config.log file says ld: library not found for -lopenblas.

  • Also, I think pkgconf should be an order-only dependency for gsl:
diff --git a/build/pkgs/gsl/dependencies b/build/pkgs/gsl/dependencies
index 052eb4373d..576809127c 100644
--- a/build/pkgs/gsl/dependencies
+++ b/build/pkgs/gsl/dependencies
@@ -1,4 +1,4 @@
-$(BLAS)
+$(BLAS) | pkgconf
 
 ----------
 All lines of this file are ignored except the first.

since the package installation scripts use pkg-config.

@yyyyx4
Copy link
Member

yyyyx4 commented Oct 23, 2021

comment:11

Replying to @jhpalmieri:

From #31621: can we also make the following changes?

Feel free to modify this branch as you please. I don't think I'm familiar enough with Sage's build system to dive any deeper into this...

@dimpase
Copy link
Member Author

dimpase commented Oct 23, 2021

comment:12

comment:10 actually gives you diffs you can apply (click on "Unified")

I am travelling and probably won't get around to do this before tomorrow

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 23, 2021

Changed commit from e4a133f to e7e2f3f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 23, 2021

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

f6f3223update GSL to 2.7
e7e2f3ftrac 32587: add pkgconf as order-only dependency for gsl,

@jhpalmieri
Copy link
Member

comment:14

Here are my proposed changes. (Dima actually proposed the change to spkg-install.in, to give credit where it's due.)

@yyyyx4
Copy link
Member

yyyyx4 commented Oct 24, 2021

Changed author from Lorenz Panny to Lorenz Panny, John Palmieri

@slel

This comment has been minimized.

@slel
Copy link
Member

slel commented Oct 25, 2021

Changed keywords from none to upgrade, gsl

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 28, 2021

Changed commit from e7e2f3f to 35c1b5f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 28, 2021

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

35c1b5fbuild/pkgs/gsl/spkg-install.in: Also pass CPPFLAGS, CFLAGS to configure

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 28, 2021

comment:19

... as suggested in #31621 comment:58

@dimpase
Copy link
Member Author

dimpase commented Nov 1, 2021

Reviewer: Dima Pasechnik

@dimpase
Copy link
Member Author

dimpase commented Nov 1, 2021

comment:20

lgtm

@vbraun
Copy link
Member

vbraun commented Nov 2, 2021

comment:21

Merge conflict

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 3, 2021

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

bb8b92dMerge tag '9.5.beta5' into public/32587

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 3, 2021

Changed commit from 35c1b5f to bb8b92d

@yyyyx4
Copy link
Member

yyyyx4 commented Nov 3, 2021

Changed author from Lorenz Panny, John Palmieri to Lorenz Panny, John Palmieri, Matthias Koeppe

@yyyyx4
Copy link
Member

yyyyx4 commented Nov 3, 2021

comment:23

Trivial merge.

@vbraun
Copy link
Member

vbraun commented Nov 12, 2021

Changed branch from public/32587 to bb8b92d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants