CMake: Search for dependencies in FluidSynthConfig.cmake

[FtZPetruska]

This PR addresses the issue of consuming FluidSynth through CMake.

Existing Issue

When linking against a static FluidSynth::libfluidsynth, CMake returns the following error:

SynthTargets.cmake:60 (set_target_properties):
  The link interface of target "FluidSynth::libfluidsynth-OBJ" contains:

    OpenMP::OpenMP_C

  but the target was not found.

If fixed manually through a find_package call, the error repeats itself for PkgConfig::GLIB and every other CMake target listed in the INTERFACE_LINK_LIBRARIES of the FluidSynth::libfluidsynth-OBJ target.

Proposed Solution

This PR addresses this issue by searching for the dependencies in the generated FluidSynthConfig.cmake.

Firstly, we save optional dependencies in FLUIDSYNTH_<lib>_SUPPORT variables using the <lib>_SUPPORT variables from the configure step.

Then, for every dependency we need, we call pkg_check_modules or find_dependency (in place of find_package) with the same arguments as the main CMakeLists.

Additionally, we now need to install the custom Find<lib>.cmake modules along the CMake config.

Any feedback is appreciated!

[derselbst]

@pedrolcl FYI

[pedrolcl]

This CMake module is where the find_dependency() macro is defined. See:
https://cmake.org/cmake/help/latest/module/CMakeFindDependencyMacro.html

Also:

find_dependency forwards the correct parameters for QUIET and REQUIRED which were passed to the original find_package()

[FtZPetruska]

Thank you for the feedback!

Now that the find_dependency and pkg_check_modules calls are guarded to be only used when the library is static, should the REQUIRED keyword be put back? I had removed it in 287dc6a after a recommendation against it on microsoft/vcpkg#29636.

[pedrolcl]

Now that the find_dependency and pkg_check_modules calls are guarded to be only used when the library is static, should the REQUIRED keyword be put back? I had removed it in 287dc6a after a recommendation against it.

I don't think so. find_dependency() forwards the correct parameters for QUIET and REQUIRED which were passed to the original find_package().

On the other hand, there is nothing similar to find_dependency() for pkg-config dependencies, so you will need to provide all the arguments directly to pkg_check_modules(), unless you add another macro to PkgConfigHelpers.cmake and install also this new macro.

[FtZPetruska]

Thank you for clarifying!

Should the pkg_check_modules calls in the exported config match the ones in the CMakeLists?
I was thinking that they could also use REQUIRED, since when pkg_check_modules is called in config:

• We know the support was built into library.
• We know we're in a static linking context so the target being available is required for linking.

In both cases CMake would fail but with different messages:

• Without REQUIRED, the output would look like:

-- Checking for module 'foobar'
--   No package 'foobar' found
...
  The link interface of target "FluidSynth::libfluidsynth-OBJ" contains:

    PkgConfig::FOOBAR

  but the target was not found.

• With REQUIRED, the output would look like:

-- Checking for module 'foobar'
--   No package 'foobar' found
CMake Error at .../FindPkgConfig.cmake:607 (message):
   A required package was not found

What do you think ?

[pedrolcl]

@pedrolcl FYI

Thanks for the heads up, Tom. Maybe also @rncbc could be interested.

It is my fault that this issue was left behind. I've felt (and I still feel the same) that there is no clean solution for the libfluidsynth static library use case. The main reason for exporting targets was to enable downstream projects to use find_package(FluidSynth) without needing pkg-config, but then any complete solution (like this PR) introduces a call to find_dependency(PkgConfig) when configuring downstream projects, which defeats that goal 😞

On the other hand, when using pkg_check_modules(FluidSynth) (the pkg-config method) on downstream projects with a static libfluidsynth there is no issue finding all the required dependencies. The problem has already a working solution.

[FtZPetruska]

The main reason for exporting targets was to enable downstream projects to use find_package(FluidSynth) without needing pkg-config, but then any complete solution (like this PR) introduces a call to find_dependency(PkgConfig) when configuring downstream projects, which defeats that goal

I hear you, on Windows specifically where having pkg-config or pkgconf in your PATH is not a given.

Unfortunately, the canonical way to avoid using pkg_check_modules in the config would be to not use it altogether in the CMakeLists since the imported targets must match.

For this to work, every dependency of FluidSynth would need to export their own CMake config which should track their own dependency or have a Find module so the library can be found through a find_package call. On that note, the CMake documentation actually uses pkg-config in their guide to write a Find module.

