#31578 closed defect (fixed)

gentoo: System gcc with multilib support generates linker (ld) warnings when running doctests, ntl-related

Reported by: Steven Trogdon Owned by:
Priority: critical Milestone: sage-9.4
Component: doctest framework Keywords:
Cc: François Bissey, Matthias Köppe, Michael Orlitzky, Dima Pasechnik, Isuru Fernando Merged in:
Authors: Steven Trogdon Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: a2ab555 (Commits, GitHub, GitLab) Commit: a2ab555e29407ae4dfb52d345fd0443ed1cc4867
Dependencies: Stopgaps:

Status badges

Description (last modified by François Bissey)

With 9.3.rc0 typical results with multilib support in gcc:

./sage -t --random-seed=0 src/sage/misc/cython.py
**********************************************************************
File "src/sage/misc/cython.py", line 136, in sage.misc.cython.?
Failed example:
    cython(os.linesep.join(code))
Expected nothing
Got:
    /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible ///usr/lib/libdl.so when searching for -ldl
    /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible ///usr/lib/libdl.a when searching for -ldl
    /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible ///usr/lib/libdl.so when searching for -ldl
    /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible ///usr/lib/libdl.a when searching for -ldl
    /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible ///usr/lib/libm.so when searching for -lm
    /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible ///usr/lib/libm.a when searching for -lm
    /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible ///usr/lib/libpthread.so when searching for -lpthread
    /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible ///usr/lib/libpthread.a when searching for -lpthread
    /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible ///usr/lib/libc.so when searching for -lc
    /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible ///usr/lib/libc.a when searching for -lc

There is no such problem with 9.3.beta7.

Upstream NTL pull request to add a .pc file

Attachments (1)

ntl-pc.patch (2.1 KB) - added by François Bissey 20 months ago.
Patch to ntl to create a ntl.pc file

Download all attachments as: .zip

Change History (104)

comment:1 Changed 20 months ago by Steven Trogdon

Using git bisect to locate the commit that is causing the issue I have from git bisect log:

git bisect start
# bad: [2c25f07cfd0cbb5cb4a9b57b7bc62ec997039987] Updated SageMath version to 9.3.rc0
git bisect bad 2c25f07cfd0cbb5cb4a9b57b7bc62ec997039987
# good: [8453ffb849b047893b6c61dd09176a84c9133342] Updated SageMath version to 9.3.beta7
git bisect good 8453ffb849b047893b6c61dd09176a84c9133342
# bad: [d32f41ee20a421ca91f36abbf889b4b4792f4822] Trac #20784: not all symbolic equations stay unevaluated
git bisect bad d32f41ee20a421ca91f36abbf889b4b4792f4822
# good: [8ff8b0b90c78d09fb2e71ee35205e96371e60f05] Trac #30537: Upgrade giac to 1.6.0-47
git bisect good 8ff8b0b90c78d09fb2e71ee35205e96371e60f05
# good: [2bcac009dd878272858ece0d853a67571fcd1c0d] Trac #31317: eclib interface uses bad default value for elliptic curve modular symbols
git bisect good 2bcac009dd878272858ece0d853a67571fcd1c0d
# bad: [64c1336fc356f17406c579e2471e0fa92f0e9123] Trac #31373: Upgrade ipython to 7.20.0 and jedi to 0.18.0
git bisect bad 64c1336fc356f17406c579e2471e0fa92f0e9123
# good: [74cfd6dbc612f8aad20cbfb67d672a4157f60b89] Trac #31358: python3 spkg-configure.m4: Do not reject python based on sysconfig LDFLAGS containing "-L."
git bisect good 74cfd6dbc612f8aad20cbfb67d672a4157f60b89
# good: [70cbb47cb79de70c21d00ed2aad8eb90f035f8b5] Trac #31364: Don't use deprecated numpy type aliases
git bisect good 70cbb47cb79de70c21d00ed2aad8eb90f035f8b5

The next commit that is to be tested is

commit 7b1e27b96e143d6d44b448692ba266050e667399 (HEAD)
Author: Matthias Koeppe <mkoeppe@math.ucdavis.edu>
Date:   Wed Feb 10 19:36:48 2021 -0800

    Merge distutils directives for Cython files using ntl

which fails to build with

Traceback (most recent call last):
  File "/local/sage-git/sage/build/pkgs/sagelib/src/setup.py", line 21, in <module>
    import sage.env
  File "/local/sage-git/sage/build/pkgs/sagelib/src/sage/env.py", line 160, in <module>
    HOSTNAME = var("HOSTNAME", socket.gethostname())
  File "/local/sage-git/sage/build/pkgs/sagelib/src/sage/env.py", line 144, in var
    import sage_conf
  File "/local/sage-git/sage/local/lib/python3.9/site-packages/sage_conf.py", line 9
    NTL_INCDIR = ".
                   ^
SyntaxError: EOL while scanning string literal

Somehow sage_conf.py has gotten corrupted?

# build/pkgs/sage_conf/src/sage_conf.py.  Generated from sage_conf.py.in by configure.

VERSION = "9.3.beta7"

MAXIMA = "/local/sage-git/sage/local/bin/maxima"

ARB_LIBRARY = "arb"

NTL_INCDIR = ".
.
///usr/include/NTL"
NTL_LIBDIR = ".
.
///usr/include"

comment:2 Changed 20 months ago by Steven Trogdon

Cc: Matthias Köppe added

@mkoeppe perhaps you have some insight into this. It's not clear to me why sage_conf.py is corrupted which prevents further bisecting. I have implemented the bisecting twice with the same result.

comment:3 Changed 20 months ago by Matthias Köppe

Cc: Michael Orlitzky Dima Pasechnik added

You should be able to see these corrupted values already in config.status. They are determined by build/pkgs/ntl/spkg-configure.m4 using a (horrible) macro called AX_ABSOLUTE_HEADER.

comment:4 Changed 20 months ago by Steven Trogdon

From config.status there is

S["SAGE_FLINT_PREFIX"]=""
S["NTL_LIBDIR"]=".\n"\
".\n"\
"///usr/include"
S["NTL_INCDIR"]=".\n"\
".\n"\
"///usr/include/NTL"
S["SAGE_NTL_PREFIX"]=""

comment:5 Changed 20 months ago by Matthias Köppe

Yes. So, as I thought, the error is made in the configure script.

You can try to debug this using CONFIG_SHELL="bash -x" ./configure.

comment:6 Changed 20 months ago by Matthias Köppe

What platform is this, by the way?

comment:7 in reply to:  6 Changed 20 months ago by Steven Trogdon

Replying to mkoeppe:

What platform is this, by the way?

Gentoo

Linux hp-probook 5.4.97-gentoo-x86_64 #16 SMP Mon Mar 15 21:41:09 MDT 2021 x86_64 AMD Ryzen 7 4700U with Radeon Graphics AuthenticAMD GNU/Linux

Same issue on

Linux hp-envy 5.4.38-gentoo #7 SMP Sat Jan 16 21:46:51 MST 2021 x86_64 Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz GenuineIntel GNU/Linux

comment:8 Changed 20 months ago by Matthias Köppe

The NTL variables in sage_conf were introduced in 6e30e3ac2d891a16fdd1ad89506db767d4edb9e5 in #31365.

Help with fixing this issue is welcome. Note that 9.3.rc0 built fine on gentoo in https://github.com/sagemath/sage/runs/2180104858?check_suite_focus=true

comment:9 in reply to:  8 Changed 20 months ago by Steven Trogdon

Replying to mkoeppe:

The NTL variables in sage_conf were introduced in 6e30e3ac2d891a16fdd1ad89506db767d4edb9e5 in #31365.

Help with fixing this issue is welcome. Note that 9.3.rc0 built fine on gentoo in https://github.com/sagemath/sage/runs/2180104858?check_suite_focus=true

For 9.3.rc0, sage_conf.py contains

# build/pkgs/sage_conf/src/sage_conf.py.  Generated from sage_conf.py.in by configure.

VERSION = "9.3.rc0"

MAXIMA = "/local/sage-git/sage/local/bin/maxima"

ARB_LIBRARY = "arb"

NTL_INCDIR = "///usr/include"
NTL_LIBDIR = "///usr/lib" 

So the question is whether the /// are causing the linker warnings when doctesting?

comment:10 Changed 20 months ago by Matthias Köppe

You can edit this file and run make sage_conf to have it installed

comment:11 in reply to:  2 Changed 20 months ago by Matthias Köppe

Replying to strogdon:

I have implemented the bisecting twice with the same result.

You may want to use git bisect --first-parent, so that you don't trip over intermediate commits within a ticket's branch.

comment:12 Changed 20 months ago by Steven Trogdon

FWIW, Sage-on-Gentoo does this to avoid linker warnings. But this change does not resolve things on vanilla Sage.

comment:13 in reply to:  10 Changed 20 months ago by Steven Trogdon

Replying to mkoeppe:

You can edit this file and run make sage_conf to have it installed

Correct me if I misunderstand. For 9.3.rc0 I changed

NTL_INCDIR = "///usr/include"
NTL_LIBDIR = "///usr/lib" 

to

NTL_INCDIR = "/usr/include"
NTL_LIBDIR = "/usr/lib"

did make sage_conf and ./sage -t --random-seed=0 src/sage/misc/cython.py still displays linker warnings. If correct I could make this change when bisecting to see if bisecting can proceed.

comment:14 Changed 20 months ago by Steven Trogdon

Ahh, maybe the issue is

NTL_LIBDIR = "/usr/lib"

Shouldn't this be

NTL_LIBDIR = "/usr/lib64"

comment:15 in reply to:  14 Changed 20 months ago by Steven Trogdon

Replying to strogdon:

Ahh, maybe the issue is

NTL_LIBDIR = "/usr/lib"

Shouldn't this be

NTL_LIBDIR = "/usr/lib64"

Yes, this is the issue.

comment:16 Changed 20 months ago by Matthias Köppe

OK, so ntl/spkg-configure.m4 needs changing so that it detects the correct library dir.

comment:17 Changed 20 months ago by Steven Trogdon

Cc: François Bissey added

comment:18 Changed 20 months ago by François Bissey

Yes, a number of packages don't do proper detection and try to do location instead. Which can cause troubles on multilib install. I'll have a look at the macro when I can.

comment:19 Changed 20 months ago by Steven Trogdon

The title is not really descriptive of the issue. It was a poor attempt to describe what what was happening. It should probably be changed.

Last edited 20 months ago by Steven Trogdon (previous) (diff)

comment:20 Changed 20 months ago by François Bissey

From the end ntl/spkg-configure.m4

    if test x$sage_spkg_install_ntl = xyes; then
        AC_SUBST(SAGE_NTL_PREFIX, ['$SAGE_LOCAL'])
    else
        AC_SUBST(SAGE_NTL_PREFIX, [''])
        AX_ABSOLUTE_HEADER([NTL/ZZ.h])
        ntl_inc_ntl_dir=`AS_DIRNAME(["$gl_cv_absolute_NTL_ZZ_h"])`
        ntl_inc_dir=`AS_DIRNAME(["$ntl_inc_ntl_dir"])`
        ntl_prefix=`AS_DIRNAME(["$ntl_inc_dir"])`
        AC_SUBST(NTL_INCDIR, [$ntl_inc_dir])
        AC_SUBST(NTL_LIBDIR, [$ntl_prefix/lib])
    fi

First of, the assumption on the value of NTL_LIBDIR is completely unwarranted. What is more extra-ordinary to me is that if the previous detection test before you get to this branch are successful nothing after AC_SUBST(SAGE_NTL_PREFIX, ['']) is useful.

Were any of these value provided for detection? No. So, why would they be needed later on?

If someone wants to provide some values for NTL_INCDIR and NTL_LIBDIR that can be arranged but then they should be tested - separately.

As far as I am concerned, those two variables should just be eliminated which would quite the patch during a rc.

comment:21 Changed 20 months ago by François Bissey

Also the macro AX_ABSOLUTE_HEADER is generating those strange /// at the beginning on purpose. So that some compiler (Sun originally) wouldn't complain with the result if it happens to be a system location. Not making it up, it is in its own documentation https://www.gnu.org/software/autoconf-archive/ax_absolute_header.html. I was really looking for a AX_ABSOLUTE_LIBRARY equivalent (there isn't).

comment:22 in reply to:  20 ; Changed 20 months ago by Matthias Köppe

Replying to fbissey:

From the end ntl/spkg-configure.m4

    if test x$sage_spkg_install_ntl = xyes; then
        AC_SUBST(SAGE_NTL_PREFIX, ['$SAGE_LOCAL'])
    else
        AC_SUBST(SAGE_NTL_PREFIX, [''])
        AX_ABSOLUTE_HEADER([NTL/ZZ.h])
        ntl_inc_ntl_dir=`AS_DIRNAME(["$gl_cv_absolute_NTL_ZZ_h"])`
        ntl_inc_dir=`AS_DIRNAME(["$ntl_inc_ntl_dir"])`
        ntl_prefix=`AS_DIRNAME(["$ntl_inc_dir"])`
        AC_SUBST(NTL_INCDIR, [$ntl_inc_dir])
        AC_SUBST(NTL_LIBDIR, [$ntl_prefix/lib])
    fi

First of, the assumption on the value of NTL_LIBDIR is completely unwarranted.

I agree, this was sloppy.

What is more extra-ordinary to me is that if the previous detection test before you get to this branch are successful nothing after AC_SUBST(SAGE_NTL_PREFIX, ['']) is useful.

Were any of these value provided for detection? No. So, why would they be needed later on?

They are needed because of the unfortunate need for a compile time environment at runtime of Sage when using the cython function. In this situation we cannot rely on the proper compile time environment being set (such as on macOS using the .homebrew-build-env.)

It would be fine with me to back out the setting of these two variables for Sage 9.3 if you think this helps.

comment:23 in reply to:  22 Changed 20 months ago by François Bissey

Replying to mkoeppe:

They are needed because of the unfortunate need for a compile time environment at runtime of Sage when using the cython function. In this situation we cannot rely on the proper compile time environment being set (such as on macOS using the .homebrew-build-env.)

That's unfortunately a valid use case. We may have had that discussion already. Total removal would break that use case while we are currently only dealing with extra warnings. Noisy, but not breaking any functionality.

comment:24 Changed 20 months ago by François Bissey

I think we should try AC_LIB_LINKFLAGS and see if that return something that we can use sensibly https://www.gnu.org/software/gnulib/manual/html_node/Searching-for-Libraries.html. This is gnulib stuff however.

comment:25 Changed 20 months ago by Matthias Köppe

