Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Singular configure so it accepts NTL installed in nonstandard locations #29335

Closed
mkoeppe opened this issue Mar 15, 2020 · 27 comments
Closed

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Mar 15, 2020

As observed in #29104, Singular's configure may fail to find NTL even if Sage finds a system NTL. It continues to build, but the result is broken.

The details:

NTL's spkg-configure.m4 (from #27265/#27822) uses AC_CHECK_HEADER/AC_LINK_IFELSE/AC_RUN_IFELSE and therefore is able to find NTL in an environment where the user has set CPPFLAGS and LDFLAGS accordingly.
In that case, it sets AC_SUBST(SAGE_NTL_PREFIX, [''])

Then build/bin/sage-build-env-config.in does:

# This is usually blank if the system NTL is used, or $SAGE_LOCAL otherwise
export SAGE_NTL_PREFIX="@SAGE_NTL_PREFIX@"
if [ -n "$SAGE_NTL_PREFIX" ]; then
    # Many packages that depend on NTL accept a --with-ntl=<prefix> flag to
    # their ./configure scripts.  When using the system's NTL this is not
    # generally necessary, but when using the NTL package installed in
    # SAGE_LOCAL it is useful to pass it.
    export SAGE_CONFIGURE_NTL="--with-ntl=$SAGE_NTL_PREFIX"
fi

But Singular's configure (via its m4/ntl-check.m4) insists to find the headers in NTL_HOME_PATH, which defaults to DEFAULT_CHECKING_PATH="/usr /usr/local /sw /opt/local", which can of course be quite wrong.

On this ticket, we use a simple patch that modifies Singular's NTL detection code to make it similar to its GMP and FLINT detection codes.

An alternative solution would be to always provide the --with-ntl=PREFIX option to Singular.
We should also make it an error if Singular's configure cannot find NTL.

See also: #25993 - Upgrade Singular

CC: @dimpase @embray @kiwifb @mwageringel @antonio-rojas

Component: build: configure

Author: Matthias Koeppe

Branch/Commit: 50f62c7

Reviewer: Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/29335

@mkoeppe mkoeppe added this to the sage-9.1 milestone Mar 15, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2020

comment:1

Singular's checks for GMP and FLINT are almost as bad, but because the check for the header file is commented out, it happens to accept the libraries that are accessible via CPPFLAGS and LDFLAGS.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2020

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2020

New commits:

50f62c7build/pkgs/singular: Add patches to accept system NTL in nonstandard locations

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2020

Commit: 50f62c7

@mkoeppe mkoeppe changed the title Always find SAGE_NTL_PREFIX and call Singular configure with it Fix Singular configure so it accepts NTL installed in nonstandard locations Mar 15, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2020

Author: Matthias Koeppe

@kiwifb
Copy link
Member

kiwifb commented Mar 15, 2020

comment:6

Those detections are a bit better in newer versions of singular, but still a bit off. My pull request to sanitize the new flint detection code has been accepted but that's not the only thing that should be sanitized. Singular/Singular#981

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2020

comment:7

Glad to hear that upstream accepts build cleanup patches!

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2020

comment:8

The fix on this ticket works (see #29104 / https://github.com/mkoeppe/sage/runs/508693083) - but it turns out that homebrew's NTL is not suitable:

  [singular-4.1.1p2.p0]   ld: illegal thread local variable reference to regular symbol __ZN3NTL13ErrorCallbackE for architecture x86_64
  [singular-4.1.1p2.p0]   clang: error: linker command failed with exit code 1 (use -v to see invocation)
  [singular-4.1.1p2.p0]   make[8]: *** [libSingular.la] Error 1

This is something that our ntl/spkg-configure.m4 should be checking. (This is #29339.)

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 16, 2020

comment:10

Ready for review

@dimpase
Copy link
Member

dimpase commented Mar 16, 2020

comment:11

what's going on in ./configure with NTL headers? Why is it there, and not in the corresponding spkg-configure.m4 ?

Why one cannot just use AC_CHECK_HEADER(S)(), instead of a dodgy loop?

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 16, 2020

comment:12

Making upstream singular configuration non-dodgy is beyond the scope of this ticket.

@dimpase
Copy link
Member

dimpase commented Mar 16, 2020

comment:13

oops, sorry, I have not realised that's Singular's ./configure
(stress and jetlag :-( )

how about we just pass it what's Sage already knows about NTL headers, i.e.
SAGE_NTL_PREFIX ?

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 16, 2020

comment:14

The problem is that we do not really determine SAGE_NTL_PREFIX. It is empty if the system package is used.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 16, 2020

comment:15

(see ticket description)

@dimpase
Copy link
Member

dimpase commented Mar 16, 2020

comment:16

Replying to @mkoeppe:

The problem is that we do not really determine SAGE_NTL_PREFIX. It is empty if the system package is used.

But we can, as we call AC_CHECK_HEADER, and it sets ac_cv_header_
so we can easily store it somewhere.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 16, 2020

comment:17

ac_cv_header_NTL_ZZ_h=yes

@dimpase
Copy link
Member

dimpase commented Mar 16, 2020

comment:18

OK, but we can use AX_ABSOLUTE_HEADER() - as used in MPIR's spkg-configure.m4
to get the absolute path.

IMHO this way we don't have to patch Singular at all, right? Just pass the value, whatever it is.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 16, 2020

comment:19

I wouldn't consider use of this terrible macro an improvement.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 16, 2020

comment:20

In particular, reconstructing an install prefix from the location of a header file is NOT a good technical solution.

@dimpase
Copy link
Member

dimpase commented Mar 16, 2020

comment:21

if you don't want a terrible macro, you can parse NTL/config_log.h to get the value of DEF_PREFIX, which is exactly the prefix we're after. If it's not defined, one can look forPREFIX, which would amount for the same thing. Finally, if neither are defined,
the prefix is /usr/local.
Cf. https://www.shoup.net/ntl/doc/tour-unix.html

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 16, 2020

comment:22

Sorry, none of these are improvements over the simple patch.

@dimpase
Copy link
Member

dimpase commented Mar 18, 2020

comment:23

OK. Upstream really needs to get this stuff fixed.

@dimpase
Copy link
Member

dimpase commented Mar 18, 2020

Reviewer: Dima Pasechnik

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 18, 2020

comment:24

Thanks.

@vbraun
Copy link
Member

vbraun commented Mar 22, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants