Skip to content
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

Use pkg-config in Autotools #2099

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

albinahlback
Copy link
Collaborator

This commit treats the following:

  • Detects if pkg-config or pkgconf is available
  • Uses pkg-config to set the correct flags for dependencies. In particular, this solves issues when compiling against static libraries.
  • Cleans up configure.ac
  • Sets LDFLAGS, CFLAGS, CPPFLAGS and LIBS correctly and trims their corresponding string
  • Actually pushes -lgc when Boehr GC is being configured for. Has gone unnoticed.

Solves #2097

Ping @jaganmn, please give your feedback. Now pkg-config is the default. The changes of interests are really in acinclude.m4.

Sidenote: I don't know if people typically use CBLAS or OpenBLAS, and if we should have an option where you choose which one to pick.

@albinahlback
Copy link
Collaborator Author

Hmm, it doesn't seem to like me. Will fix the bugs some other day, but the idea still stands.

@jaganmn
Copy link

jaganmn commented Oct 21, 2024

Thanks. I'll take a closer look tomorrow.

@jaganmn
Copy link

jaganmn commented Oct 21, 2024

But at first glance, the PR must be incomplete, because flint.pc.in is not modified to ensure that pkg-config --static --libs flint contains options necessary and sufficient for statically linking FLINT. As written, the output will contain -l options -lflint, -lmpfr, and -lgmp but never, e.g., -lcblas even if FLINT was configured with BLAS support.

This commit treats the following:

- Detects if pkg-config or pkgconf is available

- Uses pkg-config to set the correct flags for dependencies. In
  particular, this solves issues when compiling against static
  libraries.

- Cleans up configure.ac

- Sets LDFLAGS, CFLAGS, CPPFLAGS and LIBS correctly and trims their
  corresponding string

- Now pushes -lgc if GC is used. Seems to have gone unnoticed.

- Pushes LIBS into flint.pc
@albinahlback
Copy link
Collaborator Author

But at first glance, the PR must be incomplete, because flint.pc.in is not modified to ensure that pkg-config --static --libs flint contains options necessary and sufficient for statically linking FLINT. As written, the output will contain -l options -lflint, -lmpfr, and -lgmp but never, e.g., -lcblas even if FLINT was configured with BLAS support.

I believe this part should be fixed now.

@jaganmn
Copy link

jaganmn commented Oct 21, 2024

A few comments for now:

  • You should read this very short guide to writing correct .pc files. In principle, you should be using all of the fields Cflags, Requires, Requires.private, Libs, and Libs.private. You should not be putting all of LIBS into Libs, and you should not assume that all of the headers and libraries are located in includedir and libdir. Different directories could have been specified by the user on the configure command line or by pkg-config, and those should be reflected in the generated flint.pc.
  • The generated flint.pc does not specify -lgc or -lntl even when configuring with GC and NTL support. If you actually link against them (as opposed to just using their headers), then flint.pc should indicate that.
  • Linkers are sensitive to the order of -l options on the command line. If library x.a resolves symbols in library y.a, then you want -ly to appear before -lx on the linker command line, as well as in the Libs.private field of flint.pc. You can assume that the output of pkg-config --static --libs contains -l options in the correct order (if that assumption is wrong, then there is a bug in your dependency). In any case, sorting LIBS with sort is wrong. Sorting of CPPFLAGS and LDFLAGS can also affect search path order. If the scenario of duplicate options causing too long command lines is hypothetical rather than real, then maybe the sorting/deduplicating can be skipped for now.

@albinahlback
Copy link
Collaborator Author

  • You should read this very short guide to writing correct .pc files. In principle, you should be using all of the fields Cflags, Requires, Requires.private, Libs, and Libs.private.

Thanks. Why separate Libs and Libs.private though?

You should not be putting all of LIBS into Libs, and you should not assume that all of the headers and libraries are located in includedir and libdir. Different directories could have been specified by the user on the configure command line or by pkg-config, and those should be reflected in the generated flint.pc.

Suppose one uses Boehm GC and BLAS (are used "privately", I suppose), and that they are in other directories compared to FLINT's prefix. Could you give a simple example of how the pkg-config file would look like, in you opinion?

  • The generated flint.pc does not specify -lgc or -lntl even when configuring with GC and NTL support. If you actually link against them (as opposed to just using their headers), then flint.pc should indicate that.

Yes, I believe GC is incorrect in its current format. However, the NTL support is header-only -- should I still push it into flint.pc?

  • Linkers are sensitive to the order of -l options on the command line. If library x.a resolves symbols in library y.a, then you want -ly to appear before -lx on the linker command line, as well as in the Libs.private field of flint.pc.

Alright, I'll fix this (I have never gotten this problem with any mainline linker, though).

Sorting of CPPFLAGS and LDFLAGS can also affect search path order. If the scenario of duplicate options causing too long command lines is hypothetical rather than real, then maybe the sorting/deduplicating can be skipped for now.

I mainly want to remove duplicate flags (suppose MPFR and GMP are in the same directory) and strip whitespaces as it is easier to look at the build/configure results with human eyes. The hierarchy for CPPFLAGS and LDFLAGS should be the same as for LIBS, correct?

@albinahlback
Copy link
Collaborator Author

Thanks for the help, by the way! I appreciate it!

@jaganmn
Copy link

jaganmn commented Oct 22, 2024

Your flint.pc could contain something like:

Cflags: -I${includedir} @FLINT_PC_CFLAGS@
Libs: -L${libdir} -lflint
Libs.private: @FLINT_PC_LIBS_PRIVATE@
Requires:
Requires.private: @FLINT_PC_REQUIRES_PRIVATE@

Then the basic idea for each dependency xyz would be:

  • If configure finds pkg-config and pkg-config finds xyz.pc, then append xyz, or xyz <op> <ver>, to FLINT_PC_REQUIRES_PRIVATE (Requires.private is a comma-separated list), distribute the output of pkg-config --cflags xyz to CPPFLAGS and CFLAGS, and distribute the output of pkg-config --libs xyz (with or without --static depending on the outcome of linking tests) to LDFLAGS and LIBS. You can use pkg-config --*-only-* to help with partitioning the options. Don't touch FLINT_PC_CFLAGS or FLINT_PC_LIBS_PRIVATE.
  • If configure does not find pkg-config or pkg-config does not find xyz.pc, then append options for the C preprocessor or compiler to FLINT_PC_CFLAGS, CPPFLAGS, and CFLAGS, and append options for the linker to FLINT_PC_LIBS_PRIVATE, LDFLAGS, and LIBS. Don't touch FLINT_PC_REQUIRES_PRIVATE. configure must determine the necessary and sufficient options with suitable compilation and linking tests.
  • If you only use xyz.h and don't link libxyz, then there is no need to touch FLINT_PC_LIBS_PRIVATE, LDFLAGS, and LIBS.

Depending on the order in which configure processes dependencies, it can sometimes be better to "prepend", e.g., if that will ensure that -lmpfr appears before -lgmp. As far as ordering goes, I wouldn't worry too much outside of the -l options. Be aware that the default behaviour of some Autoconf macros (notably AC_CHECK_LIB) is to prepend options to LIBS. You may want to override that and do it manually for more control.

Leave Requires empty and Libs empty except for -L${libdir} -lflint, so that pkg-config --libs flint excludes linker options for your dependencies and pkg-confic --static --libs flint includes them. That's generally what you want: if the user linking libflint dynamically actually needs symbols from libmpfr or libgmp, then they can arrange to add -lmpfr and/or -lgmp themselves.

On the other hand, moving mpfr and gmp from Requires (where you've had them in 3.1.3) to *.private could break code expecting that the output of pkg-config --libs flint contains -lmpfr -lgmp. That could be an argument for continuing with Requires: mpfr, gmp and using *.private otherwise.

@jaganmn
Copy link

jaganmn commented Oct 23, 2024

Re: deduplication

I decided to take this approach in my R package's configure.ac when pkg-config is available: https://github.com/jaganmn/flint/blob/7c9e775eb35d478976f48db9628a1227c7be2296/configure.ac#L112-L127

It is definitely a hack but it seems more robust than sorting the options or filtering them "manually" by other means. I agree that it is nice to have readable CPPFLAGS, etc., if possible ...

@dimpase
Copy link
Contributor

dimpase commented Nov 5, 2024

This looks suboptimal. pkg-config comes with its own autoconf macros, why aren't they used?

@albinahlback
Copy link
Collaborator Author

This looks suboptimal.

Exactly what looks suboptimal?

pkg-config comes with its own autoconf macros, why aren't they used?

Are you referring to using their macros instead of the new changes in acinclude.m4? To be clear, I want to separate CPPFLAGS from CFLAGS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants