Skip to content

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

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

Closed
strogdon opened this issue Mar 29, 2021 · 107 comments

Comments

@strogdon
Copy link
Contributor

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

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

@strogdon
Copy link
Contributor Author

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 <[email protected]>
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"

@strogdon
Copy link
Contributor Author

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 29, 2021

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.

@strogdon
Copy link
Contributor Author

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

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 29, 2021

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 29, 2021

comment:6

What platform is this, by the way?

@strogdon
Copy link
Contributor Author

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

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 29, 2021

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

@strogdon
Copy link
Contributor Author

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?

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 29, 2021

comment:10

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

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 29, 2021

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.

@strogdon
Copy link
Contributor Author

comment:12

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

@strogdon
Copy link
Contributor Author

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.

@strogdon
Copy link
Contributor Author

comment:14

Ahh, maybe the issue is

NTL_LIBDIR = "/usr/lib"

Shouldn't this be

NTL_LIBDIR = "/usr/lib64"

@strogdon
Copy link
Contributor Author

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 29, 2021

comment:16

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

@kiwifb
Copy link
Member

kiwifb commented Mar 29, 2021

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.

@strogdon
Copy link
Contributor Author

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.

@kiwifb
Copy link
Member

kiwifb commented Mar 30, 2021

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.

@kiwifb
Copy link
Member

kiwifb commented Mar 30, 2021

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

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 30, 2021

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.

@kiwifb
Copy link
Member

kiwifb commented Mar 30, 2021

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.

@kiwifb
Copy link
Member

kiwifb commented Mar 30, 2021

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 30, 2021

comment:25

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

@kiwifb
Copy link
Member

kiwifb commented Mar 30, 2021

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 7, 2021

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 27, 2021

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 27, 2021

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.

@orlitzky
Copy link
Contributor

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.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 27, 2021

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

@mkoeppe
Copy link
Contributor

mkoeppe commented May 27, 2021

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

@mkoeppe
Copy link
Contributor

mkoeppe commented May 27, 2021

comment:92

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

@mkoeppe
Copy link
Contributor

mkoeppe commented May 27, 2021

Changed commit from b81dc85 to 99b5715

@mkoeppe
Copy link
Contributor

mkoeppe commented May 27, 2021

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?

@kiwifb
Copy link
Member

kiwifb commented May 27, 2021

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.

@kiwifb

This comment has been minimized.

@dimpase
Copy link
Member

dimpase commented Jun 9, 2021

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.

@sheerluck
Copy link
Contributor

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?

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 10, 2021

comment:97

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

@strogdon
Copy link
Contributor Author

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

@strogdon
Copy link
Contributor Author

comment:100

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

@strogdon
Copy link
Contributor Author

Changed commit from 99b5715 to a2ab555

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 10, 2021

comment:101

LGTM

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 10, 2021

Reviewer: Matthias Koeppe

@vbraun
Copy link
Member

vbraun commented Jul 23, 2021

Changed branch from u/strogdon/ntl_pkg_config to a2ab555

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

7 participants