For the general problem I have a ticket (#31041 - Set environment for sage.misc.cython) but not a solution yet

comment:26 in reply to:  25 Changed 20 months ago by François Bissey

Replying to mkoeppe:

For the general problem I have a ticket (#31041 - Set environment for sage.misc.cython) but not a solution yet

Yes, we should work on that there, that may turn out to be a bit of a patch bomb. At best I say we deal with accepting the potential warnings in the present ticket.

comment:27 Changed 20 months ago by Matthias Köppe

Milestone: sage-9.3sage-9.4

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review.

comment:28 Changed 20 months ago by Matthias Köppe

Summary: System gcc with multilib support generates linker (ld) warnings when running doctestsgentoo: System gcc with multilib support generates linker (ld) warnings when running doctests, ntl-related

comment:29 in reply to:  21 ; Changed 20 months ago by Dima Pasechnik

Replying to fbissey:

Also the macro AX_ABSOLUTE_HEADER is generating those strange /// at the beginning on purpose. So that some compiler (Sun originally) wouldn't complain with the result if it happens to be a system location. Not making it up, it is in its own documentation https://www.gnu.org/software/autoconf-archive/ax_absolute_header.html. I was really looking for a AX_ABSOLUTE_LIBRARY equivalent (there isn't).

well, on #28906 I wrote something akin to AX_ABSOLUTE_LIBRARY, see https://git.sagemath.org/sage.git/tree/m4/sage_absolute_lib.m4?id=2d4a1cdac919203e68f0d56174cd6257f600fe00

Last edited 20 months ago by Dima Pasechnik (previous) (diff)

comment:30 in reply to:  22 ; Changed 20 months ago by Dima Pasechnik

Replying to mkoeppe:

Replying to fbissey:

From the end ntl/spkg-configure.m4

    if test x$sage_spkg_install_ntl = xyes; then
        AC_SUBST(SAGE_NTL_PREFIX, ['$SAGE_LOCAL'])
    else
        AC_SUBST(SAGE_NTL_PREFIX, [''])
        AX_ABSOLUTE_HEADER([NTL/ZZ.h])
        ntl_inc_ntl_dir=`AS_DIRNAME(["$gl_cv_absolute_NTL_ZZ_h"])`
        ntl_inc_dir=`AS_DIRNAME(["$ntl_inc_ntl_dir"])`
        ntl_prefix=`AS_DIRNAME(["$ntl_inc_dir"])`
        AC_SUBST(NTL_INCDIR, [$ntl_inc_dir])
        AC_SUBST(NTL_LIBDIR, [$ntl_prefix/lib])
    fi

First of, the assumption on the value of NTL_LIBDIR is completely unwarranted.

I agree, this was sloppy.

short of using a kind of macro mentioned in comment:29, how about checking for presence of $ntl_prefix/lib/libntl.* - and if there is none, try $ntl_prefix/lib64/libntl.*

Should we error out if none of these works?

comment:31 in reply to:  30 Changed 20 months ago by François Bissey

Replying to dimpase:

how about checking for presence of $ntl_prefix/lib/libntl.* - and if there is none, try $ntl_prefix/lib64/libntl.*

What if they are both present? Does that cover Debian's way of doing multilib? Your macro is interesting.

comment:32 Changed 20 months ago by Dima Pasechnik

even though such a macro might not work on weird platforms, it would be a solution for Linux multilib. I don't recall if I tested on MacOS though. (I guess Arm vs non-arm cases might be *interesting*)

comment:33 in reply to:  29 ; Changed 20 months ago by Matthias Köppe

Replying to dimpase:

well, on #28906 I wrote something akin to AX_ABSOLUTE_LIBRARY, see https://git.sagemath.org/sage.git/tree/m4/sage_absolute_lib.m4?id=2d4a1cdac919203e68f0d56174cd6257f600fe00

I don't think this is the right approach. Mixing in runtime library paths can only create more confusion.

comment:34 Changed 20 months ago by Matthias Köppe

Wouldn't gentoo be able to provide a .pc file for NTL that we can check for first?

comment:35 in reply to:  34 Changed 20 months ago by Dima Pasechnik

Replying to mkoeppe:

Wouldn't gentoo be able to provide a .pc file for NTL that we can check for first?

it doesn't; even if it did, it won't help, as normally speaking -L is only specified for a library in its *.pc file only if the location is not known to the linker. Here, the location is known.

Amazingly, there is no standard way to find out this path...

comment:36 in reply to:  33 Changed 20 months ago by Dima Pasechnik

Replying to mkoeppe:

Replying to dimpase:

well, on #28906 I wrote something akin to AX_ABSOLUTE_LIBRARY, see https://git.sagemath.org/sage.git/tree/m4/sage_absolute_lib.m4?id=2d4a1cdac919203e68f0d56174cd6257f600fe00

I don't think this is the right approach. Mixing in runtime library paths can only create more confusion.

it already does create confusion.

Should NTL_LIBDIR be just empty list if the location of libntl is one of standard locations?

comment:37 Changed 20 months ago by Matthias Köppe

Most pkgconfig files do provide variables such as libdir.

comment:38 Changed 20 months ago by Matthias Köppe

Besides, we do not really need this absolute path. We only use these variables in cython_aliases because there is no .pc file to use.

comment:39 Changed 20 months ago by Michael Orlitzky

What exactly is the problem with cython? Is it just that people start sage without setting up their build environment (e.g. source .homebrew-build-env), and then expect to be able to compile things via cython()?

comment:40 Changed 20 months ago by Matthias Köppe

So, how about an upstream pull request that adds a pkgconfig file to NTL.

comment:41 in reply to:  39 Changed 20 months ago by Matthias Köppe

Replying to mjo:

What exactly is the problem with cython? Is it just that people start sage without setting up their build environment (e.g. source .homebrew-build-env), and then expect to be able to compile things via cython()?

Yes, precisely. (I know...)

comment:42 in reply to:  40 Changed 20 months ago by François Bissey

Replying to mkoeppe:

So, how about an upstream pull request that adds a pkgconfig file to NTL.

And we'll have to do it again and again with any packages we need? Not that it is a bad thing in and of itself.

comment:43 Changed 20 months ago by Matthias Köppe

Yes, one package at a time.

comment:44 Changed 20 months ago by Michael Orlitzky

Oh, well, that doesn't work =)

Seriously though, this problem is going to manifest elsewhere as well. Trying to guess at the exact environment that was present during ./configure so that it can be replicated during cython() will be a losing battle.

Adding a .pc file upstream might help or it might make things worse. There are two separate libdirs, and I'm not sure off the top of my head how pkg-config decides which *.pc file to use (and thus which libdir to return) on a multilib system.

Even if it works, you're basically forbidding people from installing things manually and passing -L and -I during the build, instead forcing everyone to both provide *.pc files and then override PKG_CONFIG_DIR. It seems like a lot of antipattern to bury ourselves under to avoid telling people to set up their build environment before they want to build.

comment:45 Changed 20 months ago by Matthias Köppe

The problem really just applies to the libraries listed in sage.misc.cython._standard_libs_libdirs_incdirs_aliases: the small set of libraries that were anointed as "standard" sometime in the ancient past.

comment:46 Changed 20 months ago by Matthias Köppe

https://github.com/libntl/ntl is taking pull requests.

comment:47 Changed 20 months ago by Dima Pasechnik

I already *.pc-fied mpfr, m4ri, eclib, perhaps something else as well, anyone wants to take the token?

comment:48 in reply to:  47 ; Changed 20 months ago by Dima Pasechnik

Replying to dimpase:

I already *.pc-fied mpfr, m4ri, eclib, perhaps something else as well, anyone wants to take the token?

also, I don't speak Perl, and for NTL config this is needed: https://github.com/libntl/ntl/blob/main/src/DoConfig

comment:49 in reply to:  48 Changed 20 months ago by François Bissey

Replying to dimpase:

Replying to dimpase:

I already *.pc-fied mpfr, m4ri, eclib, perhaps something else as well, anyone wants to take the token?

also, I don't speak Perl, and for NTL config this is needed: https://github.com/libntl/ntl/blob/main/src/DoConfig

Darn! I do, I am a bit rusty but the script is not really complicated enough to be a big issue.

comment:50 Changed 20 months ago by François Bissey

OK, I have started work at https://github.com/kiwifb/ntl/tree/pcfile so you can send your criticism already. There is one more little piece I think should dealt with before sending the PR is setting the variable PACKAGE_VERSION. Unfortunately, that bit will be a bit more complicated. It is contained in the file DIRNAME but preceded by the string ntl-. Some extra reading and parsing needs to be added to DoConfig for it.

comment:51 Changed 20 months ago by François Bissey

OK people can now experiment with the attached patch to see if it behaves as they expect.

comment:52 in reply to:  51 ; Changed 20 months ago by Steven Trogdon

Replying to fbissey:

OK people can now experiment with the attached patch to see if it behaves as they expect.

Perhaps cosmetic for purposes here but in the makefile there is INCLUDEDIR=$(PREFIX)/include which on a gentoo install gives

$ pkg-config --cflags ntl
-I$(PREFIX)/include/NTL

i.e. from the installed ntl.pc

includedir=$(PREFIX)/include/NTL

Somehow (PREFIX) should be {PREFIX}.

comment:53 in reply to:  52 ; Changed 20 months ago by François Bissey

Replying to strogdon:

Replying to fbissey:

OK people can now experiment with the attached patch to see if it behaves as they expect.

Perhaps cosmetic for purposes here but in the makefile there is INCLUDEDIR=$(PREFIX)/include which on a gentoo install gives

$ pkg-config --cflags ntl
-I$(PREFIX)/include/NTL

i.e. from the installed ntl.pc

includedir=$(PREFIX)/include/NTL

Somehow (PREFIX) should be {PREFIX}.

OK, I need to figure out a better substitution. I didn't notice the braces are the wrong kind. On top of that I shouldn't have added NTL I am sure.

comment:54 in reply to:  53 Changed 20 months ago by Steven Trogdon

Replying to fbissey:

Replying to strogdon:

Replying to fbissey:

OK people can now experiment with the attached patch to see if it behaves as they expect.

Perhaps cosmetic for purposes here but in the makefile there is INCLUDEDIR=$(PREFIX)/include which on a gentoo install gives

$ pkg-config --cflags ntl
-I$(PREFIX)/include/NTL

i.e. from the installed ntl.pc

includedir=$(PREFIX)/include/NTL

Somehow (PREFIX) should be {PREFIX}.

OK, I need to figure out a better substitution. I didn't notice the braces are the wrong kind. On top of that I shouldn't have added NTL I am sure.

The libdir thingy does work

sage: import pkgconfig                                                                                   
sage: pkgconfig.variables('ntl')['libdir']                                                               
'/usr/lib64'
sage:

comment:55 Changed 20 months ago by François Bissey

Yes, because when you configure on Gentoo, and probably other distros, LIBDIR is defined in full on the command line. If you were to do a vanilla config with the default location, you would get something in braces.

comment:56 Changed 20 months ago by François Bissey

It is just an extra line in DoConfig but I am not sure when I'll be able to post it.

comment:57 Changed 20 months ago by François Bissey

I have pushed on github though.

Changed 20 months ago by François Bissey

Attachment: ntl-pc.patch added

Patch to ntl to create a ntl.pc file

comment:58 Changed 20 months ago by François Bissey

Patch updated.

comment:59 Changed 19 months ago by Andrew

I have put this ntl-pc.patch in /etc/portage/patches/dev-libs/ntl and now we have /usr/lib64/pkgconfig/ntl.pc in all three latest images

I hoped to see linker warnings go away but Actions shows that those warnings are still there :(

cddlib-0.94m in logs confirms github surely used latest image.

comment:60 in reply to:  59 Changed 19 months ago by Matthias Köppe

Replying to gh-sheerluck:

I hoped to see linker warnings go away but Actions shows that those warnings are still there :(

A branch here on this ticket still needs to change Sage so it actually uses that pc file!

comment:61 Changed 19 months ago by François Bissey

Yes, I left making a branch for later. I wanted feedback on the ntl.pc file first. Making a branch is not without issues. There is a before ntl.pc file and after. So, some strategic decisions are needed.

  • Do we require a version of ntl that has a .pc file?
  • Do we have a transition period where we do one thing if there is a .pc file and another thing if not.

Those are important because they will shape ntl's spkg-configure.m4 file. I believe handling various scenario is not too hard in env.py so long as the spkg-configure.m4 does the right things, but that's where it becomes quite complicated if we allow a transition period.

comment:62 in reply to:  61 Changed 19 months ago by Matthias Köppe

Replying to fbissey:

  • Do we require a version of ntl that has a .pc file?

No, just use it if present and do whatever we do now if not.

comment:63 Changed 19 months ago by Andrew

Same linker warnings when building https://github.com/fredstro/psage

(for sagemath-9.3 it has feature/sage9.3 branch)

comment:64 Changed 19 months ago by Dima Pasechnik

how about putting the .pc file creation up as an NTL PR? (although admittedly upstream is rather picky regarding PRs, it seems - why is it a problem merging .gitignore updates? it beats me)

comment:65 Changed 19 months ago by Dima Pasechnik

Cc: Isuru Fernando added

actually, I just noticed that upstream merged autotools configuration for NTL written by Isuru. In src/libtool* dirs there. Maybe Isuru can tell us how to use it and adding a .pc file there will be trivial.

comment:66 Changed 19 months ago by François Bissey

If that's a replacement or a superset to the perl configuration tool, then writing a .pc file becomes really trivial. The perl stuff was tricky.

comment:67 Changed 19 months ago by Steven Trogdon

For discussion since I'm not an autoconf expert. With a system ntl.pc file installed the following modification

  • build/pkgs/ntl/spkg-configure.m4

    diff --git a/build/pkgs/ntl/spkg-configure.m4 b/build/pkgs/ntl/spkg-configure.m4
    index 48e41de6ea..d069a798de 100644
    a b SAGE_SPKG_CONFIGURE([ntl], [ 
    4444        ntl_inc_dir=`AS_DIRNAME(["$ntl_inc_ntl_dir"])`
    4545        ntl_prefix=`AS_DIRNAME(["$ntl_inc_dir"])`
    4646        AC_SUBST(NTL_INCDIR, [$ntl_inc_dir])
    47         AC_SUBST(NTL_LIBDIR, [$ntl_prefix/lib])
     47        ntl_libdir=`pkg-config --variable=libdir ntl`
     48        if test x$ntl_libdir = x; then
     49            AC_SUBST(NTL_LIBDIR, [$ntl_prefix/lib])
     50        else
     51            libdir=`basename $ntl_libdir`
     52            AC_SUBST(NTL_LIBDIR, [$ntl_prefix/$libdir])
     53        fi
    4854    fi
    4955])

gives, after ./configure (perhaps a ./bootstrap is needed before the ./configure)

$ grep NTL build/pkgs/sage_conf/src/sage_conf.py
NTL_INCDIR = "///usr/include"
NTL_LIBDIR = "///usr/lib64"

This assumes that pkg-config is installed and works correctly and that basename is installed. Not sure if there are autoconf equivalents to these. This gives the current result if a system ntl.pc file is not installed.

Last edited 19 months ago by Steven Trogdon (previous) (diff)

comment:68 in reply to:  67 ; Changed 19 months ago by Dima Pasechnik

Replying to strogdon: [...]

This assumes that pkg-config is installed and works correctly and that basename is installed. Not sure if there are autoconf equivalents to these.

PKG_CHECK_VAR and other PKG_ autoconf macros are ones to use from .m4 files to interact with pkg-config. Cf https://autotools.io/pkgconfig/variables.html

comment:69 in reply to:  68 ; Changed 19 months ago by Steven Trogdon

Replying to dimpase:

Replying to strogdon: [...]

This assumes that pkg-config is installed and works correctly and that basename is installed. Not sure if there are autoconf equivalents to these.

PKG_CHECK_VAR and other PKG_ autoconf macros are ones to use from .m4 files to interact with pkg-config. Cf https://autotools.io/pkgconfig/variables.html

OK, second try that seems to work:

  • build/pkgs/ntl/spkg-configure.m4

    diff --git a/build/pkgs/ntl/spkg-configure.m4 b/build/pkgs/ntl/spkg-configure.m4
    index 48e41de6ea..5f83cfaac4 100644
    a b SAGE_SPKG_CONFIGURE([ntl], [ 
    4444        ntl_inc_dir=`AS_DIRNAME(["$ntl_inc_ntl_dir"])`
    4545        ntl_prefix=`AS_DIRNAME(["$ntl_inc_dir"])`
    4646        AC_SUBST(NTL_INCDIR, [$ntl_inc_dir])
    47         AC_SUBST(NTL_LIBDIR, [$ntl_prefix/lib])
     47        PKG_CHECK_VAR([ntl_libdir], [ntl], [libdir])
     48        AS_IF([test "x$ntl_libdir" = x], [AC_SUBST(NTL_LIBDIR, [$ntl_prefix/lib])],
     49             [test "x$ntl_libdir" != x], [AC_SUBST(NTL_LIBDIR, [$ntl_prefix/[$(basename $ntl_libdir)]])]
     50            )
    4851    fi
    4952])

No comments are written to config.log. This assumes that test and basename are available.

comment:70 in reply to:  69 Changed 19 months ago by Andrew

Replying to strogdon:

OK, second try that seems to work:

These actions show that with your patch there are no skipping incompatible in logs anymore

(just sage -t --random-seed=0 src/sage/misc/sageinspect.py # Timed out but that is not for this ticket)

So, positive review, I guess?

comment:71 Changed 19 months ago by Steven Trogdon

Branch: u/strogdon/ntl_pkg_config

comment:72 Changed 19 months ago by Steven Trogdon

Authors: Steven Trogdon
Commit: a265a71c8b8f0fa8b65fa2fcd3914eb3500c0093
Status: newneeds_review

This branch will only be useful if the system ntl installs a pc file. Let's make sure I haven't missed some corner cases.


New commits:

a265a71update ntl/spkg-config.m4 to use system ntl.pc, when available, to set NTL_LIBDIR

comment:73 Changed 19 months ago by Dima Pasechnik

why [$ntl_prefix/[$(basename $ntl_libdir)]] and not just [$ntl_libdir] ?

comment:74 in reply to:  73 Changed 19 months ago by Steven Trogdon

Replying to dimpase:

why [$ntl_prefix/[$(basename $ntl_libdir)]] and not just [$ntl_libdir] ?

From what I recall (comment:23) the extra // in ///usr/lib were needed so I assumed ///usr/lib64 was also needed. If not needed then yes, use [$ntl_libdir].

comment:75 Changed 19 months ago by Dima Pasechnik

the /// story has to do with suppression of warnings from Sun compiler which is totally irrelevant to us.

comment:76 Changed 19 months ago by git

Commit: a265a71c8b8f0fa8b65fa2fcd3914eb3500c0093b81dc850448e6a3538d0038f556270401dc49845

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

b81dc85Remove extra // in definition of NTL_LIBDIR

comment:77 Changed 18 months ago by Dima Pasechnik

the question is: what to do with absence of .pc file?

comment:78 in reply to:  59 ; Changed 18 months ago by Dima Pasechnik

Replying to gh-sheerluck:

I have put this ntl-pc.patch in /etc/portage/patches/dev-libs/ntl and now we have /usr/lib64/pkgconfig/ntl.pc in all three latest images

what should to done to get ntl.pc into Gentoo mainline?

comment:79 in reply to:  78 ; Changed 18 months ago by Andrew

Replying to dimpase:

what should to done to get ntl.pc into Gentoo mainline?

It's for François Bissey (fbissey) to decide. Maybe Michael Orlitzky (mjo) can help.

comment:80 in reply to:  79 ; Changed 18 months ago by François Bissey

Replying to gh-sheerluck:

Replying to dimpase:

what should to done to get ntl.pc into Gentoo mainline?

It's for François Bissey (fbissey) to decide. Maybe Michael Orlitzky (mjo) can help.

One, since it seems that the mechanism works well enough, I will submit it upstream. Two, I can include it in the sage-on-gentoo overlay but Michael has gone to a lot of effort to bring ntl up to date in Gentoo so I will submit a PR there as well.

But hopefully upstream will accept it and include it in the next release.

comment:81 Changed 18 months ago by Michael Orlitzky

This is antithetical to why I work on these things in the first place. This is a sage bug, where sage is doing something stupid that it shouldn't be doing.

I work on these packages at the distro level to reduce the amount of work that I have to do. Distro maintainers should be able to ship working upstream software (which they're going to do anyway), and it should be usable by sage out-of-the-box. The user experience is improved with no additional effort.

Instead, what has happened is that now upstream and like five different distros all get bullied into re-creating every ugly hack that sage invents to work around sage problems. Gentoo doesn't need this patch; nobody does except sage, to fix the stupid thing that sage is doing.

Francois does most of the work anyway and I'm still happy to merge his work, but don't expect me to volunteer to backport patches like these that fix unrelated problems within sage.

comment:82 Changed 18 months ago by Steven Trogdon

Is there a way to fix this without requiring a .pc file? Can it be argued that ntl should really be supplying this file - apart from this sage issue?

comment:83 in reply to:  82 Changed 18 months ago by Matthias Köppe

Replying to strogdon:

Can it be argued that ntl should really be supplying this file - apart from this sage issue?

Yes, it is part of "best practices" of a library to supply a .pc file.

comment:84 in reply to:  82 ; Changed 18 months ago by Michael Orlitzky

Replying to strogdon:

Is there a way to fix this without requiring a .pc file? Can it be argued that ntl should really be supplying this file - apart from this sage issue?

Delete the NTL_* variables and tell macOS users that they have to run sage from homebrew if they built it using homebrew (only in rare cases where they later want to compile & run cython code).

There's no reason for most libraries to have a pkg-config file; although it's not the pkg-config file that I have a problem with if it solves an actual problem (at worse, it's harmless cruft). It's using the pkg-config file to work around a sage issue, tweaking sage to reject NTL when there is no pkg-config file, and then forcing everyone to backport (and test, and stabilize, and then support) a completely unnecessary patch if they want to use their own NTL with sage. We've all already spent time getting working versions of NTL into our distros. If I wanted sage-specific patched versions of all these packages, I could achieve that with a lot less wasted time.

comment:85 in reply to:  84 Changed 18 months ago by Matthias Köppe

Replying to mjo:

tweaking sage to reject NTL when there is no pkg-config file

I don't think this is part of the plan. We obviously cannot do this -- Sage supports lots of released distributions (which obviously do not have ntl.pc)

comment:86 in reply to:  84 Changed 18 months ago by Dima Pasechnik

Replying to mjo:

Replying to strogdon:

Is there a way to fix this without requiring a .pc file? Can it be argued that ntl should really be supplying this file - apart from this sage issue?

Delete the NTL_* variables and tell macOS users that they have to run sage from homebrew if they built it using homebrew (only in rare cases where they later want to compile & run cython code).

Even better would be to incorporate sourcing of all these things into sage script on Homebrew, no?

There's no reason for most libraries to have a pkg-config file;

this is only true for sane OSes with sane sysadmins.

although it's not the pkg-config file that I have a problem with if it solves an actual problem (at worse, it's harmless cruft). It's using the pkg-config file to work around a sage issue,

many (most?) (semi)distros are hostile to multiplatform solutions; e.g. Sage on Gentoo chooses not to run ./configure, forcing Sage to basically have a lot of cruft specifically for such cases. Sage has to jump hoops to make distros happy...

tweaking sage to reject NTL when there is no pkg-config file, and then forcing everyone to backport (and test, and stabilize, and then support) a completely unnecessary patch if they want to use their own NTL with sage. We've all already spent time getting working versions of NTL into our distros. If I wanted sage-specific patched versions of all these packages, I could achieve that with a lot less wasted time.

comment:87 in reply to:  84 ; Changed 18 months ago by Matthias Köppe

Replying to mjo:

There's no reason for most libraries to have a pkg-config file

From the viewpoint of distribution packaging, .pc files are not very useful. But that's missing the point entirely - pkg-config is designed as a solution to the problem of finding libraries in a heterogeneous environment.

comment:88 Changed 18 months ago by Matthias Köppe

How about in the absence of ntl.pc, to do a AC_TRY_LINK with candidates for the library subdirectory (lib, lib64). You can copy this kind of code from m4/ax_boost_base.m4.

comment:89 in reply to:  87 Changed 18 months ago by Michael Orlitzky

Replying to mkoeppe:

Replying to mjo:

There's no reason for most libraries to have a pkg-config file

From the viewpoint of distribution packaging, .pc files are not very useful. But that's missing the point entirely - pkg-config is designed as a solution to the problem of finding libraries in a heterogeneous environment.

For most libraries, AC_SEARCH_LIBS and friends accomplish that. Unless you're sure that all of your users will have pkg-config installed (and *.pc files for all of their libraries...), you need to have both PKG_CHECK_MODULES and fallback AC_SEARCH_LIBS checks in your configure.ac. But at that point, what purpose does the PKG_CHECK_MODULES check serve? You might as well delete it. Another benefit of AC_SEARCH_LIBS is that it lets you search for the same function in multiple libraries (or in no libraries!) easily in case the function you need is provided by different libraries or by the OS on various platforms.

Where pkg-config really shines is with packages whose layouts are universally wacky or with libraries meant to be used in nonstandard ways. And, particularly, it's better to have a standard pkg-config interface than it is to use apache-config, postgres-config, etc. if they're going to be there anyway.

But you don't really have to convince me that pkg-config itself is useful. I've been fixing awful build systems long enough, and still regularly find thousands of lines of (broken) configure.ac code that does god-knows-what with the stated goal of... detecting a library. It's much easier to send those people a pull request that deletes the whole thing and uses PKG_CHECK_MODULES instead.

comment:90 in reply to:  80 Changed 18 months ago by Matthias Köppe

Replying to fbissey:

since it seems that the mechanism works well enough, I will submit it upstream.

Great, please post the URL here on the ticket description when done

comment:91 Changed 18 months ago by Matthias Köppe

Branch: u/strogdon/ntl_pkg_configu/mkoeppe/ntl_pkg_config

comment:92 Changed 18 months ago by Matthias Köppe

Commit: b81dc850448e6a3538d0038f556270401dc4984599b5715d63e87ee1b7921d7073b762c3b996376a

I have taken the liberty to push a related fix to this ticket here.


New commits:

99b5715src/sage/rings/padics/padic_printing.pyx: Fix up distutils directive

comment:93 in reply to:  88 ; Changed 18 months ago by Matthias Köppe

Replying to mkoeppe:

How about in the absence of ntl.pc, to do a AC_TRY_LINK with candidates for the library subdirectory (lib, lib64). You can copy this kind of code from m4/ax_boost_base.m4.

Is anyone interested in implementing this?

comment:94 Changed 18 months ago by François Bissey

Description: modified (diff)

OK PR added to the description. Note that there is a minor change to the patch attached here. includedir shouldn't have included NTL as remarqued by Steve Trogdon.

comment:95 in reply to:  93 Changed 18 months ago by Dima Pasechnik

Replying to mkoeppe:

Replying to mkoeppe:

How about in the absence of ntl.pc, to do a AC_TRY_LINK with candidates for the library subdirectory (lib, lib64). You can copy this kind of code from m4/ax_boost_base.m4.

Is anyone interested in implementing this?

I can have a go at it, if noone else will.

comment:96 Changed 17 months ago by Andrew

While we all are waiting for upstream I come up with crazy idea. What if we merge this branch into develop just for the sake of GH Actions. It's not gonna break anything or hurt anyone, right?

comment:97 Changed 17 months ago by Matthias Köppe

I agree that there is no need to wait for upstream. If gentoo decides to ship a .pc file, that's a good enough reason to check for it in Sage.

The fix in ntl/spkg-configure.m4 could use some cleanup though. Getting NTL_LIBDIR from pkgconfig but not NTL_INCDIR seems strange and is basically guaranteeing breakage if any other distribution decides to ship a .pc file.

Also, the macro AS_IF does have an "run-if-false" clause, so its use can be simplified. https://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Common-Shell-Constructs.html

comment:98 Changed 17 months ago by Matthias Köppe

Status: needs_reviewneeds_work

comment:99 Changed 17 months ago by Steven Trogdon

Branch: u/mkoeppe/ntl_pkg_configu/strogdon/ntl_pkg_config

comment:100 Changed 17 months ago by Steven Trogdon

Commit: 99b5715d63e87ee1b7921d7073b762c3b996376aa2ab555e29407ae4dfb52d345fd0443ed1cc4867
Status: needs_workneeds_review

New update works on gentoo w/o system .pc file

$ grep NTL build/pkgs/sage_conf/src/sage_conf.py
NTL_INCDIR = "///usr/include"
NTL_LIBDIR = "///usr/lib"

and with system .pc file

$ grep NTL build/pkgs/sage_conf/src/sage_conf.py
NTL_INCDIR = "/usr/include"
NTL_LIBDIR = "/usr/lib64"

New commits:

a934637Merge branch 'u/mkoeppe/ntl_pkg_config' of trac.sagemath.org:sage into ntl_pkg_config
a2ab555update: also set NTL_INCDIR using pkgconfig when system ntl.pc is available

comment:101 Changed 17 months ago by Matthias Köppe

Reviewers: Matthias Koeppe
Status: needs_reviewpositive_review

LGTM

comment:102 Changed 17 months ago by Matthias Köppe

Priority: majorcritical

comment:103 Changed 17 months ago by Volker Braun

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