CMake modernization

[hobyst]

Partial re-write of the CMake project using modern CMake conventions.

• The project should now be easier to integrate as a CMake subdirectory inside other CMake projects
• Dependency detection should be much more flexible now
• Instead of using variables and lists, now every source file used in every target is clearly declared in the same CMake file used to create and set up the target, making it much easier to debug
• To compose and set up the RmlUi target, instead of using conditionals and file lists, INTERFACE targets are used. These allow to group target options like source files and libraries to link against as CMake targets that generate no build artifacts whatsoever. When a target links against an INTERFACE target, all the properties from the INTERFACE target will be set as if they were their own.
• No compiler flags are set by the project, meaning the project is easier to maintain
• Now all CMake options related to the project are preceded by RMLUI_ for better organization in the CMake GUI.

However, since this is a partial re-write:

• Only desktop platforms are expected to work
• Some dependencies like Lua are not available and will need further work since the custom find modules are outdated and need to be re-written using modern CMake conventions.
• Only has been tested with the GLFW_GL3 platform backend and the automatic election of the backend hasn't been ported over the new CMake project. All backends have been implemented and should be functional, although not every single one of them has been tested. Automatic election of the backend for samples hasn't been implemented.
• CMake logic for the following options hasn't been implemented:
  • RMLUI_DISABLE_RTTI_AND_EXCEPTIONS: Would be ideal to change it to positive logic (RMLUI_ENABLE_RTTI_AND_EXCEPTIONS) for more clarity.
  • RMLUI_ENABLE_PRECOMPILED_HEADERS: Precompiled headers haven't been implemented.
  • RMLUI_NO_THIRDPARTY_CONTAINERS
• Installing and exporting the library hasn't been implemented yet
• AppVeyor CI builds will likely fail until installing and exporting have been implemented. Once they get implemented, CPack can be used to create archives of the compiled projects directly using CMake.
• Tests haven't been implemented

If it is preferred to have the guidelines specified in CONTRIBUTING.md in the documentation repo, then I will contribute them there instead.

[hobyst]

@mikke89 Any comments so far?

[mikke89]

First of all, this is so much better to read, so thank you! A very welcome addition.

Here are some thoughts and notes, in no particular order, not everything here needs to be addressed in this PR.

• Did you write the contributions file yourself? It's very helpful, for me included, and very detailed, good job! I think generally, these are great recommendations. And I think it belongs in the repository like this.
• For dependencies, as I understand it, find modules are necessary for system packages and thirdparty libraries that don't generate their own configuration.
  • We should always prefer the built-in modules when available.
• Now that we have an opportunity to change the names: Rename to avoid negated option names (NO/DISABLE etc).
  • Also, make selected font engine a string set option. "freetype" / "none".
• Public headers not added to projects?
• I prefer not to use CXX_STANDARD, because we support anything newer and should be able to be compiled at any version the user desires. I think we should use a requirement instead.
  • I.e. target_compile_features(${NAME} PUBLIC cxx_std_14).
  • Looks like CMAKE_CXX_STANDARD_REQUIRED is a boolean value, probably does not do what you intend. Seems to be one reason some of the tests fail. In any case, we don't need this with the above command.
  • What do you think about adding set_target_properties(${NAME} PROPERTIES CXX_EXTENSIONS OFF)? Or alternatively, set CMAKE_CXX_EXTENSIONS off, I'm fine with this one as a global. I think this is useful to ensure that we don't use extensions internally, but I wonder if you think this breaks the spirit of the "compiler flag" policy?
  • Maybe make an interface target with all our requirements like the ones above (ideally as few as possible)?
• Tests are missing for now.
• Exporting and installing, any thoughts or plans on that?
• I am going to miss the script to update all the source file lists with one click.
• Precompiled headers help a lot with compile times, I don't think we can forego that option.
• Might as well get rid of the old CMakeLists, rather than keeping a copy.
• Cmake-presets ideas (feedback?):
  • Default consumer mode (Core + Debugger).
  • Default + all samples.
  • Add desired warnings. Ideally, CI fails on warnings, without setting warnings as errors.
  • Disabled exceptions.
  • I don't know if we want e.g. built-in mingw/WSL configurations etc.
• Desired options / libraries:
  • Tracy.
  • SVG plugin.
  • Is the Lottie sample added regardless of whether the plugin is enabled, or am I reading that wrong?

[hobyst]

Did you write the contributions file yourself? It's very helpful, for me included, and very detailed, good job! I think generally, these are great recommendations. And I think it belongs in the repository like this.

Yes.

For dependencies, as I understand it, find modules are necessary for system packages and thirdparty libraries that don't generate their own configuration.

We should always prefer the built-in modules when available.

Yes, that is correct.

Now that we have an opportunity to change the names: Rename to avoid negated option names (NO/DISABLE etc).

Also, make selected font engine a string set option. "freetype" / "none".

I agree on both.

Public headers not added to projects?

Adding header files as sources isn't needed. Projects do it so that IDEs like Visual Studio show them in the project structure, but isn't needed to use them. All it's actually needed is add the include directory and that's it.

I prefer not to use CXX_STANDARD, because we support anything newer and should be able to be compiled at any version the user desires. I think we should use a requirement instead.

I.e. target_compile_features(${NAME} PUBLIC cxx_std_14).
Looks like CMAKE_CXX_STANDARD_REQUIRED is a boolean value, probably does not do what you intend. Seems to be one reason some of the tests fail. In any case, we don't need this with the above command.
What do you think about adding set_target_properties(${NAME} PROPERTIES CXX_EXTENSIONS OFF)? Or alternatively, set CMAKE_CXX_EXTENSIONS off, I'm fine with this one as a global. I think this is useful to ensure that we don't use extensions internally, but I wonder if you think this breaks the spirit of the "compiler flag" policy?
Maybe make an interface target with all our requirements like the ones above (ideally as few as possible)?

Yes, the way I used CMAKE_CXX_STANDARD_REQUIRED was wrong. I've corrected it.

Using CXX_STANDARD with CMAKE_CXX_STANDARD_REQUIRED set to TRUE turns the value of CXX_STANDARD into a hard floor requirement. By doing this for the CMake project we are not setting the CXX_STANDARD, but the minimum C++ standard needed to compile the project. Or at least that's how I understand it.

I would like to remove C++ compiler-specific extensions as well, but they are needed with MSVC for the whole __dllspec(dllimport)/__dllspec(dllimport) thing. Although WINDOWS_EXPORT_ALL_SYMBOLS is a thing, so it could be possible to get rid of it. And no, it doesn't break the no compiler flag policy. The compiler flag policy is there so that the project won't add compiler flags like /W4 to the compiler calls, which are obviously completely dependent on the compiler. Some environments will likely need changes in the CMake code, but it is better to keep such cases to the very minimum.

Exporting and installing, any thoughts or plans on that?

Yeah that's a to-do.

Precompiled headers help a lot with compile times, I don't think we can forego that option.

Well, you might want to wait until the project is compatible with CMake 3.16 then.

Might as well get rid of the old CMakeLists, rather than keeping a copy.

I keep it for easier rebasing and easy access to compare with the previous implementation. Once the CMake project is completely re-written, it will be obvious to get rid of it.

Is the Lottie sample added regardless of whether the plugin is enabled, or am I reading that wrong?

Yes. I agree that is better to only enable if the plugin is enabled.

Cmake-presets ideas (feedback?):

Default consumer mode (Core + Debugger).
Default + all samples.
Add desired warnings. Ideally, CI fails on warnings, without setting warnings as errors.
Disabled exceptions.
I don't know if we want e.g. built-in mingw/WSL configurations etc.

The default consumer modes should be the one resulting from running CMake without any kind of option added to it, so the presets will likely add upon to override the defaults and pre-set stuff like compilers. All the suggestions for presets are good.
If enough people want MinGW or WSL preset, those can be made, although users can also create their own without dirtying CMakePresets.json by writing their own CMakeUserPresets.json and even create new presets based on the ones provided by the project.

[mikke89]

Adding header files as sources isn't needed. Projects do it so that IDEs like Visual Studio show them in the project structure, but isn't needed to use them. All it's actually needed is add the include directory and that's it.

I understand, however, it's very helpful to have them listed in IDEs, so in my opinion we should add them.

Using CXX_STANDARD with CMAKE_CXX_STANDARD_REQUIRED set to TRUE turns the value of CXX_STANDARD into a hard floor requirement. By doing this for the CMake project we are not setting the CXX_STANDARD, but the minimum C++ standard needed to compile the project. Or at least that's how I understand it.

The reason I prefer target_compile_features is that this sets a minimum requirement, and then this can be overriden by a parent project, or by setting the a value on the command line. By setting CMAKE_CXX_STANDARD locally we override the preference of the user, for example if they want to compile everything in C++17. I use this myself to test the library in different standard versions.

I would like to remove C++ compiler-specific extensions as well, but they are needed with MSVC for the whole __dllspec(dllimport)/__dllspec(dllimport) thing. Although WINDOWS_EXPORT_ALL_SYMBOLS is a thing, so it could be possible to get rid of it. And no, it doesn't break the no compiler flag policy.

We already turn off CMAKE_CXX_EXTENSIONS in the library, and that works just fine, so I think we can then safely set this to off from the root directory then. It does not affect things like dll-specifications or most compiler intrinsics, this is more about language-incompatible extensions (e.g. some GNU extensions). The dllspec definitions work just fine as it is in my opinion, WINDOWS_EXPORT_ALL_SYMBOLS is considered bad practice and something we should not add.

Well, you might want to wait until the project is compatible with CMake 3.16 then.

We already have pre-compiled headers enabled on our current CMake list through a version-check, and that seems to work fine. We should be able to do the same here.

The default consumer modes should be the one resulting from running CMake without any kind of option added to it, so the presets will likely add upon to override the defaults and pre-set stuff like compilers.

Sounds good. What do you think about adding compiler flags to the default cmake presets? In particular, I wonder if we should add our supported warning flags or not. Or otherwise leave that to a more "dev" type preset.

So how do you want us to proceed with this. Do you intend to keep actively working on this PR, and implementing the features and behavior discussed here? Or would you rather that we merge this early and let other people test and contribute simultaneously?

[hobyst]

I understand, however, it's very helpful to have them listed in IDEs, so in my opinion we should add them.

I'll add them as well then.

The reason I prefer target_compile_features is that this sets a minimum requirement, and then this can be overriden by a parent project, or by setting the a value on the command line. By setting CMAKE_CXX_STANDARD locally we override the preference of the user, for example if they want to compile everything in C++17. I use this myself to test the library in different standard versions.

Makes sense. I'll change the code to require C++ compile features instead.

We already turn off CMAKE_CXX_EXTENSIONS in the library, and that works just fine, so I think we can then safely set this to off from the root directory then. It does not affect things like dll-specifications or most compiler intrinsics, this is more about language-incompatible extensions (e.g. some GNU extensions). The dllspec definitions work just fine as it is in my opinion, WINDOWS_EXPORT_ALL_SYMBOLS is considered bad practice and something we should not add.

I'll enable it then.

We already have pre-compiled headers enabled on our current CMake list through a version-check, and that seems to work fine. We should be able to do the same here.

Indeed, and increasing the minimum CMake version won't be needed that way. However, I don't have experience implementing pre-compiled headers so it's something that another dev will need to implement.

Sounds good. What do you think about adding compiler flags to the default cmake presets? In particular, I wonder if we should add our supported warning flags or not. Or otherwise leave that to a more "dev" type preset.

I think warning flags set based on the consideration of the project should be left for presets targeted towards library developers. Consumers might not necessarily like having their compile or CI logs full of warnings from another project when they are likely only focused on warnings triggered by the code they actually control, not to mention the fact that the kind of warnings they are interested in might not be the same as the ones the project is interested in. If they want to ignore or add certain warning flags, it should always be done explicitly by the consumer and over what they expect to be the compiler's default behavior.

So how do you want us to proceed with this. Do you intend to keep actively working on this PR, and implementing the features and behavior discussed here? Or would you rather that we merge this early and let other people test and contribute simultaneously?

I would rather work over this a bit more, mitigate some possible bugs and merge earlier. There's a lot of features of the current (old) CMake project that I simply cannot implement by myself like compatibility for macOS, Emscripten, iOS and Android and the CMake find modules for the other dependencies. Once the PR is ready to review I would also like to get other contributors to test it to ensure that the use cases I can provide support for (Linux and Windows with or without Freetype and/or rlottie) work properly and that I haven't accidentally used CMake features not available in CMake 3.10 (I use the latest stable release).

As a side note, in order to track what is left I think it would be nice to create issues for each feature left to implement and add them to the 6.0 milestone.

[mikke89]

Alright, all of this sounds good to me. I can add the precompiled headers once this one is merged into the cmake branch.

Instead of making too many individual issues, I think we should instead make a task list in #198 with all the TODOs. And alright, let's aim for RmlUi 6.0, might be a bit ambitious but we'll try, depends mostly on the amount of feedback we get.

[hobyst]

@mikke89 I think I have amended the PR enough to be good to review again. Should I rebase to latest master commit before?

Also, here's the list of to-dos I've come up with:

• RMLUI_ENABLE_RTTI_AND_EXCEPTIONS: defaulting to ON
• RMLUI_ENABLE_PRECOMPILED_HEADERS: Precompiled headers haven't been implemented. Defaulting to ON
• RMLUI_ENABLE_THIRDPARTY_CONTAINERS: defaulting to ON
• RMLUI_USE_MATRIX_ROW_MAJOR: defaulting to OFF
• RMLUI_USE_CUSTOM_CONFIGURATION: defaulting to OFF
• RMLUI_WARNINGS_AS_ERRORS: This option should not be implemented because implementing it means handling differences in how warnings are specified based on the compiler. If wanted, this should be implemented using a developer-oriented CMake preset that adds the desired warning flag toCMAKE_<LANG>_FLAGS .
• RMLUI_BUILD_UNIVERSAL_BINARIES: This option should not be implemented. As specified in CMake's documentation, the choice of target architectures for Apple platforms should be left to the consumer's choice.
• Add the rest of the samples
• Lua support via plugin: For Lua, CMake already provides a find module.
• SVG support via plugin: Since lunasvg already uses CMake as a meta-build system, rather than creating a find module for the RmlUi project, the ideal approach would be to contribute a package config file to the upstream project.
• Tests: Add tests from Tests folder to the build project. Usage of CTest might be worth looking at.
• Implement installing and exporting: Reference from Modern CMake guide. Aside from implementing CMake config files, implementing automatic generation of pkg-config (.pc) files at install time should be investigated. Since the project is now also heavily target-oriented, we can now fill in the details of the pkg-config definitions and CMake package config files such as which compiler definitions the consumer needs to use by checking the INTERFACE_ target properties of the targets that need to be exported, similar to how CMake would do on its own if RmlUi was a target built inside the same project.
• Bring back AppVeyor CI: For this, usage of CPack should be investigated. Usage of CPack requires installing and exporting to be implemented first.
• Platform-support: Building for iOS and Android is still untested. The Android SDK provides its own CMake toolchain files and iOS is supported by both upstream CMake and the one shipped along with the iOS SDK out of the box. Therefore there should be no need to write platform nor toolchain files to support both platforms.
• Tracy: Tracy seems to now support being built using CMake and also has a CMake package config file of their own, however its usage is undocumented. Its setup is simple enough to be able to write a simple find module for it.
• Revise the naming of the CMake options
• Implement automatic selection of the backend: If the amount of code is considerable implement it in a separate .cmake file to improve conciseness and not to crowd the main CMakeLists.txt file in the Samples folder.
• Implement creation of macOS framework bundles: CMake already provides integration with macOS framework files. Contrary to what was done before, I would recommend building both the core library and the debugger library as separate frameworks.
• Create CMake presets and initial cache files: As discussed to ease building.
• Document how to interface the new build project: Likely the last step once the feature freeze has been reached. Mention as well some of the standard conventions of CMake like the fact that you can set up some of the build options directly on the environment, use CMake toolchain files in case the compiler they want to use isn't automatically detected by CMake like in Ubuntu, where many versions of the Clang compiler may be installed but clang doesn't always call the desired version as well as initial cache scripts. To aid, some templates could also be made so that consumers can take advantage of these without knowing about CMake.

Markdown source for the list:

- [ ] `RMLUI_ENABLE_RTTI_AND_EXCEPTIONS`: defaulting to `ON`
- [ ] `RMLUI_ENABLE_PRECOMPILED_HEADERS`: Precompiled headers haven't been implemented. Defaulting to `ON`
- [ ] `RMLUI_ENABLE_THIRDPARTY_CONTAINERS`: defaulting to `ON`
- [ ] `RMLUI_USE_MATRIX_ROW_MAJOR`: defaulting to `OFF`
- [ ] `RMLUI_USE_CUSTOM_CONFIGURATION`: defaulting to `OFF`
- [ ] `RMLUI_WARNINGS_AS_ERRORS`: This option should not be implemented because implementing it means handling differences in how warnings are specified based on the compiler. If wanted, this should be implemented using a developer-oriented CMake preset that adds the desired warning flag to`CMAKE_<LANG>_FLAGS` .
- [ ] `RMLUI_BUILD_UNIVERSAL_BINARIES`: This option should not be implemented. [As specified in CMake's documentation](https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html#cross-compiling-for-ios-tvos-or-watchos), the choice of target architectures for Apple platforms should be left to the consumer's choice.
- [ ] **Add the rest of the samples**
- [ ] **Lua support via plugin:** For Lua, CMake already provides a [find module](https://cmake.org/cmake/help/latest/module/FindLua.html).
- [ ] **SVG support via plugin:** Since lunasvg already uses CMake as a meta-build system, rather than creating a find module for the RmlUi project, the ideal approach would be to contribute a package config file to the upstream project.
- [ ] **Tests:** Add tests from `Tests` folder to the build project. Usage of [CTest](https://cmake.org/cmake/help/latest/manual/ctest.1.html) might be worth looking at.
- [ ] **Implement installing and exporting:** [Reference from Modern CMake guide](https://cliutils.gitlab.io/modern-cmake/chapters/install.html). Aside from implementing CMake config files, implementing automatic generation of pkg-config (`.pc`) files at install time should be investigated. Since the project is now also heavily target-oriented, we can now fill in the details of the pkg-config definitions and CMake package config files such as which compiler definitions the consumer needs to use by checking the `INTERFACE_` target properties of the targets that need to be exported, similar to how CMake would do on its own if RmlUi was a target built inside the same project.
- [ ] **Bring back AppVeyor CI:** For this, usage of [CPack](https://cmake.org/cmake/help/book/mastering-cmake/chapter/Packaging%20With%20CPack.html) should be investigated. Usage of CPack requires installing and exporting to be implemented first.
- [ ] **Platform-support:** Building for iOS and Android is still untested. The Android SDK provides its own CMake toolchain files and iOS is supported by both [upstream CMake](https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html#cross-compiling-for-ios-tvos-or-watchos) and the one shipped along with the iOS SDK out of the box. Therefore there should be no need to write platform nor toolchain files to support both platforms.
- [ ] **Tracy:** Tracy seems to now support being built using CMake and also has a CMake package config file of their own, however its usage is undocumented. Its setup is simple enough to be able to write a simple find module for it.
- [ ] **Revise the naming of the CMake options**
- [ ] **Implement automatic selection of the backend:** If the amount of code is considerable implement it in a separate `.cmake` file to improve conciseness and not to crowd the main `CMakeLists.txt` file in the Samples folder.
- [ ] **Implement creation of macOS framework bundles:** CMake already provides integration with macOS framework files. Contrary to what was done before, I would recommend building both the core library and the debugger library as separate frameworks.
- [ ] **Create CMake presets and initial cache files:** As discussed to ease building.
- [ ] **Document how to interface the new build project:** Likely the last step once the feature freeze has been reached. Mention as well some of the standard conventions of CMake like the fact that you can set up some of the build options [directly on the environment](https://cmake.org/cmake/help/latest/manual/cmake-env-variables.7.html), use CMake toolchain files in case the compiler they want to use isn't automatically detected by CMake like in Ubuntu, where many versions of the Clang compiler may be installed but `clang` doesn't always call the desired version as well as initial cache scripts. To aid, some templates could also be made so that consumers can take advantage of these without knowing about CMake.

[mikke89]

Thanks for continuing to work on this! I am a bit under the weather these days, but I'll get back to you soon enough.

[mikke89]

First of all, excellent work! Very high quality, I'm sure this will be very nice to work with for everyone, and a lot more approachable for new users. I'm looking forward to having this fully merged.

I have tested many different combinations of settings, and they generally seem to work very well for me. I only have some minor comments.

Thanks a lot for making such a comprehensive todo-list as well. I agree with everything on this list, no objections here.

In terms of naming, I'm leaning towards these guiding principles:

• Prefix with RMLUI_.
• Drop the prefix verb entirely.
• Do not name things with negation (such as 'not', 'disable'), to avoid situations with double negation.

This means names such as:

• RMLUI_SAMPLES
• RMLUI_LUA_BINDINGS
• RMLUI_THIRDPARTY_CONTAINERS

I believe this looks natural when used, e.g. RMLUI_PRECOMPILED_HEADERS=ON, and also makes finding suitable names much easier and more consistent.

---

You don't have to rebase. When we are both happy with this PR, I will squash and merge it into the cmake branch. We can continue working on it there for some time, and possibly invite more contributors. We should make it as stable as possible and hopefully quite close in terms of feature parity with what we have today, but not necessarily fully so. Generally, we don't really need to rebase, unless there are some library-wide changes being merged (the 'effects' branch being one).

Once we determine that things are working well enough, then I will merge it all to master. I will have to do that in a single commit, to avoid any transitive complications or issues, so all the commits will have to be squashed in the end. Then it will live in master for some time, give users time to report any new issues, before finally making a new release.

I hope this plan sounds good to you, let me know if you have any comments.

[hobyst]

@mikke89 I think I have applied all the discussed changes. Anything else?

[mikke89]

Very good! I'm happy to merge this into the cmake branch in this state, if you agree with this.

The new CMake code is very clean and modern. There is still quite a significant todo-list, many of which should be completed before merging into master, but this is an excellent start.

Once this is merged into cmake, I might make some additions, work on some of the todo-things, start adding some presets, experiment a bit with naming, and so on. Everyone is encouraged to continue contributing with new PRs or feedback. Finally, when its ready, we'll merge it into master.

[hobyst]

I'm ok with merging it as it is right now.

[mikke89]

Awesome work, thanks again!