Opened 5 years ago

Closed 5 years ago

#21188 closed defect (fixed)

Patch to Singular build to not explicitly link to MPIR

Reported by: embray Owned by:
Priority: critical Milestone: sage-7.4
Component: packages: standard Keywords: FLINT not found --with-mp=gmp -lmpir
Cc: leif Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer, Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: bc84a91 (Commits, GitHub, GitLab) Commit: bc84a9124e8f6eb88e1120ebb4d36ba0ceec7d17
Dependencies: Stopgaps:

Status badges

Description

When building Singular on a system that has Flint (i.e. in Sage), its configure/makefiles add some compiler and linker flags specifically "needed" for Flint.

One of these flags includes -lmpir which is incongruous with the fact that Singular itself links with GMP with -lgmp.

This patch removes the unnecessary -lmpir. As was mentioned in this thread, there has been effort in the past (for compatibility purposes) to remove explicit mentions of "mpir" in build systems for Sage packages in favor of "gmp". This is just an example that slipped through the cracks.

It didn't cause a problem on Linux because everything in Singular linked first with GMP via -lgmp, so all symbols got resolved to libgmp, and libmpir was never even used (not even added as a DT_NEEDED entry). However due to subtle linking differences in Cygwin (that I have no worked out the specifics of), some symbols end up being found in libgmp while most others are found in libmpir, and the resulting executables end up loading both.

This is related to #21174, but not necessarily dependent. Not sure whether or not this is worth reporting upstream.

Change History (26)

comment:1 Changed 5 years ago by jdemeyer

I would prefer to bump the Singular version number, to force the patch to take effect.

And remember to set the ticket to needs_review when you are done.

comment:2 Changed 5 years ago by embray

Okay, yes of course.

comment:3 Changed 5 years ago by embray

  • Status changed from new to needs_review

comment:4 Changed 5 years ago by git

  • Commit changed from e57745150784ac435b6150bb5cab9dcb494e0a74 to ff9fb0a0f37e43f95a0de0963db4efceff95cd08

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

ff9fb0aBump Singular spkg version number

comment:5 Changed 5 years ago by jdemeyer

  • Milestone set to sage-7.4
  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work
[singular-3.1.7p1.p2] Applying ../patches/flint-libs.patch
[singular-3.1.7p1.p2] can't find file to patch at input line 6
[singular-3.1.7p1.p2] Perhaps you used the wrong -p or --strip option?
[singular-3.1.7p1.p2] The text leading up to this was:
[singular-3.1.7p1.p2] --------------------------
[singular-3.1.7p1.p2] |Remove explicit references to -lmpir; instead use plain GMP (which may have
[singular-3.1.7p1.p2] |been provided by MPIR).
[singular-3.1.7p1.p2] |diff -ruN a/factory/configure b/factory/configure
[singular-3.1.7p1.p2] |--- a/factory/configure  2016-08-04 14:32:27.460652100 +0200
[singular-3.1.7p1.p2] |+++ b/factory/configure  2016-08-04 14:31:41.086952100 +0200
[singular-3.1.7p1.p2] --------------------------
[singular-3.1.7p1.p2] File to patch: 
[singular-3.1.7p1.p2] Skip this patch? [y] 
[singular-3.1.7p1.p2] Skipping patch.
[singular-3.1.7p1.p2] 1 out of 1 hunk ignored
[singular-3.1.7p1.p2] can't find file to patch at input line 18
[singular-3.1.7p1.p2] Perhaps you used the wrong -p or --strip option?
[singular-3.1.7p1.p2] The text leading up to this was:
[singular-3.1.7p1.p2] --------------------------
[singular-3.1.7p1.p2] |diff -ruN a/factory/flint-check.m4 b/factory/flint-check.m4
[singular-3.1.7p1.p2] |--- a/factory/flint-check.m4     2016-08-04 14:32:27.439640500 +0200
[singular-3.1.7p1.p2] |+++ b/factory/flint-check.m4     2016-08-04 14:31:26.780956400 +0200
[singular-3.1.7p1.p2] --------------------------
[singular-3.1.7p1.p2] File to patch: 
[singular-3.1.7p1.p2] Skip this patch? [y] 
[singular-3.1.7p1.p2] Skipping patch.
[singular-3.1.7p1.p2] 1 out of 1 hunk ignored
[singular-3.1.7p1.p2] can't find file to patch at input line 30
[singular-3.1.7p1.p2] Perhaps you used the wrong -p or --strip option?
[singular-3.1.7p1.p2] The text leading up to this was:
[singular-3.1.7p1.p2] --------------------------
[singular-3.1.7p1.p2] |diff -ruN a/Singular/configure.in b/Singular/configure.in
[singular-3.1.7p1.p2] |--- a/Singular/configure.in      2016-08-04 14:32:25.813801600 +0200
[singular-3.1.7p1.p2] |+++ b/Singular/configure.in      2016-08-04 14:31:08.788116800 +0200
[singular-3.1.7p1.p2] --------------------------
[singular-3.1.7p1.p2] File to patch: 
[singular-3.1.7p1.p2] Skip this patch? [y] 
[singular-3.1.7p1.p2] Skipping patch.
[singular-3.1.7p1.p2] 1 out of 1 hunk ignored
[singular-3.1.7p1.p2] can't find file to patch at input line 41
[singular-3.1.7p1.p2] Perhaps you used the wrong -p or --strip option?
[singular-3.1.7p1.p2] The text leading up to this was:
[singular-3.1.7p1.p2] --------------------------
[singular-3.1.7p1.p2] |--- a/Singular/configure 2016-08-04 14:32:27.084437500 +0200
[singular-3.1.7p1.p2] |+++ b/Singular/configure 2016-08-04 15:12:29.084236200 +0200
[singular-3.1.7p1.p2] --------------------------
[singular-3.1.7p1.p2] File to patch: 
[singular-3.1.7p1.p2] Skip this patch? [y] 
[singular-3.1.7p1.p2] Skipping patch.
[singular-3.1.7p1.p2] 1 out of 1 hunk ignored
[singular-3.1.7p1.p2] Error applying '../patches/flint-libs.patch'
[singular-3.1.7p1.p2] Error building Singular (error in apply_patches).

comment:6 Changed 5 years ago by leif

Note that #17254 upgrades Singular to 4.x (but it doesn't seem this ticket will be ready soon).

While (I've heard) a lot has changed in the new Singular version, including the build, I haven't looked at the latter yet.

So we may have to apply a similar patch to Singular 4.x as well.

comment:7 follow-up: Changed 5 years ago by leif

It is still unclear to me why the linker didn't bail out when I built Sage with --with-mp=gmp from scratch on a Linux system where MPIR isn't installed.

I also have no traces of MPIR in my Singular build logs even in Sage builds with MPIR.


And btw., unless you specify --as-needed, any shared library you give on the linker command line will appear as a dependency in the shared library or program you create, no matter whether any symbol from it is referenced or not, provided the library can be found of course. (This may be the default on some systems though.)

AFAIK on Cygwin static import libraries are used, where in principle only object files with referenced symbols should get included into the link (unless one specifies --whole-archive).

comment:8 follow-up: Changed 5 years ago by embray

Not sure what the problem is here. The patch applies fine for me.

[singular-3.1.7p1.p1] Applying /home/embray/src/sagemath/sage-cygwin/local/var/tmp/sage/build/singular-3.1.7p1.p1/patches/flint-libs.patch
[singular-3.1.7p1.p1] patching file factory/configure
[singular-3.1.7p1.p1] patching file factory/flint-check.m4
[singular-3.1.7p1.p1] patching file Singular/configure.in
[singular-3.1.7p1.p1] patching file Singular/configure

Interesting that you get a relative path in the "Applying" message while I get an absolute path. Odd.

Last edited 5 years ago by embray (previous) (diff)

comment:9 in reply to: ↑ 7 Changed 5 years ago by leif

  • Keywords FLINT not found --with-mp=gmp -lmpir added

Replying to leif:

It is still unclear to me why the linker didn't bail out when I built Sage with --with-mp=gmp from scratch on a Linux system where MPIR isn't installed.

Ok, looking closer, the configure check for FLINT simply failed (because of the -lmpir), and Singular "silently" got built without FLINT, without me ever noticing. XD


I also have no traces of MPIR in my Singular build logs even in Sage builds with MPIR.

Trying again I have to take that back; no idea how, but I somehow must have misgrepped mpir, or accidentally only searched Sage trees that were in fact built with GMP, searched all logs but Singular's was deleted, or whatever. When building with MPIR, -lmpir is of course there.


So more or less incidentally, this ticket will also fix that when configuring Sage with --with-mp=gmp, currently Singular gets built without (using) FLINT. :-)

comment:10 in reply to: ↑ 8 Changed 5 years ago by leif

Replying to embray:

Not sure what the problem is here. The patch applies fine for me.

The -p1 when patching (I think).

Looks like you didn't put latest/ in when creating the patches.

(Just try head build/pkgs/singular/patches/*.patch.)

comment:11 Changed 5 years ago by embray

Oh. It looks like maybe #20933 was merged into develop since I started this branch, or something. Or more specifically, since I committed to the branch that this branch cherry-picked from (which I don't pull in from develop often because I'm trying to get a version of Sage working, and pulling too frequently from develop is too much of a fast moving target).

comment:12 follow-up: Changed 5 years ago by leif

So although #20933 was closed as fixed 11 days ago, and its milestone is 7.3, it's not merged into 7.3?

Or am I missing something?

comment:13 in reply to: ↑ 12 ; follow-up: Changed 5 years ago by jdemeyer

#20933 was merged in 7.3.rc0, and the branch on this ticket also contains #20933.

comment:14 in reply to: ↑ 13 Changed 5 years ago by leif

Replying to jdemeyer:

#20933 was merged in 7.3.rc0, and the branch on this ticket also contains #20933.

Ah, ok. I thought it was the other way around, as I recalled the patches had latest/ in already before, but only some, and now it's a/latest/ for these.

Last edited 5 years ago by leif (previous) (diff)

comment:15 Changed 5 years ago by embray

I thought I clarified this above, but I'm working in an older branch that is a few weeks behind because I don't want incoming changes to constantly impede my progress.

But for the purpose of creating tickets I'm branching off the latest develop and cherry-picking individual commits.

comment:16 Changed 5 years ago by git

  • Commit changed from ff9fb0a0f37e43f95a0de0963db4efceff95cd08 to 8b48583dd29f66228b081259081ee8fddcdafd35

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

8b48583Bump Singular spkg version number

comment:17 Changed 5 years ago by embray

I updated the commit in this branch so that the newly added patch conforms to the "standardized" patch format, and also added a direct reference to this ticket.

Last edited 5 years ago by embray (previous) (diff)

comment:18 Changed 5 years ago by git

  • Commit changed from 8b48583dd29f66228b081259081ee8fddcdafd35 to bc84a9124e8f6eb88e1120ebb4d36ba0ceec7d17

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

bc84a91Adds a patch to Singular to remove spurious -lmpir flags when linking--never use 'mpir' explicitly.

comment:19 Changed 5 years ago by leif

  • Status changed from needs_work to needs_review

comment:20 Changed 5 years ago by leif

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Leif Leonhardy
  • Status changed from needs_review to positive_review

comment:21 Changed 5 years ago by leif

ROFL, this also fixes three failing doctests with GMP (two of them due to different ordering in results, the third a more weird one, namely an NTL abort when doing unpickle_all() twice, cf. #5838).

comment:22 Changed 5 years ago by leif

  • Priority changed from major to critical

Or maybe even a blocker.

comment:23 Changed 5 years ago by leif

  • Component changed from porting: Cygwin to packages: standard

comment:24 follow-up: Changed 5 years ago by embray

How was #5838 resolved before? Were the tests just disabled or something? I tried skimming through but it wasn't clear.

Update: I think I see what you mean now--just that they were failing when building Sage with GMP instead of MPIR. Well that's cool, I guess ;)

Last edited 5 years ago by embray (previous) (diff)

comment:25 in reply to: ↑ 24 Changed 5 years ago by leif

Replying to embray:

How was #5838 resolved before? Were the tests just disabled or something? I tried skimming through but it wasn't clear.

The double-unpickle error presumably vanished with Singular using FLINT (because the failing code apparently gets shadowed); you can at least get it back (also with MPIR) by simply configuring Singular --without-flint (as I've posted here; haven't updated #5838 yet). :-)

Note that the actual test that it's "solved" (or "worksforme") was added years after the issue popped up.

comment:26 Changed 5 years ago by vbraun

  • Branch changed from u/embray/singular-flint-mpir-patch to bc84a9124e8f6eb88e1120ebb4d36ba0ceec7d17
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.