#31592 closed defect (fixed)

Upgrade/patch singular more to remove bad -rpath linker options

Reported by: mkoeppe Owned by:
Priority: blocker Milestone: sage-9.3
Component: packages: standard Keywords:
Cc: justin, fbissey, dimpase, slelievre, jhpalmieri, gh-zlscherr, vdelecroix, arojas Merged in:
Authors: Matthias Koeppe Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: 2571513 (Commits, GitHub, GitLab) Commit: 25715130d515ab607a0d715e9dbda3863e149611
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

The issue reported in https://groups.google.com/g/sage-release/c/fXpbYwaqt9o/m/929SlEXcCAAJ is likely caused by the upgrade to singular (#25993) from 4.1.1p2 to 4.2.0p1. Upstream singular introduced some rather unfortunate linker flags in 4.1.3 or earlier.

We fix it by a number of PRs to the singular build system. The new snapshot release is made from the current singular spielwiese HEAD, which has merged the PRs. There are a number of other change since the last snapshot release built in #31552, which seem to be bugfixes.

Previous tickets: #25993, #31552

Change History (33)

comment:1 Changed 22 months ago by mkoeppe

Description: modified (diff)

comment:2 Changed 22 months ago by mkoeppe

comment:3 Changed 22 months ago by mkoeppe

Cc: fbissey added

comment:4 Changed 22 months ago by fbissey

While /usr and /usr/local made it back in DEFAULT_CHECKING_PATH in flint-check.m4, if you use that variable that means they have not been found there previously by the bit that check standard paths - that I have added https://github.com/Singular/Singular/pull/981. So, I haven't asked for that re-addition to be removed. If you get to the bit checking DEFAULT_CHECKING_PATH they won't be found there anyway.

It would have been nice for gmp-check.m4 to follow that model. That would also prevent /usr/lib and /usr/local/lib to ever end up in a rpath unless you explicitly pass it as a path to be checked.

comment:5 Changed 22 months ago by mkoeppe

Yes, I plan to make more improvements in this direction later. The first PR is designed to make no difference in behavior

comment:6 Changed 22 months ago by fbissey

The replacement of all those gmp detections by a single one is certainly overdue. Code duplication is such a maintenance mess. It is an improvement on that front already.

Last edited 22 months ago by fbissey (previous) (diff)

comment:7 in reply to:  2 Changed 22 months ago by mkoeppe

Replying to mkoeppe:

A first PR as preparation: https://github.com/Singular/Singular/pull/1072

It has been merged upstream.

comment:8 Changed 22 months ago by mkoeppe

Next PR: https://github.com/Singular/Singular/pull/1074 (Replace -Wl,-rpath -Wl,... by -Wl,-rpath,...)

comment:9 Changed 22 months ago by mkoeppe

And another one: https://github.com/Singular/Singular/pull/1075 (Define DEFAULT_CHECKING_PATH once only)

comment:10 Changed 22 months ago by mkoeppe

And another one: https://github.com/Singular/Singular/pull/1076 (m4/*.m4: In the search loops, test for the presence of headers using the compiler)

comment:11 Changed 22 months ago by mkoeppe

These three are all merged by upstream, so here's the next one:

https://github.com/Singular/Singular/pull/1078 (m4/flint-check.m4: Refactor)

comment:12 Changed 22 months ago by mkoeppe

Merged, so here is the last one:

https://github.com/Singular/Singular/pull/1080

comment:13 Changed 22 months ago by fbissey

I am not sure why you leave '/usr/local'? I would have thought anything in there would be found when testing DEFAULTS.

comment:14 Changed 22 months ago by mkoeppe

Not true on macOS when using CC=CC=gcc -isysroot ....

comment:15 Changed 22 months ago by fbissey

OK, fair enough.

comment:16 Changed 22 months ago by mkoeppe

Description: modified (diff)

comment:17 Changed 22 months ago by mkoeppe

Branch: u/mkoeppe/upgrade_patch_singular_more_to_remove_bad__rpath_linker_options

comment:18 Changed 22 months ago by mkoeppe

Authors: Matthias Koeppe
Cc: dimpase slelievre jhpalmieri gh-zlscherr vdelecroix arojas added
Commit: ea8652bb55b4e8e41234a2d2cf71ff28f112f261
Description: modified (diff)
Status: newneeds_review

All of the above PRs have been merged by upstream.

I have made another PR, https://github.com/Singular/Singular/pull/1081, just with minor fixes to the Singular docbuild so the process of making a release is simpler: autoreconf -fi && ./configure --disable-pyobject-module --enable-doc-build && make && make dist

New release for sage at https://github.com/mkoeppe/Singular/releases/tag/singular-4.2.0p1%2B2021-04-06%2Bsage


New commits:

ea8652bbuild/pkgs/singular: Use 4.2.0p1+2021-04-06+sage

comment:19 Changed 22 months ago by fbissey

Reviewers: François Bissey
Status: needs_reviewpositive_review

Thanks for taking the time to go through that mess that likely represented years of accumulated hacks. I am satisfied that it should work out of the box. It needs to go to the bots.

comment:20 Changed 22 months ago by git

Commit: ea8652bb55b4e8e41234a2d2cf71ff28f112f2611c005205f1cdd7167cb26449231f2d3052688ffe
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

1c00520build/pkgs/singular/spkg-install.in: Use --with-libparse - just in case we use a tarball without built documentation

comment:21 Changed 22 months ago by fbissey

That's a new option to me. I now see what objects it builds. I am unfamiliar with the documentation building, is that a component to help build it?

comment:22 Changed 22 months ago by mkoeppe

Yes, to my understanding it is needed by the docbuild to extract test cases from the library code.

comment:23 Changed 22 months ago by fbissey

Status: needs_reviewpositive_review

I am not against it but that may have been more appropriate for future tickets. On the other hand that would mean more bumping of singular, not like we need any more of that.

I am happy to put it back to positive review, that commit doesn't add any requirements which was my first knee jerk reaction to it.

comment:24 Changed 22 months ago by mkoeppe

I added it here so that the upstream CI integration tests with Sage run properly: https://github.com/Singular/Singular/actions/runs/723960919

comment:25 Changed 22 months ago by git

Commit: 1c005205f1cdd7167cb26449231f2d3052688ffe4769678e2b825d48420eddf5ab27b9be53dc9a20
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

4769678Merge tag '9.3.rc2' into t/31592/upgrade_patch_singular_more_to_remove_bad__rpath_linker_options

comment:26 Changed 22 months ago by mkoeppe

Status: needs_reviewpositive_review

now on top of 9.3.rc2

comment:27 Changed 22 months ago by mkoeppe

Status: positive_reviewneeds_work

Just saw this error on cygwin-standard, a cygwin-specific makefile bug related to libparse.

g++ -std=gnu++11 -O2 -g -march=native -pipe -fno-common -g0 -O3 -Wno-unused-function -Wno-trigraphs -Wno-unused-parameter -Wunknown-pragmas -Wno-unused-variable -fomit-frame-pointer -fwrapv -fvisibility=default -finline-functions -fno-exceptions -fno-threadsafe-statics -fno-enforce-eh-specs -fconserve-space -funroll-loops -fno-delete-null-pointer-checks -fno-rtti  -L/opt/sage-ee023d7e18dfacf53b08fd0cf862661c48fe39c5/lib -Wl,-rpath,/opt/sage-ee023d7e18dfacf53b08fd0cf862661c48fe39c5/lib  -pipe -fno-common -g0 -O3 -Wno-unused-function -Wno-trigraphs -Wno-unused-parameter -Wunknown-pragmas -Wno-unused-variable -fomit-frame-pointer -fwrapv -fvisibility=default -finline-functions -fno-exceptions -fconserve-space -funroll-loops -fno-delete-null-pointer-checks  -Wl,-Bdynamic  libparse.cc   -o libparse
libparse.cc:31:10: fatal error: factory/globaldefs.h: No such file or directory
   31 | #include "factory/globaldefs.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

https://github.com/mkoeppe/sage/runs/2309214320 (this run is on top of other tickets, in particular #28890, but let's play safe and remove 1c00520)

Last edited 22 months ago by mkoeppe (previous) (diff)

comment:28 Changed 22 months ago by git

Commit: 4769678e2b825d48420eddf5ab27b9be53dc9a2025715130d515ab607a0d715e9dbda3863e149611

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

2571513Revert "build/pkgs/singular/spkg-install.in: Use --with-libparse - just in case we use a tarball without built documentation"

comment:29 Changed 22 months ago by mkoeppe

Status: needs_workneeds_review
Last edited 22 months ago by mkoeppe (previous) (diff)

comment:30 Changed 22 months ago by mkoeppe

Status: needs_reviewpositive_review

Setting it back to positive review - back to the version reviewed in comment 19.

comment:31 Changed 22 months ago by mkoeppe

Priority: criticalblocker

comment:32 Changed 22 months ago by mkoeppe

Follow-up in #31642, where 1c00520 is coming back

comment:33 Changed 22 months ago by vbraun

Branch: u/mkoeppe/upgrade_patch_singular_more_to_remove_bad__rpath_linker_options25715130d515ab607a0d715e9dbda3863e149611
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.