-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix cmake presets naming chaos #2
fix cmake presets naming chaos #2
Conversation
preset names have it own scope. use simple and short names were possible
NOTE: tested only with cmake -S . --preset=unixlike-clang
cmake --build --preset=clang-release
cmake --build --preset=clang-release --target install
ctest --preset=clang-release |
}, | ||
{ | ||
"name": "config-windows-common", | ||
"name": "windows-common", |
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.
for the hidden presets that are used to compose the non-hidden presets (using the "inherits") I would prefer to keep the prefix type to prevent any misuse. Those hidden names are not visible to the end user anyway.
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.
configure, build,
and test
preets are independent namespaces!
you can't inherit a build preset
from a configure preset
AFAIK!
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.
thats' a good point. I changed them all in the most recent version!
@@ -0,0 +1,1313 @@ | |||
{ | |||
"$schema": "http://json-schema.org/draft-07/schema#", |
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.
Is this necessary? The "version" at the start of the preset file should be enough to define the correct schema.
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.
If the use want to change the User preset, this schema
file is needed to verfiy the preset.
cmake
does not help if the preset is invalid!
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 didn't know that. It should be very helpful for debugging. I am sorry but the time I noticed your PR there were just too many conflicts to resolve before the merge... Do you mind doing another PR with this file directly to the cpp_boilerplate_project? I am sorry again that I missed that.
"displayName": "Release", | ||
"description": "Set Strict rules for windows msvc (User Mode) release tests", | ||
"inherits": "test-common-release", | ||
"inherits": "common-release", |
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've done most of the changes you suggested in the most recent commits. If you have anything else in mind that will help future users, please comment in my PR to the upstream cpp_boilerplate_project cpp-best-practices#19 so we can merge it with other open cmakepresets issues we have.
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.
IMHO: to long to write on concole!
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: CMake discusion
I have tested this version, you have to test your version again on each host! |
preset names have it own scope.
use simple and short names were possible