-
Notifications
You must be signed in to change notification settings - Fork 267
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
CMake Modernizations #969
CMake Modernizations #969
Conversation
The build system creates two exported targets: - The executable FluidSynth::fluidsynth - The library FluidSynth::libfluidsynth A downstream project using CMake can find and link the library target directly with cmake (without needing pkg-config) this way: ~~~ project(sample LANGUAGES C) find_package ( FluidSynth ) if (FluidSynth_FOUND) add_executable( sample sample.c ) target_link_libraries ( sample PRIVATE FluidSynth::libfluidsynth ) endif () ~~~ After installing fluidsynth in a prefix like "$HOME/Fluidsynth3": cmake -DCNAKE_PREFIX_PATH="$HOME/Fluidsynth3/;..." Instead installing, the build directory can be used directly, for instance: cmake -DFluidSynth_DIR="$HOME/fluidsynth-2.2.2/build/" ...
* FluidSynthConfigVersion.cmake is created with ${VERSION} instead of ${LIB_VERSION_INFO} * FluidSynthConfig.cmake.in simplified: it doesn't need to include the version file. * Simplified BUILD_INTERFACE generator expression as suggested
In /CMakeLists.txt#L851 I don't understand why 3.13.3 is requested, when the enclosing if() is checking for version 3.12.0 This message was introduced in PR #933 merged last month. |
I wanted to avoid having to bump the cmake version twice within a short time (imagine we bump to 3.12 and later we find that we need 3.13 or so). That's why I proposed 3.13.3 - because it's the last version still supported on Windows XP - which I hope will last a bit longer as minimum required version in the future. |
I would expect 3.13.5 instead of 3.13.3: |
Oh, you're right. Seems like my brain-memory is corrupted, sorry. |
* raised cmake_minimum_required ( VERSION 3.13 ) * removed other CMAKE_VERSION checks * fixed overlinking of library, cmdline client, and unit tests
The OBS builds in unresolvable state are the 14 pending forever checks. Wouldn't be better to exclude them for this branch? |
* by target_include_directories * by target_link_libraries, using imported targets when possible
Done. I hope the pending ones will be gone after the next push. |
* sanitize_target_dirs(target) removes include and link directories that do not exist from the given immported target * generate_pkgconfig_spec() builds fluidsynth.pc taking the private libraries from a given target dependencies
No global macros
Looks like the recent FindOpenMP modernization broke compilation on Mac. This wasn't a problem before, because I silently added openMP support to our Mac Builds when I merged #975. And you can ignore the obsolete circleCI build for now. |
Succeeded in macOS 10.14 because OpenMP was not found, but failed in 10.15 and 11.0 when it was found. Not sure where is the problem. Looks like the OpenMP include directory is not being searched when compiling the CLI program, but it should be inherited from the object library. |
* removed and/or replaced most definitions from DefaultDirs.cmake * removed LIB_SUFFIX * fixed DEFAULT_SOUNDFOUNT in Windows
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
by explicitly enabling C language extensions. This will cause the std=gnu90 to be passed via CMAKE_REQUIRED_FLAGS, making clang know about the inline keyword rather than failing when querying for the netinet headers: /usr/local/lib/android/sdk/ndk-bundle/toolchains/llvm/prebuilt/linux-x86_64//sysroot/usr/include/linux/swab.h:39:8: error: unknown type name 'inline' static inline __attribute__((__const__)) __u32 __fswahw32(__u32 val) {
Kudos, SonarCloud Quality Gate passed! |
@pedrolcl There are two open points, which AFAIR were from you:
Could you let me know what exactly you had in mind? |
I don't think so. I've never mentioned clang-tidy, and I don't have any proposals or plans about it.
It is understandable that you link that with me, because I've opened #840 and #842 proposing a new CMake option to enable or disable linking the static MSVC runtime, but you didn't agree and implemented #848 in another way. It was you who mentioned during the discussions |
Ok, I've added a hint to the wiki. The advice for users of older CMake is to use CMake 3.15 - sry, but in former CMake versions the |
Ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'm merging this now. Thanks @pedrolcl !
This is a WIP for modernizing our cmake build system. The ideas emerged from #847. Open points:
link_directories
,include_directories
, etc)GnuInstallDirs
(requested already in Prefer CMAKE_INSTALL_LIBDIR over LIB_SUFFIX #411)enable-debug
enable-pkgconfig
PC_REQUIRES_PRIV
related overlinking influidsynth.pc
CMAKE_MSVC_RUNTIME_LIBRARY
better clang-tidy support(don't remember what was meant by that, or who came up with it)see if/how Add Android testing setup #912 could be incorporated(this will be handled separately, as there are too many dependencies / conflicts on / with the CMake related changes here)Rethink(other than merging both options, I have no idea here... keep it for now)enable-fpe-check
andenable-trap-on-fpe