-
Notifications
You must be signed in to change notification settings - Fork 79
[CMAKE][GITHUB] Simplify and consolidate CMakePresets for multi-config builds #1235
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
base: main
Are you sure you want to change the base?
Conversation
3485ab1
to
1883f2c
Compare
9405621
to
e334e47
Compare
e334e47
to
9631194
Compare
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.
I don't think we are going to see eye to eye on the subject of mixing build configuration with optimisation level.
The vcpkg stuff needs fixing at some point rather than removing too as it will be required for future development and we already have code merged into the code base that depends on it to build.
@@ -49,26 +48,29 @@ if(NOT IS_VS6_BUILD) | |||
target_compile_features(core_config INTERFACE cxx_std_20) | |||
endif() | |||
|
|||
target_compile_options(core_config INTERFACE ${RTS_FLAGS}) | |||
target_compile_options(core_config INTERFACE "/W3") |
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.
Don't hard code warning options, they differ between compilers. Better to pull this from an environment variable and set it either from presets or from the environment directly such as on the CI.
For example when cleaned up we might want the CI to error on warnings to stop them creeping back in, but don't want that hard coded as it might break builds on platforms we haven't tested on or on compilers not run on the CI.
if(RTS_BUILD_OPTION_PROFILE) | ||
target_compile_definitions(core_config INTERFACE RTS_PROFILE) | ||
endif() | ||
# otherwise DEBUG defines are tied to Debug generator config and PROFILE defines to RelWithDebug info generator config |
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.
Disagree with this, it prevents building an end user configuration without optimisation and breaks the current behaviour largely just for the benefit of Visual Studio users who don't build using the command line so they can build two seperate types of build without switching solution. Correct solultion is to have two solutions when developing in my opinion, one with developer instrumentation enabled and one without. This isn't being developed as just and MSVC solution, we aren't tied to the limitations of just that IDE anymore.
Summary
configurePresets
into a single"default"
plus generator/toolset-specific presets (ninja
,ninja-vc6
,vs2022
,vs2022-vc6
).ninja
andninja-vc6
presets remain separate to avoid requiring binaryDir deletion when switching compilers.vs2022
,vs2022-vc6
).vs2022-vc6
(toolset v60) requires Daffodil and VS2010 to be installed.buildPresets
: all build configuration logic is now handled in CI or at build time, not by maintaining a large preset matrix.Debug
,Release
, andRelWithDebInfo
at build time for each preset.cmakeMinimumRequired
(now set in top-levelCMakeLists.txt
).Benefits
Users can set debug/profile defines at configure time to decouple them from optimization level, or leave them unset and have them switch automatically at build time using generator configuration. This makes mixed setups (like release builds with debug defines) easy and explicit.