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

CMake Modernizations #969

Merged
merged 24 commits into from
Jan 4, 2022
Merged

CMake Modernizations #969

merged 24 commits into from
Jan 4, 2022

Conversation

derselbst
Copy link
Member

@derselbst derselbst commented Sep 1, 2021

This is a WIP for modernizing our cmake build system. The ideas emerged from #847. Open points:

  • exported targets
  • raise cmake_minimum_required ( VERSION 3.13 )
  • fix the over-linking issue (i.e. fix the unit tests)
  • better OpenMP support
  • Avoiding global functions (link_directories, include_directories, etc)
  • Using the more common GnuInstallDirs (requested already in Prefer CMAKE_INSTALL_LIBDIR over LIB_SUFFIX #411)
  • Remove enable-debug
  • Remove enable-pkgconfig
  • fix PC_REQUIRES_PRIV related overlinking in fluidsynth.pc
  • document 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 enable-fpe-check and enable-trap-on-fpe (other than merging both options, I have no idea here... keep it for now)

pedrolcl and others added 4 commits September 1, 2021 21:00
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
@pedrolcl
Copy link
Contributor

pedrolcl commented Sep 2, 2021

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.

@derselbst
Copy link
Member Author

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.

@pedrolcl
Copy link
Contributor

pedrolcl commented Sep 2, 2021

I would expect 3.13.5 instead of 3.13.3:
https://cmake.org/cmake/help/v3.13/release/3.13.html#id4

@derselbst
Copy link
Member Author

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
@pedrolcl
Copy link
Contributor

pedrolcl commented Sep 3, 2021

86 successful and 14 pending checks

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
@derselbst
Copy link
Member Author

Wouldn't be better to exclude them for this branch?

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
@pedrolcl pedrolcl mentioned this pull request Sep 7, 2021
@derselbst
Copy link
Member Author

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.

@pedrolcl
Copy link
Contributor

pedrolcl commented Sep 7, 2021

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.

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.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

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) {
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@derselbst
Copy link
Member Author

@pedrolcl There are two open points, which AFAIR were from you:

  • better clang-tidy support
  • CMAKE_MSVC_RUNTIME_LIBRARY

Could you let me know what exactly you had in mind?

@pedrolcl
Copy link
Contributor

@pedrolcl There are two open points, which AFAIR were from you:

* better clang-tidy support

I don't think so. I've never mentioned clang-tidy, and I don't have any proposals or plans about it.

* CMAKE_MSVC_RUNTIME_LIBRARY

Could you let me know what exactly you had in mind?

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 CMAKE_MSVC_RUNTIME_LIBRARYas a better alternative, but your patch was only setting the policy CMP0091 to NEW, and indeed this option requires CMake >= 3.15, so in my opinion what is missing is a little bit of documentation including some advice for users of older CMake versions.

@derselbst
Copy link
Member Author

so in my opinion what is missing is a little bit of documentation including some advice for users of older CMake versions.

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 /MD flag is hardcoded, AFAIK. So manually supplying /MT via CFLAGS wouldn't help. I've linked the regex hack that we used before, just in case any WindowsXP user out there wants to do that.

@derselbst derselbst marked this pull request as ready for review December 21, 2021 11:08
@derselbst
Copy link
Member Author

Ready for review

Copy link
Member Author

@derselbst derselbst left a 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 !

@derselbst derselbst merged commit 6b96c19 into master Jan 4, 2022
@derselbst derselbst deleted the cmake-modernization branch January 4, 2022 14:12
derselbst added a commit that referenced this pull request Jan 4, 2022
In the previous CMake change the pkgconfig file was accidentally not installed anymore. Didn't recognized this earlier, because the OBS CI workflow was broken and therefore not running.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants