Opened 17 months ago
Closed 17 months ago
#31552 closed defect (fixed)
Update singular to stop /usr/local from leaking into Singular build
Reported by: | mkoeppe | Owned by: | |
---|---|---|---|
Priority: | blocker | Milestone: | sage-9.3 |
Component: | packages: standard | Keywords: | |
Cc: | dimpase, slelievre, jhpalmieri, gh-zlscherr, fbissey, vdelecroix, arojas | Merged in: | |
Authors: | Matthias Koeppe | Reviewers: | François Bissey |
Report Upstream: | Fixed upstream, but not in a stable release. | Work issues: | |
Branch: | 8b5b273 (Commits, GitHub, GitLab) | Commit: | 8b5b2739d107271f8477cee8cd941bec8b255827 |
Dependencies: | Stopgaps: |
Description (last modified by )
On macOS, even when using gcc -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk
(which disables use of /usr/local
), Singular's configure
script sneaks in /usr/local/include
and /usr/local/lib
.
It was created by singular configure 4.2.0p1+2021-03-13+sage, which was generated by GNU Autoconf 2.69. Invocation command line was $ ./configure --prefix=/var/tmp/sage-9.3.rc0-cpython-38-darwin/local --libdir=/var/tmp/sage-9.3.rc0-cpython-38-darwin/local/lib --disable-maintainer-mode --disable-dependency-tracking --exec-prefix=/var/tmp/sage-9.3.rc0-cpython-38-darwin/local --bindir=/var/tmp/sage-9.3.rc0-cpython-38-darwin/local/bin --wit h-ntl=/var/tmp/sage-9.3.rc0-cpython-38-darwin/local --with-flint=/var/tmp/sage-9.3.rc0-cpython-38-darwin/local --enable-gfanlib --enable-Singular --enable- factory --disable-doc --disable-polymake --without-python --without-pythonmodule --disable-python --disable-python_module --disable-python-module --disable -static PATH: /private/var/tmp/sage-9.3.rc0-cpython-38-darwin/build/bin PATH: /private/var/tmp/sage-9.3.rc0-cpython-38-darwin/src/bin PATH: /var/tmp/sage-9.3.rc0-cpython-38-darwin/local/bin PATH: /private/var/tmp/sage-9.3.rc0-cpython-38-darwin/build/bin PATH: /private/var/tmp/sage-9.3.rc0-cpython-38-darwin/src/bin PATH: /var/tmp/sage-9.3.rc0-cpython-38-darwin/local/bin PATH: /private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-build-env-dcsf70hx/overlay/bin PATH: /private/var/folders/38/wnh4gf1552g_crsjnv2vmmww0000gp/T/pip-build-env-dcsf70hx/normal/bin PATH: /Users/mkoeppe/s/sage/sage-rebasing/worktree-dist/src/pkgs/sage_conf-relocatable/.tox/python-macos-10.15-python3.8/bin PATH: /usr/local/opt/xz/bin PATH: /usr/local/opt/gpatch/bin PATH: /usr/bin PATH: /bin PATH: /usr/sbin PATH: /sbin configure:20714: checking gmp.h usability configure:20714: gcc -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -c -O2 -g -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 -funroll-loops -fno-delete-null-pointer-checks -Qunused-arguments -pthread -I/usr/local/include conftest.c >&5 configure:20714: $? = 0 configure:20714: result: yes
If NTL from homebrew is installed, this can lead to an NTL threading mismatch:
[singular-4.2.0p1+2021-03-13+sage] ld: illegal thread local variable reference to regular symbol __ZN3NTL13ErrorCallbackE for architecture x86_64 [singular-4.2.0p1+2021-03-13+sage] clang: error: linker command failed with exit code 1 (use -v to see invocation)
PR: https://github.com/Singular/Singular/pull/1066
See also: #31348 build/pkgs/{mpir,mpfr}/spkg-configure.m4
: Check pkg-config first
Attachments (1)
Change History (24)
comment:1 Changed 17 months ago by
comment:2 Changed 17 months ago by
- Priority changed from critical to major
But the problem is real - Singular will not use gmp that can be found by compiler and linker if it finds /usr/local/include/gmp.h
This ought to be fixed upstream.
comment:3 Changed 17 months ago by
It also ignores an explicitly passed gmp directory (using --with-gmp=...) when there is an installation in /usr/local
because the search loop is missing a break
. https://github.com/Singular/Singular/blob/spielwiese/m4/gfanlib-check.m4#L40
This can only be fixed by making a PR + new singular tarball.
comment:4 Changed 17 months ago by
- Description modified (diff)
- Report Upstream changed from N/A to Reported upstream. No feedback yet.
comment:5 Changed 17 months ago by
- Priority changed from major to critical
Changed 17 months ago by
comment:6 Changed 17 months ago by
- Branch set to u/mkoeppe/_usr_local_leaks_into_singular_build
comment:7 Changed 17 months ago by
- Cc fbissey vdelecroix arojas added
- Commit set to 8b5b2739d107271f8477cee8cd941bec8b255827
- Status changed from new to needs_review
- Summary changed from /usr/local leaks into Singular build to Update singular to stop /usr/local from leaking into Singular build
Picking up a newer upstream spielwiese HEAD (which contains a few new commits that look like bugfixes) plus two new PRs: https://github.com/Singular/Singular/pull/1066 and https://github.com/Singular/Singular/pull/1067; the new tarball is made from tag https://github.com/mkoeppe/Singular/tree/singular-4.2.0p1+2021-03-24%2Bsage-2
Recipe for making the tarball: From within a Sage shell, I used ./configure --with-ntl=/usr/local --with-gmp=/usr/local --disable-pyobject-module --enable-doc-build --with-libparse && make && make dist
New commits:
8b5b273 | build/pkgs/singular/checksums.ini: Use 4.2.0p1+2021-03-24+sage-2
|
comment:8 Changed 17 months ago by
Upstream has merged the two PRs.
Needs review
comment:9 Changed 17 months ago by
- Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.
How to review this?
comment:10 follow-up: ↓ 11 Changed 17 months ago by
This is a tiny increment over the update done in #25993. If it builds, it ships, I'd think
comment:11 in reply to: ↑ 10 Changed 17 months ago by
Replying to mkoeppe:
This is a tiny increment over the update done in #25993. If it builds, it ships, I'd think
I'd agree with that. I have been thinking of giving the detection of other libraries the treatment I gave to flint and I decided to give up because I don't need that kind of problem right now. I like the simplicity of your minimal fix.
comment:12 Changed 17 months ago by
- Reviewers set to https://github.com/mkoeppe/sage/runs/2197735803
Tests have been running at https://github.com/mkoeppe/sage/runs/2197735803
comment:13 Changed 17 months ago by
Rather than using our own tarball, it's more common to use patches (and then delete the patches when they're merged in a stable upstream release). Why not do that here?
comment:14 follow-up: ↓ 15 Changed 17 months ago by
https://doc.sagemath.org/html/en/developer/packaging.html#when-to-patch-when-to-repackage-when-to-autoconfiscate explains why patching is not always possible. (In short: Changes to the autotooling are done, and thus the build system is regenerated)
comment:15 in reply to: ↑ 14 Changed 17 months ago by
Replying to mkoeppe:
https://doc.sagemath.org/html/en/developer/packaging.html#when-to-patch-when-to-repackage-when-to-autoconfiscate explains why patching is not always possible. (In short: Changes to the autotooling are done, and thus the build system is regenerated)
The fact that autotools files need to be regenerated is one of the strongest point in favor of cmake compared to autotools for downstream re-packagers in my opinion.
comment:16 Changed 17 months ago by
Beyond the scope of this ticket to switch upstream to another build system.
comment:17 follow-up: ↓ 18 Changed 17 months ago by
That was just a general comment. Is there anything holding that ticket? It mostly looks good to me, some of your testruns appear to have failed, one at least for lack of space. Anything more serious?
comment:18 in reply to: ↑ 17 Changed 17 months ago by
comment:19 Changed 17 months ago by
- Reviewers changed from https://github.com/mkoeppe/sage/runs/2197735803 to François Bissey
- Status changed from needs_review to positive_review
I am putting my name on the reviewer field then. Ship it.
comment:20 Changed 17 months ago by
Thanks!
comment:21 Changed 17 months ago by
Next ticket in the same direction: #31562
comment:22 Changed 17 months ago by
- Priority changed from critical to blocker
Setting priority to blocker to bring this ticket to the attention of the release bot.
comment:23 Changed 17 months ago by
- Branch changed from u/mkoeppe/_usr_local_leaks_into_singular_build to 8b5b2739d107271f8477cee8cd941bec8b255827
- Resolution set to fixed
- Status changed from positive_review to closed
Actually, driver error, I ran into #29620 again