-
Notifications
You must be signed in to change notification settings - Fork 16
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
Create CMake preset #44
Conversation
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.
You can use variables like CMAKE_CXX_FLAGS_Debug_INIT
to tune what a Debug
or Release
build entails, for what it's worth. It's maybe a little nicer to use those variables instead of CMAKE_CXX_FLAGS
. But not a huge deal.
https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS_CONFIG_INIT.html
It could be a separate PR, but I expect that especially for introducing a new, friendly workflow, we'll want to make sure we have the workflow documented for potential contributors and users. |
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 tried this on an ubuntu machine and it's a good improvement.
In testing this I used cmake --list-presets
, cmake --workflow --preset gcc-release
, and cmake --workflow --preset gcc-debug
. All worked as expected on an Ubuntu VM.
I tried this on MacOS and got an error. The reason is that MacOS has a gcc
executable in the path that is actually clang under the hood...one that doesn't support the -fsanitize=leak
option. Clang presets will help mitigate this issue, but it's good to be aware of in case we want to attempt better error messages in the future.
Thank you for looking over this PR. Should I include this in the documentation? |
I don't think that's necessary. Let's see if others run into the same issue first. It feels kinda niche to me, especially if we include a clang preset. |
What would that workflow be different from |
Noted! I've updated the documentation. I could add clang-debug/release to this pr too, or I could also do this via another PR. |
{ | ||
"name": "gcc-debug", | ||
"inherits": "_test_base", | ||
"configurePreset": "gcc-debug" |
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.
Not for this PR, but it just occurred to me that we might leverage the ninja-multiconfig generator to keep the list of configurePresets small? That is have one that configures Debug, Release, and possibly various sanitizer flavors? This might help keep the combinatorics down.
cmake -B build -S . -DCMAKE_CXX_STANDARD=20 | ||
cmake --build build | ||
ctest --test-dir build | ||
cmake --workflow --preset gcc-debug | ||
cmake --install build --prefix /opt/beman.exemplar | ||
``` | ||
|
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.
Note that the "verbose logs" sections below need to be updated as well.
On the other hand, I'd be happier to see them removed entirely. I've used beman.exemplar for a few different projects now and it's a pain to have to update those sections.
The "Disable tests build" section also needs to be updated.
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.
Thank you for double checking! Sorry I missed those sections. I will have them updated later today. !!
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.
Updated cmake command and output in 8430517 .
Co-authored-by: David Sankel <[email protected]>
In the README, we includes support for turning off build test. However, How should I update the documentations?
@camio , cc @bretbrownjr as that paragraph was introduced in #15 . |
This looks like it's ready to close? |
There's a pending documentation change, see above comment. |
There's a lot of PR's open here so hopefully we can close them soon :) |
#48 should be ready, can you review and merge it? |
I think this is ready to merge.
I think this is a good idea. Can be done in a different PR.
I don't think we need a preset for this as it is a specialized need.
I think this only needs to be discussed in a "how to incorporate in another project" section which needs more work anyway (e.g. a FetchContent example) |
closes #17 .
This is a work in process seeking feedback, currently only included gcc related workflows.
Needs feedback on which compiler and what flags I should include/ exclude.
Let's keep the list of presets short (#17 (comment)) so we don't strain the CI system too much.
I omitted
-Werror
in #5 as this flag isn't indicated elsewhere (e.g. README).There's two workflow presets:
gcc-debug
, (-g
with all non-conflicting sanitizer included, see: Add CMake preset support #17 (comment))gcc-release
, (-g
with-O3
)Assumes C++ 20 per: #38 (comment) , but will monitor the progress happening at #38 . Please forward minimum version discussion to that thread unless it should be specific to a workflow.
CI test is only meant to test if the workflow config is functional, main testing should be done in the main build & test matrix, thus there's no cross-platform test. We could merge the two but that will need future discussion.
This is also a good peak into docker-less build & test for CI.
Please test this locally, I have experienced the test process hanging, possibly due to the excessive amount of sanitizer enabled. We may need to include a warning in the README to advise turning off/ splitting specific performance-impacting sanitizer on bigger projects.
Related pr: #7