Don't ignore "pkg-config --libs-only-other" when building sagelib; and fix linbox.pc

[tobihan]

When sage uses pkgconfig to determine compile flags, for example in src/module_list.py and src/sage/env.py it only extracts the "libraries" but not the "libs" field from pkg-config. In the case of fflas-ffpack this field (as well as cflags) contains -fopenmp. So sage builds but does not link with this flag which leads to missing symbols on Ubuntu:

ImportError: /<<PKGBUILDDIR>>/debian/build/usr/lib/python2.7/dist-packages/sage/matrix/matrix_modn_sparse.so: undefined symbol: GOMP_loop_ull_runtime_start

This was mentioned in https://bugs.debian.org/919573 .

To reproduce:

  tox -e docker-debian-bullseye-standard

The resulting sage crashes on startup with the above error.

Applying the Debian patch reveals another Sage bug:

$ pkg-config --libs linbox
-L/sage/local/lib -llinbox @LINBOXSAGE_LIBS@ -lntl -lmpfr -liml -lflint -fopenmp -lblas -llapack -lgivaro -lgmp -lgmpxx

This is linbox upstream issue linbox-team/linbox#170

This ticket fixes both.

CC: @kiwifb @ClementPernet @antonio-rojas @dimpase @mkoeppe @isuruf @vbraun

Component: build

Author: Tobias Hansen, Matthias Koeppe

Branch: d678f78

Reviewer: Dima Pasechnik

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

[jdemeyer]

comment:1

I think we are either misusing pkgconfig or there is something wrong with pkgconfig upstream.

[tobihan]

comment:2

pkg-config --libs or pkgconfig.libs(pkg) in Python gives the LDFLAGS required for the package. According to the manpage of pkg-config this splits into pkg-config --libs-only-l, pkg-config --libs-only-L and pkg-config --libs-only-other.

What sage does is to get pkg-config --libs-only-l, pkg-config --libs-only-L by means of pkgconfig.parse(pkg)['libraries'] and pkgconfig.parse(pkg)['library_dirs'], missing the content of pkg-config --libs-only-other.

[tobihan]

comment:3

There is extra_link_args to specify the remaining LDFLAGS in distutils extensions.

[tobihan]

comment:4

I created this patch which takes care of the extra flags only for linbox and fflas-ffpack for now:

https://salsa.debian.org/science-team/sagemath/blob/master/debian/patches/u1-pkgconfig-extra-link-flags.patch

To apply it here, should I do the same for all modules that use pkgconfig in env.py and module_list.py?

[kiwifb]

comment:6

Clearly an oversight on my part when I last updated that pkg-config bit. I didn't know about --libs-only-other and thought we would get something like -fopenmp from cflags. I see pkgconfig (the python binding) had a few new releases since I last updated it, but nothing that would help.

I think we should make a feature request for upstream pkgconfig so we can simplify the stuff in your patch. Did you make one?

For the case at hand here in Gentoo I don't have any problems with linking. I am guessing this is because libgomp is in the NEEDED list of libfflas. I am more concerned that -fopenmp in not in CFLAGS where it could have an impact on section of headers being used. At linking it only bring the openmp runtime (libgomp when using gcc) which feels a little bit too late and is only interesting if you have a direct use of openmp symbol or indirect but unresolved ones (which is not the case for me here).

[kiwifb]

comment:7

I have opened matze/pkgconfig#38 for upstream pkgconfig to help deal with this issue. Of course in the meantime the debian patch is OK.

[tobihan]

comment:8

Thanks. On Debian -fopenmp is in the cflags of linbox and fflas-ffpack. That's precisely why this was discovered: If you build with this flag you also need to link with it. The build failed on Ubuntu with a missing symbol from libgomp. For some reason that I don't know the build didn't fail on Debian.

[kiwifb]

comment:9

Well, I have been giving this some thoughts and I have come to the conclusion that you cannot have that problem unless either libfflas.so or libffpack.so is underlinked relative to libgomp. linbox, while it does have a openmp configure option, doesn't link to the openmp runtime as far as I can see. So the trouble come from fflas-ffpack.

[kiwifb]

comment:10

After closer inspection it could also happen if you use openblas compiled with openmp and it is underlinked. I guess that's another possibility, and in that case adding fflas-ffpack's flag to fix it, is just accidental.

[embray]

comment:12

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

[embray]

comment:13

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).

[embray]

comment:14

I have also been seeing problems like this on Cygwin lately, though I'm not sure where it began.

I've been compiling fflas-ffpack with FFLAS_FFPACK_CONFIGURE=--disable-openmp.

[mkoeppe]

comment:16

I think this is showing up in https://github.com/mkoeppe/sage/runs/581024689?check_suite_focus=true too

[dimpase]