Another option would be to list out the libraries in INTERFACE_LINK_LIBRARIES not through their target but through the actual library list, e.g.: put gthread-2.0;intl;glib-2.0;intl;iconv;m;pcre2-8 instead of PkgConfig::GLIB. After a call to pkg_check_modules(FOO ...) this list is stored in FOO_STATIC_LIBRARIES. The limitation of this approach is the link directory, while it can be set in INTERFACE_LINK_DIRECTORIES, this makes the package no longer easily relocatable.
For what it is worth, I was not able to easily make this approach work on my machine.

While I agree that the proposed solution is not perfect by far, it would at least provide parity between using find_package(FluidSynth) and pkg_check_modules(FluidSynth).

[pedrolcl]

every dependency of FluidSynth would need to export their own CMake config which should track their own dependency or have a Find module so the library can be found through a find_package call

Exactly. Let's do it.

• If our dependency does not export cmake targets, then we add and install our own private FindXXX.cmake module
• We should also contribute proper cmake support to the upstream dependency, if possible
• When a new version of the dependency starts providing proper cmake support, we request that minimum version
• Loop until we live on a better world

For instance, libsndfile 1.2 is almost ready

[FtZPetruska]

Okay, I've been working on it for the past few days and I am quite happy with the results. Here is some insight with how I implemented it:

Firstly, for the "easy" ones, ALSA, DBUS, oboe, PulseAudio, and SDL2 did not need any custom find modules. ALSA, similarly to Threads, has a module that ships with CMake. As for the others, they have had an upstream config shipped for a while.

Secondly, there were libraries that have an official CMake config sometimes. Libraries such as sndfile and its dependencies do ship CMake configs, but only when built with CMake. The issue is that not every package manager does, most notably on macOS Homebrew builds most of them with autotools, which results in no config installed. For such libraries, their Find module creates the same variables and targets to be as compatible as possible with upstream.

Lastly, there were libraries that export no CMake config whatsoever, most notably GLib2, old libraries such as LASH or MidiShare, or even libinstpatch (which uses CMake as a build system). For such libraries, I had to make a find module from scratch.

One thing I should mention, you may notice that in many Find modules, pkg-config is still used. But its use is fully optional! When available, it provides a lot of valuable information that is not necessarily accessible, such as the version of the package and its dependencies. If it is not available, then the modules try to work off what the library headers may expose.

Similarly, I made sure the provided Find modules are fully interchangeable with the upstream CMake config files. If a developer or a consumer sets CMAKE_FIND_PACKAGE_PREFER_CONFIG to a TRUE value, CMake will prioritise the config over the find module if it exists.

---

Testing played a big part in ensuring things were worked as intended. So here were the tests that were successfully conducted:

• Windows:
  • dependencies from vcpkg (x64-windows, x64-windows-static)
  • without pkg-config
  • static and shared build
  • with and without CMAKE_FIND_PACKAGE_PREFER_CONFIG
  • build using the vcpkg recipe
  • build sdl2-mixer through vcpkg with the fluidsynth feature, and link an executable
  • build sdl2-mixer through vcpkg with the all features (shares transitive dependencies with sndfile), and link an executable
• macOS:
  • dependencies from Homebrew and vcpkg
  • static and shared build
  • with and without pkg-config
  • with and without CMAKE_FIND_PACKAGE_PREFER_CONFIG
• Linux (Ubuntu 22.04):
  • LASH built from source
  • dependencies from APT and vcpkg
  • static and shared build
  • with and without pkg-config
  • with and without CMAKE_FIND_PACKAGE_PREFER_CONFIG
  • install the library and use it in a sample CMake project

What I was not able to test:

• oboe and OpenSLES, I don't have the proper Android dev environment
• OS/2
• MidiShare, I wasn't able to compile it since it needs a very old Kernel

---

I know this is suddenly a much bigger PR, but as it stands, this makes the project now able to be configured and consumed without pkg-config, yet still benefit from it if it is available!

Let me know if you have any question regarding the implementation and if you need any change!

[derselbst]

Ok, wow. Much bigger than expected. So I see a few new things here:

1. Some dependency libraries now require newer versions
2. The dependencies now have to be tracked in two different places (CMakeLists.txt and FluidSynthConfig.cmake.in)

I'm fine with 1. in general, I'm just asking myself whether it's fine to release this change with the next bugfix release (2.3.2) or if we better wait until next minor bump (2.4.0).

Regarding 2.: Would it make sense to store the versions of the dependencies in separate variables, so that we can get rid of keeping them in sync at two different places? I mean, the FluidSynthConfig.cmake.in is sourced anyway, so it should be possible to e.g. set( GLIB_VERSION 2.6.5) in CMakeLists.txt which is then expanded in FluidSynthConfig.cmake.in to the desired version. Instead of defining a sparate variable, we could also think about directly consuming the version variable set by the find modules, so in this case GLib2_VERSION.

Additionally, I'd like to ask you for two more things:

• Pls. merge the recent master into your changes (I've just fixed the Android CI pipeline, and Carlo has made some CYGWIN specific cmake changes).
• And pls., put a small README into the cmake_admin/ folder that briefly explains why all these Find modules suddenly exist now. (This is just for some future contributors to explain, why the previous dependency handling with pkg_config has been replaced with custom cmake find modules.)

I'm leaving the technical review to @pedrolcl :)

EDIT: Oh, and if there is any simple test case that you or Pedro could think of, you're welcome to add it to one of the CI pipelines as well.

[pedrolcl]

This PR is not yet ready for production. I could not test many scenarios, but some of my tests have been unsuccessful.

I've made my tests on a Fedora 37 box, using a patched libsndfile dependency as a test case, building libsndfile as a static library with MP3 support; cmake argument: -DCMAKE_INSTALL_PREFIX=$HOME/SndFile. Note: there is an open libsndfile issue preventing the upstream project to be used out of the box.

1. Installing libsndfile at $HOME, and providing (or not, this is not strictly necessary, this custom prefix is probed automatically.) -DCMAKE_PREFIX_PATH=$HOME/SndFile to cmake. This scenario is broken. The logs prove that libsndfile is found, but libmp3lame is not, which invalidates the whole libsndfile dependency.
2. Using the dependenciesbuild directory directly, providingcmakethe argument-DSndFile_DIRpointing to thelibsndfile`build directory. This scenario is also broken, but this is a libsndfile fault which is not fixed by my patch.

I've not tried the next level: installing libfluidsynth at the $HOME directory, and trying to build a test project using similar scenarios: using CMAKE_PREFIX_PATH and FluidSynth_DIR cmake variables.

In short: even when the upstream projects seem to provide cmake exported targets, they may be broken. And writing custom FindXXXX cmake modules is more difficult than it seems.

[FtZPetruska]

Thank you everyone for your feedback!

Regarding 2.: Would it make sense to store the versions of the dependencies in separate variables, so that we can get rid of keeping them in sync at two different places? I mean, the FluidSynthConfig.cmake.in is sourced anyway, so it should be possible to e.g. set( GLIB_VERSION 2.6.5) in CMakeLists.txt which is then expanded in FluidSynthConfig.cmake.in to the desired version. Instead of defining a sparate variable, we could also think about directly consuming the version variable set by the find modules, so in this case GLib2_VERSION.

I went with the option of setting <LIB>_MINIMUM_VERSION when applicable. Using the value "returned" from the find modules wouldn't not work well as not all libraries expose their version outside of pkg-config.

On the topic of version, I reverted the changes of minimum version for SDL2 and libsndfile as the Linux CI does not have a recent enough version for each.

• Pls. merge the recent master into your changes (I've just fixed the Android CI pipeline, and Carlo has made some CYGWIN specific cmake changes).

Rebased this branch on master. Thank you for the heads up!

• And pls., put a small README into the cmake_admin/ folder that briefly explains why all these Find modules suddenly exist now. (This is just for some future contributors to explain, why the previous dependency handling with pkg_config has been replaced with custom cmake find modules.)

I updated the README.cmake.md at the top of the file tree so it no longer uses pkg_check_modules. Additionally, I wrote a README.md in cmake_admin to briefly explain the presence of the Find modules.

EDIT: Oh, and if there is any simple test case that you or Pedro could think of, you're welcome to add it to one of the CI pipelines as well.

I added a test case under test/cmake, this is a simple CMake project that calls find_package on FluidSynth and attempts to link the library to an executable. I updated the linux CI to run this test twice, once from the build directory, and once from the installation directory. Unfortunately, I'm not familiar enough with the other CI systems to run the test there.

In aef4eb9, I also updated the rest of that workflow. There were issues with the installed packages (e.g. ladspa was enabled but not installed, clang-tidy-10 did not install a clang-tidy binary, the runtime package of glib2 was installed instead of the -dev package). But if you think it's out of place for this PR, this commit can easily be reverted since it does not bring any functional changes to the rest of this.

I've made my tests on a Fedora 37 box, using a patched libsndfile dependency as a test case, building libsndfile as a static library with MP3 support; cmake argument: -DCMAKE_INSTALL_PREFIX=$HOME/SndFile. Note: there is an open libsndfile issue preventing the upstream project to be used out of the box.

Thank you for pointing this out, I was able to reproduce your issues and partially come up with solutions.

Firstly, there was an issue with Findmp3lame introduced in 9214277. In theory, mp3lame_FOUND should only be set to TRUE if both components are found, but in practice, it makes the upstream libsndfile config fail when mpghip is unavailable. With this fix, I was able to consume libsndfile with SndFile_DIR set to its build directory or $HOME/libsndfile/lib/cmake/SndFile.

The second and third issues are somewhat tied. With the static-only build of libsndfile living outside of standard compiler search paths, when using the find module instead of the config file, linking the fluidsynth executable would fail with the message ld: library not found for -lsndfile, despite the INTERFACE_LINK_DIRECTORIES being properly set on the SndFile::sndfile target. And when addressed, linking would still fail with undefined references to MPG123 and mp3lame.

It appears that the INTERFACE_LINK_DIRECTORIES property of libfluidsynth's dependencies do not get forwarded to the consumers of libfluidsynth. On the other hand, the undefined references come from getting the dependencies out of PC_SNDFILE_ LIBRARIES. vcpkg patches pc files in static builds by making .private field public. But in normal builds, the MPEG libraries are only in the .private fields, even in static-only builds.

My idea to solve these two issues was to use either the <Lib>_STATIC_LDFLAGS or <Lib>_LDFLAGS depending on the library's extension and set it as the INTERFACE_LINK_LIBRARIES. But while it works fine on Unix systems, the Windows CI seems to strongly disagree with this idea as the linker seems unable to understand the flags provided by pkg-config.

I did notice that linking fluidsynth-OBJ publicly instead of privately did solve the link_directories issue. In combination with using the LIBRARIES variables instead of LDFLAGS, it should fix build on Windows. But linking fluidsynth-OBJ publicly does not feel right, what do you think ?

[derselbst]

I did notice that linking fluidsynth-OBJ publicly instead of privately did solve the link_directories issue. In combination with using the LIBRARIES variables instead of LDFLAGS, it should fix build on Windows. But linking fluidsynth-OBJ publicly does not feel right, what do you think ?

It seems like Pedro was the last one who changed it to private linkage. However, the CMake docs suggest public linkage. Bumping CMake minimum version to 3.12 seems to be required as well. I would be fine with that.

[pedrolcl]

I did notice that linking fluidsynth-OBJ publicly instead of privately did solve the link_directories issue. In combination with using the LIBRARIES variables instead of LDFLAGS, it should fix build on Windows. But linking fluidsynth-OBJ publicly does not feel right, what do you think ?

It seems like Pedro was the last one who changed it to private linkage. However, the CMake docs suggest public linkage. Bumping CMake minimum version to 3.12 seems to be required as well. I would be fine with that.

Do you mean in commit 21aca08 ? It was already private before. But yes, I believe that changing the linkage to PUBLIC would not change anything on the shared library, but will propagate dependencies to the CLI fluidsynth program, and to every other downstream program producing severe over-linking.

For instance, this is what we get on the shared library when using target_link_libraries ( libfluidsynth PRIVATE libfluidsynth-OBJ )

$ objdump -p libfluidsynth.so.3.1.4 | grep NEEDED
  NEEDED               libgomp.so.1
  NEEDED               libgthread-2.0.so.0
  NEEDED               libglib-2.0.so.0
  NEEDED               libsndfile.so.1
  NEEDED               libpulse-simple.so.0
  NEEDED               libpulse.so.0
  NEEDED               libasound.so.2
  NEEDED               libpipewire-0.3.so.0
  NEEDED               libdbus-1.so.3
  NEEDED               libreadline.so.8
  NEEDED               libstdc++.so.6
  NEEDED               libm.so.6
  NEEDED               libgcc_s.so.1
  NEEDED               libc.so.6

  $ objdump -p fluidsynth | grep NEEDED
  NEEDED               libfluidsynth.so.3
  NEEDED               libgomp.so.1
  NEEDED               libsystemd.so.0
  NEEDED               libgthread-2.0.so.0
  NEEDED               libglib-2.0.so.0
  NEEDED               libpipewire-0.3.so.0
  NEEDED               libc.so.6

If now we change it to: target_link_libraries ( libfluidsynth PUBLIC libfluidsynth-OBJ )

$ objdump -p libfluidsynth.so.3.1.4 | grep NEEDED
  NEEDED               libgomp.so.1
  NEEDED               libgthread-2.0.so.0
  NEEDED               libglib-2.0.so.0
  NEEDED               libsndfile.so.1
  NEEDED               libpulse-simple.so.0
  NEEDED               libpulse.so.0
  NEEDED               libasound.so.2
  NEEDED               libpipewire-0.3.so.0
  NEEDED               libdbus-1.so.3
  NEEDED               libreadline.so.8
  NEEDED               libstdc++.so.6
  NEEDED               libm.so.6
  NEEDED               libgcc_s.so.1
  NEEDED               libc.so.6

  $ objdump -p fluidsynth | grep NEEDED
  NEEDED               libfluidsynth.so.3
  NEEDED               libgomp.so.1
  NEEDED               libgthread-2.0.so.0
  NEEDED               libglib-2.0.so.0
  NEEDED               libpipewire-0.3.so.0
  NEEDED               libm.so.6
  NEEDED               libsndfile.so.1
  NEEDED               libpulse-simple.so.0
  NEEDED               libpulse.so.0
  NEEDED               libasound.so.2
  NEEDED               libdbus-1.so.3
  NEEDED               libreadline.so.8
  NEEDED               libsystemd.so.0
  NEEDED               libc.so.6

[FtZPetruska]

It appears that this issue is not related to the indirection from using libfluidsynth-OBJ, but is an issue from CMake. On 3.16 and below, INTERFACE_LINK_DIRECTORIES is not a transitive property on static libraries (see policy CMP0099).

Because of that, if:

• libfluidsynth is built statically
• A dependency is found with pkg-config
• That dependency has transitive dependencies installed outside of the compiler's default search paths

Linking an executable against libfluidsynth will fail because the library directory is not forwarded.

As an alternative, I attempted making and linking against an imported target. Through adding IMPORTED_TARGET to the pkg_check_modules call and linking to the resulting PkgConfig:: target, similar to what was used before this PR. Unfortunately, this did not succeed, moreover, this also has the issue of not linking the transitive dependencies.
For example, my copy of libsndfile is built statically but the PkgConfig:: target does not provide information about its transitive dependencies.

The only solution I found that fixed the issues is the CMP0099 policy. Considering there are still Windows XP workflows, I figure simply bumping the minimum version to 3.17 is not possible, so I added a warning similar to glib's in case of a static build:

if(POLICY CMP0099)
  cmake_policy(SET CMP0099 NEW)
elseif(NOT BUILD_SHARED_LIBS)
  message(WARNING "Your version of CMake is very old. This may cause linking issues if your dependencies are not in your compiler's default search paths.")
endif()

For reference, Debian stable (bullseye) ships CMake 3.18, and this version is also available on oldstable (buster) through backports.

[derselbst]

Great finding, thanks! We are almost there. One very special MacOS build has failed:

https://dev.azure.com/tommbrt/tommbrt/_build/results?buildId=8848&view=logs&j=2c1603d1-f56f-58d2-9e4b-d6489fed1fea&t=94bc5656-6b8e-5164-2ec4-8be4a516e929&l=344

I just made a quick diff of the longish call to c++. It seems like previously, it was linking against the following libs:
-lgthread-2.0 -lsndfile -lreadline

whereas they are now missing, however we got a new rpath: -Wl,-rpath,/opt/local/lib

[FtZPetruska]

That failure seems to be related to the selection of libraries for that build, below that long command there are the following statements:

ld: warning: ignoring file /usr/local/lib/libglib-2.0.dylib, building for macOS-arm64 but attempting to link with file built for macOS-x86_64
ld: warning: ignoring file /usr/local/lib/libgthread-2.0.dylib, building for macOS-arm64 but attempting to link with file built for macOS-x86_64
ld: warning: ignoring file /usr/local/lib/libsndfile.dylib, building for macOS-arm64 but attempting to link with file built for macOS-x86_64

I assume the universal libraries are located in /opt/local, but find_library finds the one in /usr/local first:

-- Found GLib2: /usr/local/lib/libglib-2.0.dylib (found suitable version "2.72.3", minimum required is "2.6.5") 
-- Found SndFile: /usr/local/lib/libsndfile.dylib (found suitable version "1.2.0", minimum required is "1.0.0") 

I will try including -DCMAKE_PREFIX_PATH=/opt/local to the CMake command, according to its documentation, this should make find_library (and find_package in general) prioritise /opt/local over /usr/local.

---

EDIT: While this worked, it seems to be preferable to use HINTS instead of PATHS in find_library/find_path, the difference between the two is priority:

• HINTS is checked first
• system directories are then checked
• PATHS is checked last
  Since we use on pkg-config for the libraries in /opt/local, HINTS should be used.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[derselbst]

Ok, I'd would be fine with this change. Thanks! @pedrolcl What about you?

[pedrolcl]

What about you?

I agree. Let's go ahead.