Opened 9 months ago

Closed 8 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:

Status badges

Description (last modified by mkoeppe)

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 9 months ago by mkoeppe

Actually, driver error, I ran into #29620 again

comment:2 Changed 9 months ago by mkoeppe

  • 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.

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

comment:3 Changed 9 months ago by mkoeppe

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 9 months ago by mkoeppe

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:5 Changed 9 months ago by mkoeppe

  • Priority changed from major to critical

Changed 9 months ago by mkoeppe

comment:6 Changed 9 months ago by mkoeppe

  • Branch set to u/mkoeppe/_usr_local_leaks_into_singular_build

comment:7 Changed 9 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • 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:

8b5b273build/pkgs/singular/checksums.ini: Use 4.2.0p1+2021-03-24+sage-2
Last edited 9 months ago by mkoeppe (previous) (diff)

comment:8 Changed 9 months ago by mkoeppe

Upstream has merged the two PRs.

Needs review

comment:9 Changed 9 months ago by slelievre

  • 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: Changed 9 months ago by mkoeppe

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 9 months ago by fbissey

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 9 months ago by mkoeppe

  • Reviewers set to https://github.com/mkoeppe/sage/runs/2197735803

comment:13 Changed 8 months ago by jhpalmieri

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: Changed 8 months ago by 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)

comment:15 in reply to: ↑ 14 Changed 8 months ago by fbissey

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 8 months ago by mkoeppe

Beyond the scope of this ticket to switch upstream to another build system.

comment:17 follow-up: Changed 8 months ago by fbissey

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 8 months ago by mkoeppe

Replying to fbissey:

Is there anything holding that ticket?

Not as far as I can see

comment:19 Changed 8 months ago by fbissey

  • 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 8 months ago by mkoeppe

Thanks!

comment:21 Changed 8 months ago by mkoeppe

Next ticket in the same direction: #31562

comment:22 Changed 8 months ago by mkoeppe

  • Priority changed from critical to blocker

Setting priority to blocker to bring this ticket to the attention of the release bot.

comment:23 Changed 8 months ago by vbraun

  • Branch changed from u/mkoeppe/_usr_local_leaks_into_singular_build to 8b5b2739d107271f8477cee8cd941bec8b255827
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.