comment:18

this seems to be related to OpenMP:

2020-04-12T20:04:09.1360983Z   [dochtml]   ImportError: /sage/local/lib/python3.7/site-packages/sage/matrix/matrix_modn_sparse.cpython-37m-x86_64-linux-gnu.so: undefined symbol: GOMP_loop_ull_maybe_nonmonotonic_runtime_next

[isuruf]

comment:20

Looks like fflas-ffpack have openmp pragmas on their header files. Sage should detect whether __FFLASFFPACK_USE_OPENMP is defined in spkg-configure.m4 and add -fopenmp to extra_link_args until the pkgconfig issue is fixed

[mkoeppe]

Branch: u/mkoeppe/sage_ignores_libs_from_pkgconfig__also_linbox_pc_broken_by_a_sage_patch

[mkoeppe]

Commit: 257f057

[sagetrac-git]

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

79a6577

backport patch

[sagetrac-git]

Changed commit from 257f057 to 79a6577

[mkoeppe]

comment:27

Fixes the problem on debian-bullseye-standard.

[dimpase]

comment:28

good catch. needs testing.

by the way, is there a ticket to make gh action testing doable from CLI?

[mkoeppe]

comment:29

Tests running at https://github.com/mkoeppe/sage/actions/runs/80516747

[mkoeppe]

comment:30

Replying to @dimpase:

by the way, is there a ticket to make gh action testing doable from CLI?

No ticket; but from a comment in #29087:
gh pr create -f -d would be a command line interface to trigger the GH actions run. ​https://cli.github.com/manual/gh_pr_create

[mkoeppe]

comment:31

The tests are ready; needs review

[mkoeppe]

comment:32

Let's get this into 9.1 please...

[dimpase]

comment:33

lgtm, thanks.

[dimpase]

Reviewer: Dima Pasechnik

[mkoeppe]

comment:34

Thanks!

[vbraun]

comment:35

See patchbot:

[sagelib-9.1.rc0] g++: error: LINBOXSAGE_LIBS@: No such file or directory

[mkoeppe]

comment:36

Should have bumped the patch level

[sagetrac-git]

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

d678f78

build/pkgs/linbox/package-version.txt: Bump up the patchlevel so that incremental builds of sagelib do not fail

[sagetrac-git]

Changed commit from 79a6577 to d678f78

[vbraun]

Changed branch from u/mkoeppe/sage_ignores_libs_from_pkgconfig__also_linbox_pc_broken_by_a_sage_patch to d678f78

[kliem]

comment:40

Does this ticket fix the following error when building the documentation?

 [dochtml] installing. Log file: logs/dochtml.log
  [dochtml] error installing, exit status 1. End of log file:
  [dochtml]   Traceback (most recent call last):
  [dochtml]     File "/sage/local/lib/python3.7/runpy.py", line 183, in _run_module_as_main
  [dochtml]       mod_name, mod_spec, code = _get_module_details(mod_name, _Error)
  [dochtml]     File "/sage/local/lib/python3.7/runpy.py", line 142, in _get_module_details
  [dochtml]       return _get_module_details(pkg_main_name, error)
  [dochtml]     File "/sage/local/lib/python3.7/runpy.py", line 109, in _get_module_details
  [dochtml]       __import__(pkg_name)
  [dochtml]     File "/sage/local/lib/python3.7/site-packages/sage_setup/docbuild/__init__.py", line 59, in <module>
  [dochtml]       import sage.all
  [dochtml]     File "/sage/local/lib/python3.7/site-packages/sage/all.py", line 132, in <module>
  [dochtml]       from sage.matrix.all     import *
  [dochtml]     File "/sage/local/lib/python3.7/site-packages/sage/matrix/__init__.py", line 2, in <module>
  [dochtml]       import sage.matrix.args
  [dochtml]     File "sage/matrix/args.pyx", line 23, in init sage.matrix.args (build/cythonized/sage/matrix/args.c:21217)
  [dochtml]     File "/sage/local/lib/python3.7/site-packages/sage/matrix/matrix_space.py", line 46, in <module>
  [dochtml]       from . import matrix_modn_sparse
  [dochtml]   ImportError: /sage/local/lib/python3.7/site-packages/sage/matrix/matrix_modn_sparse.cpython-37m-x86_64-linux-gnu.so: undefined symbol: GOMP_loop_ull_maybe_nonmonotonic_runtime_next
  [dochtml] Full log file: logs/dochtml.log

E.g. on ubuntu-focal or debian bullseye: https://github.com/mkoeppe/sage/runs/610674634

This is a new bug that appears to have been introduced in 9.1.rc1.

[kliem]

Changed commit from d678f78 to none

[mkoeppe]

comment:41

Yes, this ticket fixes this bug.