Enable static builds with pkg-config integration. #288

jptrainor · 2025-12-15T23:10:49Z

This PR fixes static build correctness across CMake and pkg-config.

Static build issues surfaced while implementing static linking using pkg-config for an ffmpeg/MXL integration. MXL supported shared-library builds only, ignored BUILD_SHARED_LIBS, and provided incomplete pkg-config metadata for static linkage.

Key build-related changes:

Correct handling of BUILD_SHARED_LIBS=ON|OFF across mxl targets
Added MXL_ENABLE_PIC (default ON) to support static builds used in shared contexts
Fixed pkg-config metadata for static linking
Corrected CMake export-set dependency relationships

This change adds a C consumer link test to prevent regressions.

Tested with:

Linux (GCC, Clang)
macOS (Clang/x86_64)
Shared and static builds (BUILD_SHARED_LIBS=ON|OFF)
Debug and release builds
Static ffmpeg build as downstream consumer on Linux

The ffmpeg integration lives in a downstream fork and is still being
finalized, but is included here for reference:
https://github.com/cbcrc/FFmpeg/tree/dmf-mxl/master

vt-tv

Looks good to me. Thanks for your contribution!

vt-tv · 2025-12-16T01:58:53Z

You are mising the SPDX headers/comments in these files :

lib/tests/test_pkgconfig_c_link.cpp
lib/tests/test_pkgconfig_c_link.sh

vt-tv

See comments about SPDX

jonasohland

For consistency, the same changes need to be applied to targets in lib/fabrics/ofi

CMakeLists.txt

KimonHoffmann

Thank you very much for this contribution and filling in the missing bits for the static library build. That has been long on my TODO list and I very much appreciate being able to tick this off now.

Regarding the pkg-config compile check I'm a bit conflicted. I think it's a good idea, but having it as part of the tests on the side feels wrong to me. Could you please transform this into a post-build step that is independent of the other tests?

Also regarding the C++ parts of this PR please try to match the style present in other tests with regards to pointer types, right hand types and explicit qualification of entities in the global namespace.

CMakeLists.txt

lib/tests/test_pkgconfig_c_link.sh

jptrainor · 2025-12-16T15:43:49Z

You are mising the SPDX headers/comments in these files :

lib/tests/test_pkgconfig_c_link.cpp lib/tests/test_pkgconfig_c_link.sh

Resolved

vt-tv

clang-tidy issues introcued by the spdx changes.

jptrainor · 2025-12-16T19:16:02Z

Thank you very much for this contribution and filling in the missing bits for the static library build. That has been long on my TODO list and I very much appreciate being able to tick this off now.

Regarding the pkg-config compile check I'm a bit conflicted. I think it's a good idea, but having it as part of the tests on the side feels wrong to me. Could you please transform this into a post-build step that is independent of the other tests?

Also regarding the C++ parts of this PR please try to match the style present in other tests with regards to pointer types, right hand types and explicit qualification of entities in the global namespace.

I moved the pkg-config consumer test out of Catch2, removed the cpp wrapper, and added a standalone CTest test (via add_test()) that executes the test shell script directly. It is cleaner this way. Does that address your "independent of other tests" concern?

jptrainor · 2025-12-16T21:21:06Z

clang-tidy issues introcued by the spdx changes.

I removed the cpp wrapper that would have produced that. So it's now a non-issue I believe.

jonasohland

It would be nice if we could have the test script also test linking the fabrics API. But fabrics is not in scope for v1.0.0, so I will just add that later.

jptrainor · 2025-12-17T18:24:44Z

It would be nice if we could have the test script also test linking the fabrics API. But fabrics is not in scope for v1.0.0, so I will just add that later.

I can do it. I looked at the fabric code and noticed that it was not documented yet and not enabled so I was uncertain. It's not a problem for me to add it to the test script so I'll do that. Standby for that update.

jptrainor · 2025-12-17T18:25:25Z

It would be nice if we could have the test script also test linking the fabrics API. But fabrics is not in scope for v1.0.0, so I will just add that later.

Or if this goes in first then I'll do it a followup PR.

pedro-alves-ferreira

LGTM

KimonHoffmann · 2025-12-18T14:13:11Z

Thank you for making the changes regarding position independent code. Could you please have a look at my suggestion regarding the linker invocation above?

KimonHoffmann

Dummy comment to set the status of my review to to "request changes".

- Respect BUILD_SHARED_LIBS for mxl, mxl-common, and mxl-fabrics - Add MXL_ENABLE_PIC option (default ON) for static builds - Fix CMake export set dependencies involving mxl-internal-headers - Update libmxl.pc to correctly express static linking requirements - Add test to validate pkg-config C consumer linking (static and shared) - Add "CMAKE_LINKER_TYPE": "LLD" to the clang preset to resolve LTO linking in the test. Signed-off-by: jpt <jim.trainor@jptrainor.com>

vt-tv · 2025-12-22T22:34:34Z

@jptrainor Is the request for changes from Kimon adressed?

jptrainor · 2025-12-22T22:39:14Z

@jptrainor Is the request for changes from Kimon adressed?

Yes

vt-tv · 2025-12-22T22:40:57Z

@jptrainor Is the request for changes from Kimon adressed?

Yes

Ready to merge (with rebase)? if so I'll do it now.

jptrainor force-pushed the main branch from 8d43c7e to 93f9f5d Compare December 15, 2025 23:13

vt-tv requested review from KimonHoffmann and jonasohland December 16, 2025 01:56

vt-tv approved these changes Dec 16, 2025

View reviewed changes

vt-tv requested changes Dec 16, 2025

View reviewed changes

jonasohland requested changes Dec 16, 2025

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

KimonHoffmann reviewed Dec 16, 2025

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

lib/tests/test_pkgconfig_c_link.sh Outdated Show resolved Hide resolved

jptrainor force-pushed the main branch 3 times, most recently from 0be1757 to 2d0aa94 Compare December 16, 2025 15:41

vt-tv self-requested a review December 16, 2025 15:46

vt-tv approved these changes Dec 16, 2025

View reviewed changes

vt-tv requested changes Dec 16, 2025

View reviewed changes

jptrainor force-pushed the main branch from 2d0aa94 to f9ceb29 Compare December 16, 2025 19:00

jptrainor force-pushed the main branch from f9ceb29 to 7d5b469 Compare December 16, 2025 20:59

vt-tv approved these changes Dec 16, 2025

View reviewed changes

jptrainor force-pushed the main branch from 7d5b469 to d09b60c Compare December 16, 2025 21:11

jonasohland approved these changes Dec 17, 2025

View reviewed changes

pedro-alves-ferreira approved these changes Dec 18, 2025

View reviewed changes

vt-tv force-pushed the main branch from d09b60c to 3bee94b Compare December 18, 2025 14:07

KimonHoffmann requested changes Dec 18, 2025

View reviewed changes

jptrainor force-pushed the main branch 2 times, most recently from 11dd246 to 33fe8f0 Compare December 19, 2025 15:19

jptrainor force-pushed the main branch from 33fe8f0 to cad9df7 Compare December 19, 2025 15:43

jptrainor requested a review from KimonHoffmann December 19, 2025 17:45

jptrainor force-pushed the main branch 2 times, most recently from c488e19 to 7d71eb0 Compare December 22, 2025 18:00

jptrainor force-pushed the main branch from 7d71eb0 to 218083d Compare December 22, 2025 19:43

vt-tv merged commit 1ccf048 into dmf-mxl:main Dec 22, 2025
9 checks passed

Enable static builds with pkg-config integration. #288

Enable static builds with pkg-config integration. #288

Uh oh!

Conversation

jptrainor commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vt-tv left a comment

Choose a reason for hiding this comment

Uh oh!

vt-tv commented Dec 16, 2025

Uh oh!

vt-tv left a comment

Choose a reason for hiding this comment

Uh oh!

jonasohland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KimonHoffmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jptrainor commented Dec 16, 2025

Uh oh!

vt-tv left a comment

Choose a reason for hiding this comment

Uh oh!

jptrainor commented Dec 16, 2025

Uh oh!

jptrainor commented Dec 16, 2025

Uh oh!

jonasohland left a comment

Choose a reason for hiding this comment

Uh oh!

jptrainor commented Dec 17, 2025

Uh oh!

jptrainor commented Dec 17, 2025

Uh oh!

pedro-alves-ferreira left a comment

Choose a reason for hiding this comment

Uh oh!

KimonHoffmann commented Dec 18, 2025

Uh oh!

KimonHoffmann left a comment

Choose a reason for hiding this comment

Uh oh!

vt-tv commented Dec 22, 2025

Uh oh!

jptrainor commented Dec 22, 2025

Uh oh!

vt-tv commented Dec 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jptrainor commented Dec 15, 2025 •

edited

Loading