Opened 22 months ago
Closed 22 months ago
#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: |
Description (last modified by )
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.
Change History (33)
comment:1 Changed 22 months ago by
Description: | modified (diff) |
---|
comment:2 follow-up: 7 Changed 22 months ago by
comment:3 Changed 22 months ago by
Cc: | fbissey added |
---|
comment:4 Changed 22 months ago by
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
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
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.
comment:7 Changed 22 months ago by
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
Next PR: https://github.com/Singular/Singular/pull/1074 (Replace -Wl,-rpath -Wl,...
by -Wl,-rpath,...
)
comment:9 Changed 22 months ago by
And another one: https://github.com/Singular/Singular/pull/1075 (Define DEFAULT_CHECKING_PATH
once only)
comment:10 Changed 22 months ago by
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
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:13 Changed 22 months ago by
I am not sure why you leave '/usr/local'? I would have thought anything in there would be found when testing DEFAULTS
.
comment:16 Changed 22 months ago by
Description: | modified (diff) |
---|
comment:17 Changed 22 months ago by
Branch: | → u/mkoeppe/upgrade_patch_singular_more_to_remove_bad__rpath_linker_options |
---|
comment:18 Changed 22 months ago by
Authors: | → Matthias Koeppe |
---|---|
Cc: | dimpase slelievre jhpalmieri gh-zlscherr vdelecroix arojas added |
Commit: | → ea8652bb55b4e8e41234a2d2cf71ff28f112f261 |
Description: | modified (diff) |
Status: | new → needs_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:
ea8652b | build/pkgs/singular: Use 4.2.0p1+2021-04-06+sage
|
comment:19 Changed 22 months ago by
Reviewers: | → François Bissey |
---|---|
Status: | needs_review → positive_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
Commit: | ea8652bb55b4e8e41234a2d2cf71ff28f112f261 → 1c005205f1cdd7167cb26449231f2d3052688ffe |
---|---|
Status: | positive_review → needs_review |
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
1c00520 | build/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
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
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
Status: | needs_review → positive_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
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
Commit: | 1c005205f1cdd7167cb26449231f2d3052688ffe → 4769678e2b825d48420eddf5ab27b9be53dc9a20 |
---|---|
Status: | positive_review → needs_review |
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
4769678 | Merge tag '9.3.rc2' into t/31592/upgrade_patch_singular_more_to_remove_bad__rpath_linker_options
|
comment:27 Changed 22 months ago by
Status: | positive_review → needs_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)
comment:28 Changed 22 months ago by
Commit: | 4769678e2b825d48420eddf5ab27b9be53dc9a20 → 25715130d515ab607a0d715e9dbda3863e149611 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
2571513 | Revert "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
Status: | needs_work → needs_review |
---|
comment:30 Changed 22 months ago by
Status: | needs_review → positive_review |
---|
Setting it back to positive review - back to the version reviewed in comment 19.
comment:31 Changed 22 months ago by
Priority: | critical → blocker |
---|
comment:33 Changed 22 months ago by
Branch: | u/mkoeppe/upgrade_patch_singular_more_to_remove_bad__rpath_linker_options → 25715130d515ab607a0d715e9dbda3863e149611 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
A first PR as preparation: https://github.com/Singular/Singular/pull/1072