flintlib / flint

FLINT (Fast Library for Number Theory)
http://www.flintlib.org
GNU Lesser General Public License v3.0
445 stars 244 forks source link

Use pkg-config to detect flags for statically linking dependencies (notably BLAS, GC, NTL) #2097

Open jaganmn opened 1 month ago

jaganmn commented 1 month ago

And substitute those flags in flint.pc so that software wanting to statically link FLINT can know how to do so. This means that configure.ac should, if it finds pkg-config, test linking first with pkg-config --libs <name> and (only if that fails) again with pkg-config --static --libs <name>. The output would typically contain both -L and -l options, so it could override user options --with-*-lib not designed for static linking. In that case, pkg-config --cflags <name> should probably override --with-*-include.

You could use --libs-only-L, --libs-only-l, and --libs-only-other to separate the flags if that simplifies the Autoconf implementation.

albinahlback commented 1 month ago

I'm afraid I'm not sure what you mean. What is the problem, and how does this solve the issue?

jaganmn commented 1 month ago

Linking against a static CBLAS can require both -lcblas and -lblas, and linking against a static GC can require both -lgc and -latomic_ops. For example, we can have a static library cblas.a in our library search path, but your

AC_SEARCH_LIBS([cblas_dgemm], [cblas openblas blas], [], ...)

can fail to "find" it because it passes -lcblas to the linker instead of -lcblas -lblas. The linker fails due to undefined symbols, so configure passes over a perfectly usable CBLAS installation. pkg-config --static --libs cblas would look at cblas.pc and report that statically linking against CBLAS requires -lcblas -lblas, not simply -lcblas.

This matters at least to CRAN which links libraries statically to simplify distribution of R package binaries to Windows and macOS users, e.g., so that they can use an R interface to FLINT without needing to have FLINT, MPFR, GMP, CBLAS, ... installed on their systems.

albinahlback commented 1 month ago

I see, thanks for the explanation. We could fix this, but I don't believe pkg-config should be a requirement for installing FLINT.

I believe these cases are very limited (correct me if I'm wrong), so I would propose using the fifth argument in AC_SEARCH_LIBS (see documentation) instead.

jaganmn commented 1 month ago

One would typically condition the use of pkg-config on the result of AC_CHECK_PROG, perhaps falling back to the current behaviour on systems where pkg-config is not available.

Using the fifth argument of AC_CHECK_LIB or AC_SEARCH_LIBS can be fragile because you are guessing at what is needed (which could change between versions/configurations of the library that you want to link), instead of consulting the .pc file containing the necessary and sufficient command line options. (Well, .pc files are often not written correctly, but that would be "their" bug.)

jaganmn commented 1 month ago

Of course I can empathize with reluctance to add complexity to configure.ac. And I can imagine a situation where it would make sense to just do AC_CHECK_LIB with and without the fifth argument, because you are very confident about the necessary and sufficient additional options (perhaps because they reflect very stable, non-optional dependencies such as MPFR's dependency on GMP).

But really pkg-config was designed to avoid such assumptions and in my experience it is really worthwhile (for maintainers and users) to take advantage of it.

albinahlback commented 1 month ago

Can you give any examples of libraries that use pkg-config this way in Autoconf?

jaganmn commented 1 month ago

Yes, you can get inspiration from mature projects such as R, curl, and ghostscript. R in particular is very careful to test linking with each of --libs and --static --libs in turn. Much of that logic appears not in its configure.ac but in its m4/R.m4 here. Search for AC_DEFUN([R_BITMAPS] and AC_DEFUN([R_BITMAPS2] and compare the code under each.

albinahlback commented 1 month ago

Thanks, I will look into it!

albinahlback commented 1 month ago

I'm on board with this, and I believe it should be an easy fix. Will try to fix it before next release.