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

Question

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

Closed this issue 4 years ago · 107 comments

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

libntl/ntl#7

CC: @kiwifb @mkoeppe @orlitzky @dimpase @isuruf

Component: doctest framework

Author: Steven Trogdon

Branch/Commit: a2ab555

Reviewer: Matthias Koeppe

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

Answer 1 · 2021-03-29T17:52:00.000Z

comment:1

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"

Answer 2 · 2021-03-29T18:09:41.000Z

comment:2

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

Answer 3 · 2021-03-29T18:47:19.000Z

comment:3

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.

Answer 4 · 2021-03-29T18:53:49.000Z

comment:4

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"]=""

Answer 5 · 2021-03-29T18:59:20.000Z

comment:5

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.

Answer 6 · 2021-03-29T19:06:24.000Z

comment:6

What platform is this, by the way?

Answer 7 · 2021-03-29T19:17:32.000Z

comment:7

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

Answer 8 · 2021-03-29T19:25:17.000Z

comment:8

The NTL variables in sage_conf were introduced in 6e30e3a 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

Answer 9 · 2021-03-29T19:58:06.000Z

comment:9

Replying to @mkoeppe:

The NTL variables in sage_conf were introduced in 6e30e3a 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?

Answer 10 · 2021-03-29T20:06:44.000Z

comment:10

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

Answer 11 · 2021-03-29T20:11:53.000Z

comment:11

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.

Answer 12 · 2021-03-29T20:20:46.000Z

comment:12

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

Answer 13 · 2021-03-29T20:28:07.000Z

comment:13

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.

Answer 14 · 2021-03-29T20:31:27.000Z

comment:14

Ahh, maybe the issue is

NTL_LIBDIR = "/usr/lib"

Shouldn't this be

NTL_LIBDIR = "/usr/lib64"

Answer 15 · 2021-03-29T20:32:49.000Z

comment:15

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.

Answer 16 · 2021-03-29T22:41:53.000Z

comment:16

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

Answer 17 · 2021-03-29T23:59:03.000Z

comment:18

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.

Answer 18 · 2021-03-30T03:22:55.000Z

comment:19

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.

Answer 19 · 2021-03-30T22:09:33.000Z

comment:20

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.

Answer 20 · 2021-03-30T22:20:25.000Z

comment:21

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

Answer 21 · 2021-03-30T22:20:42.000Z

comment:22

Replying to @kiwifb:

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.

Answer 22 · 2021-03-30T22:24:23.000Z

comment:23

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.

Answer 23 · 2021-03-30T22:46:22.000Z

comment:24

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.

Answer 24 · 2021-03-30T22:50:43.000Z

comment:25

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

Answer 25 · 2021-03-30T23:48:28.000Z

comment:26

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.

Answer 26 · 2021-04-07T19:29:15.000Z

comment:27

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

Answer 27 · 2021-04-16T10:20:18.000Z

comment:29

Replying to @kiwifb:

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://github.com/sagemath/sagetrac-mirror/blob/develop/m4/sage_absolute_lib.m4?id=2d4a1cdac919203e68f0d56174cd6257f600fe00

Answer 28 · 2021-04-16T10:58:27.000Z

comment:30

Replying to @mkoeppe:

Replying to @kiwifb:

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?

Answer 29 · 2021-04-16T11:02:38.000Z

comment:31

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.

Answer 30 · 2021-04-16T14:10:22.000Z

comment:32

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)

Answer 31 · 2021-04-16T16:22:07.000Z

comment:33

Replying to @dimpase:

well, on #28906 I wrote something akin to AX_ABSOLUTE_LIBRARY, see
https://github.com/sagemath/sagetrac-mirror/blob/develop/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.

Answer 32 · 2021-04-16T16:48:28.000Z

comment:34

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

Answer 33 · 2021-04-16T20:08:41.000Z

comment:35

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

Answer 34 · 2021-04-16T20:15:39.000Z

comment:36

Replying to @mkoeppe:

Replying to @dimpase:

well, on #28906 I wrote something akin to AX_ABSOLUTE_LIBRARY, see
https://github.com/sagemath/sagetrac-mirror/blob/develop/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?

Answer 35 · 2021-04-16T20:15:56.000Z

comment:37

Most pkgconfig files do provide variables such as libdir.

Answer 36 · 2021-04-16T20:17:19.000Z

comment:38

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.

Answer 37 · 2021-04-16T20:17:40.000Z

comment:39

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()?

Answer 38 · 2021-04-16T20:18:20.000Z

comment:40

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

Answer 39 · 2021-04-16T20:19:00.000Z

comment:41

Replying to @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()?

Yes, precisely. (I know...)

Answer 40 · 2021-04-16T20:21:02.000Z

comment:42

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.

Answer 41 · 2021-04-16T20:29:53.000Z

comment:43

Yes, one package at a time.

Answer 42 · 2021-04-16T20:31:24.000Z

comment:44

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.

Answer 43 · 2021-04-16T20:34:18.000Z

comment:45

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.

Answer 44 · 2021-04-17T15:58:54.000Z

comment:46

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

Answer 45 · 2021-04-17T16:53:02.000Z

comment:47

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

Answer 46 · 2021-04-18T09:44:41.000Z

comment:48

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

Answer 47 · 2021-04-18T09:51:55.000Z

comment:49

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.

Answer 48 · 2021-04-20T01:30:08.000Z

comment:50

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.

Answer 49 · 2021-04-20T10:19:48.000Z

comment:51

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

Answer 50 · 2021-04-20T17:47:33.000Z

comment:52

Replying to @kiwifb:

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

Answer 51 · 2021-04-20T19:41:13.000Z

comment:53

Replying to @strogdon:

Replying to @kiwifb:

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.

Answer 52 · 2021-04-20T20:00:14.000Z

comment:54

Replying to @kiwifb:

Replying to @strogdon:
Replying to @kiwifb:

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:

Answer 53 · 2021-04-20T20:02:37.000Z

comment:55

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.

Answer 54 · 2021-04-20T20:13:20.000Z

comment:56

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

Answer 55 · 2021-04-20T20:14:44.000Z

comment:57

I have pushed on github though.

Answer 56 · 2021-04-21T02:31:31.000Z

Attachment: ntl-pc.patch.gz

Patch to ntl to create a ntl.pc file

Answer 57 · 2021-04-21T02:31:55.000Z

comment:58

Patch updated.

Answer 58 · 2021-05-02T19:19:58.000Z

comment:59

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.

Answer 59 · 2021-05-02T19:43:40.000Z

comment:60

Replying to @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!

Answer 60 · 2021-05-02T21:28:08.000Z

comment:61

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.

Answer 61 · 2021-05-03T01:42:12.000Z

comment:62

Replying to @kiwifb:

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.

Answer 62 · 2021-05-18T07:26:49.000Z

comment:63

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

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

Answer 63 · 2021-05-18T08:28:35.000Z

comment:64

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)

Answer 64 · 2021-05-18T08:37:26.000Z

comment:65

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.

Answer 65 · 2021-05-18T08:41:15.000Z

comment:66

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.

Answer 66 · 2021-05-21T05:19:29.000Z

comment:67

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

diff --git a/build/pkgs/ntl/spkg-configure.m4 b/build/pkgs/ntl/spkg-configure.m4
index 48e41de6ea..d069a798de 100644
--- a/build/pkgs/ntl/spkg-configure.m4
+++ b/build/pkgs/ntl/spkg-configure.m4
@@ -44,7 +44,13 @@ SAGE_SPKG_CONFIGURE([ntl], [
         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])
+        ntl_libdir=`pkg-config --variable=libdir ntl`
+        if test x$ntl_libdir = x; then
+            AC_SUBST(NTL_LIBDIR, [$ntl_prefix/lib])
+        else
+            libdir=`basename $ntl_libdir`
+            AC_SUBST(NTL_LIBDIR, [$ntl_prefix/$libdir])
+        fi
     fi
 ])

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.

Answer 67 · 2021-05-21T08:19:58.000Z

comment:68

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

Answer 68 · 2021-05-22T06:08:58.000Z

comment:69

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:

diff --git a/build/pkgs/ntl/spkg-configure.m4 b/build/pkgs/ntl/spkg-configure.m4
index 48e41de6ea..5f83cfaac4 100644
--- a/build/pkgs/ntl/spkg-configure.m4
+++ b/build/pkgs/ntl/spkg-configure.m4
@@ -44,7 +44,10 @@ SAGE_SPKG_CONFIGURE([ntl], [
         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])
+        PKG_CHECK_VAR([ntl_libdir], [ntl], [libdir])
+        AS_IF([test "x$ntl_libdir" = x], [AC_SUBST(NTL_LIBDIR, [$ntl_prefix/lib])],
+             [test "x$ntl_libdir" != x], [AC_SUBST(NTL_LIBDIR, [$ntl_prefix/[$(basename $ntl_libdir)]])]
+            )
     fi
 ])

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

Answer 69 · 2021-05-22T10:59:57.000Z

comment:70

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?

Answer 70 · 2021-05-22T16:44:18.000Z

Branch: u/strogdon/ntl_pkg_config

Answer 71 · 2021-05-22T16:49:29.000Z

comment:72

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:

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

Answer 72 · 2021-05-22T16:49:29.000Z

Author: Steven Trogdon

Answer 73 · 2021-05-22T16:49:29.000Z

Commit: a265a71

Answer 74 · 2021-05-24T21:51:29.000Z

comment:73

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

Answer 75 · 2021-05-24T23:30:42.000Z

comment:74

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

Answer 76 · 2021-05-25T10:48:20.000Z

comment:75

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

Answer 77 · 2021-05-25T15:16:52.000Z

Changed commit from a265a71 to b81dc85

Answer 78 · 2021-05-25T15:16:52.000Z

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

`b81dc85`	`Remove extra // in definition of NTL_LIBDIR`

Answer 79 · 2021-05-27T10:56:08.000Z

comment:77

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

Answer 80 · 2021-05-27T10:59:08.000Z

comment:78

Replying to @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?

Answer 81 · 2021-05-27T11:04:57.000Z

comment:79

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.

Answer 82 · 2021-05-27T11:12:19.000Z

comment:80

Replying to @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.

Answer 83 · 2021-05-27T15:32:22.000Z

comment:81

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.

Answer 84 · 2021-05-27T17:03:40.000Z

comment:82

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?

Answer 85 · 2021-05-27T17:11:19.000Z

comment:83

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.

Answer 86 · 2021-05-27T17:33:49.000Z

comment:84

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.

Answer 87 · 2021-05-27T18:09:18.000Z

comment:85

Replying to @orlitzky:

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)

Answer 88 · 2021-05-27T18:18:03.000Z

comment:86

Replying to @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).

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.

Answer 89 · 2021-05-27T18:49:16.000Z

comment:87

Replying to @orlitzky:

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.

Answer 90 · 2021-05-27T18:53:48.000Z

comment:88

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.

Answer 91 · 2021-05-27T19:16:18.000Z

comment:89

Replying to @mkoeppe:

Replying to @orlitzky:

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.

Answer 92 · 2021-05-27T19:26:42.000Z

comment:90

Replying to @kiwifb:

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

Answer 93 · 2021-05-27T19:36:03.000Z

Changed branch from u/strogdon/ntl_pkg_config to u/mkoeppe/ntl_pkg_config

Answer 94 · 2021-05-27T19:36:43.000Z

comment:92

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

New commits:

`99b5715`	`src/sage/rings/padics/padic_printing.pyx: Fix up distutils directive`

Answer 95 · 2021-05-27T19:36:43.000Z

Changed commit from b81dc85 to 99b5715

Answer 96 · 2021-05-27T19:37:07.000Z

comment:93

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?

Answer 97 · 2021-05-27T22:08:41.000Z

comment:94

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.

Answer 98 · 2021-05-27T22:08:41.000Z

Description changed:

--- 
+++ 
@@ -20,3 +20,6 @@
     /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
+* [https://github.com/libntl/ntl/pull/7](https://github.com/libntl/ntl/pull/7)

Answer 99 · 2021-06-09T14:56:41.000Z

comment:95

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.

Answer 100 · 2021-07-10T07:18:08.000Z

comment:96

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?