Pkgconfig and dependency handling improvements #3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OpenMP is not used in m4rie, and its flags are not needed to build m4rie against an m4ri that _does_ have OpenMP support. The one scenario where m4ri/m4rie might need to agree on the matter is when linking a static executable that uses them both; but for this, it will be easier to rely on pkg-config to provide the correct linker flags (and only when necessary).
Now that m4ri has a pkg-config file, we can use it to automatically obtain the correct preprocessor/linker flags for m4ri, rather than detecting and including them ourselves.
Now that m4ri has a pkg-config file, we can use it to obtain $M4RI_CFLAGS and $M4RI_LIBS in configure.ac rather than using the custom macro AX_M4RI_CFLAGS() from m4/ax_m4ri_flags.m4. This commit replaces AX_M4RI_CFLAGS() with PKG_CHECK_MODULES(), leaves $M4RI_CFLAGS alone, and replaces -lm4ri with $M4RI_LIBS throughout the build system. The old macro defined only $M4RI_CFLAGS, but now we get $M4RI_LIBS separately.
|
I don't think we currently require |
|
On 2025-01-22 00:39:04, Martin R. Albrecht wrote:
I don't think we currently require `pkg-config`, but use it if it's there. True, people need to figure it out themselves what flags to use, but currently they can.
Ok, I will take another shot at this in the next few days. I was
inspired to replace the AX_M4RI_CFLAGS() macro because it has the same
problem that m4ri's pc file used to, namely that it includes some
flags that don't really belong there.
It should still be possible to support "normal" systems that are
missing pkg-config however if we do a link test and if -lm4ri works.
|
|
Thank you! |
Now if pkg-config fails to find m4ri, we will try to link against the system copy with -lm4ri. It is only an error if both methods fail (and if --with-m4ri was not given).
The three methods we have for detecting m4ri, 1. By fiat using --with-m4ri 2. pkg-config 3. AC_SEARCH_LIBS now all use the same two variables to store the necessary preprocessor and linker flags, $M4RI_CFLAGS and $M4RI_LIBS (these are the pkg-config names, don't blame me). This obsoletes the M4RIE_M4RI_CFLAGS and M4RIE_M4RI_LDFLAGS variables, which otherwise would have been used in combination with M4RI_CFLAGS and M4RI_LIBS, and could have lead to confusion.
This is redundant at the moment because AC_SEARCH_LIBS() already prepends -lm to $LIBS, but m4ri.pc includes -lm and m4ri.h includes math.h, so it's probably best to explicitly include -lm when linking m4ri.
Only one file (bench/benchmarking.c) includes math.h directly. Others include it indirectly via m4ri headers, but those cases should be covered by $M4RI_LIBS.
|
Further improvements:
|
|
Looks good to me, thank you! |
|
On 2025-01-25 04:23:18, Martin R. Albrecht wrote:
Looks good to me, thank you!
No problem, thank you too.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As a follow-up to,
we can make some similar improvements in m4rie and simplify its detection of m4ri:
OPENMP_FLAGSentirely. This does not appear to be used by m4rie, and it isn't needed to use an m4ri that was built with OpenMP support.Requiresin m4rie's pkg-config file rather than explicitly appending m4ri's flags to those of m4rie. TheRequiresline is designed to handle this situation automatically. Here's what it does on my (patched) system:--with-m4ri=<path>was not specified). This makes pkg-config a requirement, but it kind of is anyway. If someone wants to link against m4rie, they should be using pkg-config to get the correct flags because otherwise it's not obvious that-lm4rineeds to go there. And at that point, not much is lost by also requiring pkg-config to build m4rie in the first place.CC: @kiwifb, @dimpase, @tornaria, @arojas, @isuruf