Skip to content
This repository has been archived by the owner on Apr 19, 2023. It is now read-only.

Feature/presets enchancements #19

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Jason5480
Copy link
Contributor

@Jason5480 Jason5480 commented Apr 1, 2022

This PR contains several enhancement to the Cmake presets.

  • - Setting "Ninja Multi-Config" as the default generator should have several benefits for your IDEs (see also Feature/ninja multi presets support #15)
  • - Better structure of the hidden presets to facilitate better composability and extensibility!
  • - Due to those changes different modes can be added depending on the use case (developer, user, ci etc.)
  • - Add integration of the presets with the CI

Currently we support 4 toolchains +1 mode option for msvc (thus 5 non-hidden configure presets) and the corresponding Debug and Release builds (thus 10 non-hidden buildPresets and 10 testPresets)

Since this is getting bigger and bigger with each new configuration option added to the project the configurePresets are doubled in numbers (e.g. msvc + mode option -> msvc Developer Mode and msvc User Mode) and the buildPresets and testPresets are quadrupled.
So, at this point I have the following questions.

  1. Shall we consider adding a CMakeUserPresets.json file to that repository that has the default user settings for developing that project? What I have in mind is to let the default build options to the CMakePresets.json and move the user specific options (and modes) to the CMakeUserPresets.json.
  2. Should I also add support for RelWithDebInfo for completeness?
  3. Is it necessary to be able to run tests both in Debug and Release?
  4. Should the mode option be added in all supported toolchains currently it is only in msvc. gcc clang and clang-cl do not have that option
  5. What is the value of the 'ENABLE_DEVELOPER_MODE' if it is not defined?

@ddalcino @aminya @lefticus @oskidan some feedback would be welcome!
Also, @oskidan and @ddalcino can you check that those changes are still functional in macOS to make sure I didn't mess with #16

Co-Author: @ClausKlein Thank you for the feedback and your test in macOS that unfortunately cannot do.

@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2022

Codecov Report

Merging #19 (ab84504) into main (c77d33f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #19   +/-   ##
=======================================
  Coverage   77.77%   77.77%           
=======================================
  Files           3        3           
  Lines          36       36           
  Branches       19       19           
=======================================
  Hits           28       28           
  Misses          7        7           
  Partials        1        1           
Flag Coverage Δ
Linux 37.03% <ø> (ø)
Windows 80.00% <ø> (ø)
macOS 38.46% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c77d33f...ab84504. Read the comment docs.

@Jason5480
Copy link
Contributor Author

@ddalcino on gcc Debug build the CI failed to install cmake is anything wrong?

@Jason5480
Copy link
Contributor Author

Jason5480 commented Apr 6, 2022

@lefticus there are still some pending questions about the user/developer mode. And the combinatorial complexity of all the supported platforms, toolchains etc. Also, it was proposed by @ClausKlein to also support mingw and BSD. @oskidan if I am not mistaken you mentioned that BSD should be supported with the current unixlike presets. Am I right?

README_building.md Outdated Show resolved Hide resolved
@Jason5480 Jason5480 force-pushed the feature/presets-enchancements branch from 4bb8363 to 1ea2289 Compare April 6, 2022 13:45
CMakeUserPresets.json Outdated Show resolved Hide resolved
@ddalcino
Copy link
Contributor

ddalcino commented Apr 7, 2022

@ddalcino on gcc Debug build the CI failed to install cmake is anything wrong?

That looks like an issue with setup-cpp. Looks like it resolved itself though. If you're worried about it, you could file an issue. It's reasonable to expect that setup-cpp would either cache this dependency for us (our cache is turned on) or retry on error.

@ddalcino
Copy link
Contributor

ddalcino commented Apr 7, 2022

@lefticus there are still some pending questions about the user/developer mode. And the combinatorial complexity of all the supported platforms, toolchains etc. Also, it was proposed by @ClausKlein to also support mingw and BSD. @oskidan if I am not mistaken you mentioned that BSD should be supported with the current unixlike presets. Am I right?

We are deliberately not supporting mingw due to a long history of really nasty bugs. I thought we had discussed this somewhere else in the cpp_best_practices org, but I can't find it now. Relevant: https://twitter.com/lefticus/status/1484215081806098432

IMHO, we can only claim to support the platforms that we are testing in CI. We should try not to go out of our way to break the project on BSD, but I don't think we need to go out of our way to support it. If you support BSD, where do you draw the line? Solaris? ReactOS? FreeDOS? Redox? Serenity? There's just too many. Please don't forget about our Conan dependencies, many of which do not support any OSes other than win/mac/linux.

Let's not forget, the cmake presets are a convenience. Users can easily do without them, or write their own.

@Jason5480 Jason5480 requested a review from ddalcino April 7, 2022 13:00
@ClausKlein
Copy link
Contributor

ClausKlein commented Apr 7, 2022

@Jason5480 Hi Nikloas, we should have a separate forum to have such a diskusion.

If you want to use the presets in an IDE, you only need the configpresets, or not?

If you do not know, were the binary directory is, you need the buildpresets too on console.

And with the cmake --build builddirpath --taget test you need no testpreset!

Now I see why the presets are from microsoft

@Jason5480
Copy link
Contributor Author

Jason5480 commented Apr 8, 2022

If you want to use the presets in an IDE, you only need the configpresets, or not?

Now I see why the presets are from microsoft

2022-04-08 10_33_54-

Currently VSCode and VisualStudio support all three types of presets. CLion supports only build presets QtCreators integration is on its roadmap. For XCode I do not know but the key concept of the presets is that in the future you can have IDE agnostic codebases no need to support *.vsxpjoj files along with *.creator and other IDE specific files. You have one format to produce reproducible builds in all of them! And the command line options should not be excluded from this for development.

@Jason5480
Copy link
Contributor Author

If you do not know, were the binary directory is, you need the buildpresets too on console.

And with the cmake --build builddirpath --taget test you need no testpreset!
...

Command line options override the presets so you can pass your own binary and install dir through the cli. I just put the VSCode/VS defaults there in case you want to open the project that is already build before with another IDE it should be able to find existing build folder and not re-build all of them on its own (different) default folder

@ClausKlein
Copy link
Contributor

ClausKlein commented Apr 8, 2022

Currently VSCode and VisualStudio support all three types of presets. CLion supports only build presets QtCreators integration is on its roadmap. For XCode I do not know but the key concept of the presets is that in the future you can have IDE agnostic codebases no need to support *.vsxpjoj files along with *.creator and other IDE specific files. You have one format to produce reproducible builds in all of them! And the command line options should not be excluded from this for development.

In my world, I generate a build project (MVS, Xcode, Ninja Multi-Config, Eclipse, ...) from console with CMake.
For CI build, we build and test them too from a bash shell.

If needed, I open the generated IDE to develop or debug, ...

So I never need an build preset or a test preset.

In every IDE I ever used, the CMake generated targets all, test, clean, install, ... are available.

see https://discourse.cmake.org/t/cmake-presets-what-is-a-testpreset-for/5404

@Jason5480
Copy link
Contributor Author

Jason5480 commented Apr 8, 2022

In my world, I generate a build project (MVS, Xcode, Ninja Multi-Config, Eclipse, ...) from console with CMake. For CI build, we build and test them too from a bash shell.

If needed, I open the generated IDE to develop or debug, ...

So I never need an build preset or a test preset.

In every IDE I ever used, the CMake generated targets all, test, clean, install, ... are available.

see https://discourse.cmake.org/t/cmake-presets-what-is-a-testpreset-for/5404

Yes, this is the "old" way (better than the manual) of supporting several IDEs. With the presets the IDEs are getting closer to the CMake and not the other way around. Also, vendor specific options enable some customization points for the IDE without affecting others. Having different generators for each IDE is also an option but how do you make sure that project for Eclipse and VS have been generated/configured with the same cmake variables and have the same behavior? Well, you can inspect the cmake variables but when using presets the author makes sure that the correct variables are passed.
For IDEs I think that test presets are optional but I added them for ease of use and completeness. It is convenient to just click a Run Ctest button and get the results inside your IDE.
Now for the command line all presets are optional you can still pass -DENABLE_CLANG_TIDY_DEFAULT=false -DCMAKE_C_COMPILER=cl -DCMAKE_CXX_COMPILER=cl -DENABLE_DEVELOPER_MODE=OFF manually.
My point is that the minimum requirements are defined by the IDE integration and not by the cli usage. Otherwise, we could claim that presets are useless, then remove the preset file and we all use the command line.

@ClausKlein
Copy link
Contributor

My point is that the minimum requirements are defined by the IDE integration and not by the cli usage. Otherwise, we could claim that presets are useless, then remove the preset file and we all use the command line.

I love presets, at least the configurePrests, but not more!

@Jason5480
Copy link
Contributor Author

My point is that the minimum requirements are defined by the IDE integration and not by the cli usage. Otherwise, we could claim that presets are useless, then remove the preset file and we all use the command line.

I love presets, at least the configurePrests, but not more!

Well I agree that it is way simpler to just pass --build and --test option in the cli but I would also like to have the full integration in my IDE.
What really bugs me though is that I have to write 200 additional lines to achieve this and the duplication in build and test presets is so obvious. What I really want (and I should be able to write) is two buildPresets (Relese/Debug) that applies to all config presets and one testPreset.
The limitation here is that in the current schema the "configurePreset" variable cannot take a list. If that was possible then I could make the presets file way more concise. Since you searched a little bit more about the schema is it possible to change the limitation of the "configurePreset" of build and test Presets in a way that can be understood by the IDEs?

Copy link
Contributor

@ddalcino ddalcino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty good so far. I, like you, am a little disappointed that this couldn’t be shorter, but I don’t see a reasonable way to make that happen (other that generating this file with a script, and I’m not convinced we want that).

The only thing that this PR is really lacking is automated testing. I think it would be reasonable to add a separate build step to the .github/workflows/ci.yml file that runs cmake configure, build, and test using the appropriate presets. I don’t think anybody wants to test the presets manually on 3 different OSs over and over again. An automated CLI test should give us a really good idea of whether or not the tests will work on VS Code on a given platform.

With automated tests, I think this is an easy approval from me.

@Jason5480 Jason5480 requested review from lefticus and ddalcino April 16, 2022 18:05
@Jason5480
Copy link
Contributor Author

The only thing that this PR is really lacking is automated testing. I think it would be reasonable to add a separate build step to the .github/workflows/ci.yml file that runs cmake configure, build, and test using the appropriate presets. I don’t think anybody wants to test the presets manually on 3 different OSs over and over again. An automated CLI test should give us a really good idea of whether or not the tests will work on VS Code on a given platform.
...

Nice idea to add those presets to the CI I will try it!

@Jason5480 Jason5480 marked this pull request as draft April 18, 2022 07:19
@Jason5480 Jason5480 force-pushed the feature/presets-enchancements branch 3 times, most recently from 9fe24db to 26bcdf0 Compare April 18, 2022 13:37
Iason Nikolas added 6 commits May 22, 2022 17:17
Squashed commit:

[c64694d] Rename the linux presets to unixlike

[0a57a97] Add binaryDir and installDir to be used in case of cli

[ad5a3e7] Move mode options to CMakeUserPresets.json file

[1c0fa00] Make hidden preset for each supported compiler/toolchain and reuse them in not hidden presets using "inherits" list. This way we the future changes will be applied only to the preset that is responsible for that functionality.

[607a69f] Set "Ninja Multi-Config" generator as the default. Add buildPresets for Debug and Release. Modify testPresets accordingly

[a98c295] Create separate developer and user mode hidden presets and use inherits to compose them to the non-hidden presets

[e26f74c] replace conf- with config- in preset names

[802e988] Let the IDE to set the default binary and install directories
…so to prevent clutter (+1 squashed commits)

Squashed commits:

[7f3630d] Keep it simple remove unecessary 'build-` and 'test-` prefixes to make life easier as @KlausKlein suggested here cpp-best-practices#31 (comment)
Keep some naming conventions consistent (+1 squashed commits)

Squashed commits:

[8252d13] Update documentation

Keep some naming conventions consistent
@Jason5480 Jason5480 force-pushed the feature/presets-enchancements branch from e12ce1f to 7d1d29f Compare May 22, 2022 14:18
@Jason5480
Copy link
Contributor Author

This is looking pretty good so far. I, like you, am a little disappointed that this couldn’t be shorter, but I don’t see a reasonable way to make that happen (other that generating this file with a script, and I’m not convinced we want that).

The only thing that this PR is really lacking is automated testing. I think it would be reasonable to add a separate build step to the .github/workflows/ci.yml file that runs cmake configure, build, and test using the appropriate presets. I don’t think anybody wants to test the presets manually on 3 different OSs over and over again. An automated CLI test should give us a really good idea of whether or not the tests will work on VS Code on a given platform.

With automated tests, I think this is an easy approval from me.

It's been a long time since we discussed this but I was not able to pass the presets in the ci.yml and action.yml. The problem here is that the compiler, generator, build_type, and developer_mode settings are defined in the ci where this info should be read from the CMakePresets.json file instead if we want to support CMake 3.16. Maybe @aminya or @lefticus can help with that. Otherwise, I will revert the yml changes and I will open another issue for the CI part.

@ClausKlein
Copy link
Contributor

CMake v3.25 supports Workflow Presets, is this supported b this PR too?

see aminya/cpp_vcpkg_project#20

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants