SFML dependency wrapper code cleanup #551
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
hobyst
commented
Dec 8, 2023
hobyst
commented
- Remove remnant of SFML <= 2.4 support
- Add prefix to variable names on loop
- Fix a typo in a comment
- Remove remnant of SFML <= 2.4 support - Add prefix to variable names on loop - Fix a typo in a comment
|
Thanks for the follow-up! |
mikke89
pushed a commit
that referenced
this pull request
Mar 31, 2024
- Remove remnant of SFML <= 2.4 support - Add prefix to variable names on loop - Fix a typo in a comment
mikke89
pushed a commit
that referenced
this pull request
Apr 6, 2024
- Remove remnant of SFML <= 2.4 support - Add prefix to variable names on loop - Fix a typo in a comment
mikke89
added a commit
that referenced
this pull request
May 7, 2024
Partial rewrite originally written by @hobyst in #446 and #551. Remaining code and changes written by @mikke89. These changes should generally be feature-complete with the previous CMake code, with some exceptions to legacy and certain Apple-specific behavior. Please see the changelog for breaking changes, in particular, several options, CMake targets, and file names have been changed. The following lists some noteworthy changes: - CMake presets now available. - Major changes to CI automation and tests. - Most tests on Appveyor are moved to Github actions. This includes a completely new packaging workflow on GitHub. - Packaging now uses Visual Studio 2019 instead of 2017. Keep Appveyor only for testing with Visual Studio 2017. - Use CMake scripts for some packaging tasks to avoid avoid duplicate workflow code. - `RmlUi::RmlUi` is now an include-all library target, while components like `RmlUi::Core` and `RmlUi::Debugger` can be chosen individually. - Add `contributing.md` guidelines for CMake. - Backends can now be changed without recompiling the whole library. - Set CMake `policy_max` to a recent version. This opts in to a number of CMake improvements for users that can take advantage of that. - Sample bitmapfont now available without FreeType. - Runtime outputs are now placed in the binary root directory. - Runtime dependencies can now be installed automatically, this includes dependencies from vcpkg. - Update .editorconfig and format all CMake files consistently. - Rename `harfbuzzshaping` sample to `harfbuzz`. - Fix harfbuzz compatibility for older versions. - Remove CMake warning on FreeType version. In the following, some working notes and lessons learned are written down (authored by @mikke89): - Avoid separate libraries for subparts of Core Originally, parts of core such as Elements, Layout and FontEngineDefault, were added as interface libraries. Defining these as interface libraries cause issues during export. I believe we would have to export them as separate targets, but that doesn't relly make sense from a client standpoint and causes new issues with exported source files and dependencies. It seems to be a lot less troublesome to use a single CMake library and set properties and attach sources to that directly. - Avoid interface libraries for plugins and Lua dependencies Using interface libraries to add plugins causes problems when installing targets. Even trying to split the interface libraries into private and public parts did not work suitably. An issue arises when compiling the project as static libraries. Consider when we are linking targets to rmlui_core. Even when they are added as private dependencies, CMake adds it as a transient link-time dependency. This even applies to interface dependencies, which causes problems for our private interface libraries representing the plugins. In particular, CMake wants us to export these libraries, as now they are referenced from rmlui_core, but we don't want to export them, as that was the whole point of making them private and would cause new issues. Instead, add the SVG and Lottie source files and imported dependencies directly to Core. This simplifies things quite a bit, and seems to work reliably. For similar reasons, use imported interface targets instead of a pure interface library for the custom Lua target. - Common options for CMake targets Using CMake presets to specify compiler flags, warnings in particular, is not a great experience. Presets require one to set all the flags at the same time (in a single variable). For example, we can't easily set just /WX in one profile and /MP in another. There are workarounds, but being more pragmatic, setting these flags in a cmake file is much more straightforward with proper conditionals. - On CMake presets I also experimented with more detailed, platform-dependent presets, but found that it is not really that useful. In particular, we want users themselves to use whichever compilers and generators they want to rather than to constrain the choices. We strive to be agnostict toward both generators and platforms. This way, we we can also give more common build instructions regardless of platform. Also tested with making presets for e.g. vcpkg and mingw toolchains. However, this doesn't really make sense in presets. First of all, the choice of such a toolchain is not mutually exclusive with the current presets, so we would have to replicate most of them for each toolchain we wanted to support. Further, the exact specifics of these toolcahins, like version, platform, install location, and so on, is very specific to each case. Thus, if we wanted to handle these it would create additional dimensions to the presets, and we'd end up with a huge list of similar presets we would have to mainatain and which users would have to choose from. Or alternatively, users would have to provide a lot of their setup in any case, thereby dismissing some of the initially considered usefulness. Instead, many tools already allow you to setup a toolchain first which can then be used with a given preset. Thus, making the choice of toolchains and cmake presets orthogonal makes a lot more sense. One unfortunate issue from a user-friendliness perspective is that the cmake gui doesn't allow you to select a preset without a specified generator. There's an issue for this here: https://gitlab.kitware.com/cmake/cmake/-/issues/23341 I figured accepting this for now is better than duplicating all our presets to specify different generators. But the situations is not ideal, hopefully it will be fixed soon. - Recommended compiler flags The compiler flags are enabled by default to ensure users get a good experience when building the library without fiddling with options. Especially on MSVC, the default compiler flags means really slow builds (no /MP), and we also risk exposing more warnings. Co-authored-by: Michael Ragazzon <michael.ragazzon@gmail.com>
mikke89
added a commit
that referenced
this pull request
May 7, 2024
Partial rewrite originally written by @hobyst in #446 and #551. Remaining code and changes written by @mikke89. These changes should generally be feature-complete with the previous CMake code, with some exceptions to legacy and certain Apple-specific behavior. Please see the changelog for breaking changes, in particular, several options, CMake targets, and file names have been changed. The following lists some noteworthy changes: - CMake presets now available. - Major changes to CI automation and tests. - Most tests on Appveyor are moved to Github actions. This includes a completely new packaging workflow on GitHub. - Packaging now uses Visual Studio 2019 instead of 2017. Keep Appveyor only for testing with Visual Studio 2017. - Use CMake scripts for some packaging tasks to avoid avoid duplicate workflow code. - `RmlUi::RmlUi` is now an include-all library target, while components like `RmlUi::Core` and `RmlUi::Debugger` can be chosen individually. - Add `contributing.md` guidelines for CMake. - Backends can now be changed without recompiling the whole library. - Set CMake `policy_max` to a recent version. This opts in to a number of CMake improvements for users that can take advantage of that. - Sample bitmapfont now available without FreeType. - Runtime outputs are now placed in the binary root directory. - Runtime dependencies can now be installed automatically, this includes dependencies from vcpkg. - Update .editorconfig and format all CMake files consistently. - Rename `harfbuzzshaping` sample to `harfbuzz`. - Fix harfbuzz compatibility for older versions. - Remove CMake warning on FreeType version. In the following, some working notes and lessons learned are written down (authored by @mikke89): - Avoid separate libraries for subparts of Core Originally, parts of core such as Elements, Layout and FontEngineDefault, were added as interface libraries. Defining these as interface libraries cause issues during export. I believe we would have to export them as separate targets, but that doesn't relly make sense from a client standpoint and causes new issues with exported source files and dependencies. It seems to be a lot less troublesome to use a single CMake library and set properties and attach sources to that directly. - Avoid interface libraries for plugins and Lua dependencies Using interface libraries to add plugins causes problems when installing targets. Even trying to split the interface libraries into private and public parts did not work suitably. An issue arises when compiling the project as static libraries. Consider when we are linking targets to rmlui_core. Even when they are added as private dependencies, CMake adds it as a transient link-time dependency. This even applies to interface dependencies, which causes problems for our private interface libraries representing the plugins. In particular, CMake wants us to export these libraries, as now they are referenced from rmlui_core, but we don't want to export them, as that was the whole point of making them private and would cause new issues. Instead, add the SVG and Lottie source files and imported dependencies directly to Core. This simplifies things quite a bit, and seems to work reliably. For similar reasons, use imported interface targets instead of a pure interface library for the custom Lua target. - Common options for CMake targets Using CMake presets to specify compiler flags, warnings in particular, is not a great experience. Presets require one to set all the flags at the same time (in a single variable). For example, we can't easily set just /WX in one profile and /MP in another. There are workarounds, but being more pragmatic, setting these flags in a cmake file is much more straightforward with proper conditionals. - On CMake presets I also experimented with more detailed, platform-dependent presets, but found that it is not really that useful. In particular, we want users themselves to use whichever compilers and generators they want to rather than to constrain the choices. We strive to be agnostict toward both generators and platforms. This way, we we can also give more common build instructions regardless of platform. Also tested with making presets for e.g. vcpkg and mingw toolchains. However, this doesn't really make sense in presets. First of all, the choice of such a toolchain is not mutually exclusive with the current presets, so we would have to replicate most of them for each toolchain we wanted to support. Further, the exact specifics of these toolcahins, like version, platform, install location, and so on, is very specific to each case. Thus, if we wanted to handle these it would create additional dimensions to the presets, and we'd end up with a huge list of similar presets we would have to mainatain and which users would have to choose from. Or alternatively, users would have to provide a lot of their setup in any case, thereby dismissing some of the initially considered usefulness. Instead, many tools already allow you to setup a toolchain first which can then be used with a given preset. Thus, making the choice of toolchains and cmake presets orthogonal makes a lot more sense. One unfortunate issue from a user-friendliness perspective is that the cmake gui doesn't allow you to select a preset without a specified generator. There's an issue for this here: https://gitlab.kitware.com/cmake/cmake/-/issues/23341 I figured accepting this for now is better than duplicating all our presets to specify different generators. But the situations is not ideal, hopefully it will be fixed soon. - Recommended compiler flags The compiler flags are enabled by default to ensure users get a good experience when building the library without fiddling with options. Especially on MSVC, the default compiler flags means really slow builds (no /MP), and we also risk exposing more warnings. Co-authored-by: Michael Ragazzon <michael.ragazzon@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.