Skip to content
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

Add CMake Presets #1209

Merged
merged 10 commits into from
Oct 29, 2021
Merged

Add CMake Presets #1209

merged 10 commits into from
Oct 29, 2021

Conversation

sweemer
Copy link
Contributor

@sweemer sweemer commented Oct 11, 2021

Closes #1207.

To start with, it includes the following presets:

  • Linux with GCC
  • Linux with Clang
  • Windows with MSVC (x64)
  • Windows with Clang (x64)

Unfortunately I do not have a Mac, so was unable to add or test presets for that platform.

@boring-cyborg
Copy link

boring-cyborg bot commented Oct 11, 2021

Thanks for opening this pull request! It might take a while before we look at it, so don't worry if there seems to be no feedback. We'll get to it.

@CLAassistant
Copy link

CLAassistant commented Oct 11, 2021

CLA assistant check
All committers have signed the CLA.

@sweemer sweemer mentioned this pull request Oct 11, 2021
@coveralls
Copy link

coveralls commented Oct 11, 2021

Coverage Status

Coverage increased (+0.0003%) to 71.11% when pulling 632692f on sweemer:add-cmake-presets into d782f21 on lballabio:master.

@lballabio
Copy link
Owner

Hmm — frankly I'm a little less warm to the idea now that I see the presets spelled out. It looks like a lot of code, mostly restating defaults. And half of the cmake invocations in the .yml looked simpler before. What am I missing?

@sweemer
Copy link
Contributor Author

sweemer commented Oct 12, 2021

I agree that the CMakePresets.json format can be a bit verbose, especially with a lot of hidden presets the way I’ve structured it. The advantage of this approach is easy composability to new unhidden presets based on users’ requirements with minimal copy-paste. The alternative would be fewer hidden presets but perhaps less composability or reusability.

Regarding the yml file, the name of the preset can be shortened if that’s what you’re referring to. The advantage of using presets in the yml (to me at least) is less statefulness - all commands are run from the root directory as opposed to creating and changing build directories.

Last but not least, the killer feature of presets is being able to switch IDEs or even use command line tools with no additional cache configuration. This is a big help for cross platform projects such as quantlib.

Thanks for taking the time to look at the change and I’m open to any other comments you may have.

@lballabio
Copy link
Owner

Thanks, I see. If we can provide some useful reusable blocks, that's ok of course — I'm just not sure if we need to add presets like, for instance,

    {
      "name": "linux",
      "hidden": true,
      "inherits": "base",
      "generator": "Unix Makefiles"
    },

that (as far as I can see) only contains defaults that you would get even without specifying any presets. (windows-x64 looks like defaults, too? But I might be wrong.) I'd start simpler.

What would you think about starting with some presets that we would use for the more complex CI builds, for examples, without thinking too much about abstracting out basic pieces yet? We can add more as we find use cases.

Also, for the CI jobs without options, I'd leave the usual cmake .. ; cmake --build . commands, because that's probably what most people would try and I'd like CI to make sure it works.

What do you think?

@lballabio
Copy link
Owner

The four presets you mentioned in the PR description look useful, too — but maybe I wouldn't explode them over this many levels of inheritance...

@sweemer
Copy link
Contributor Author

sweemer commented Oct 12, 2021

Sure, I can collapse some of the inheritance if you prefer, but just be aware that this will probably result in a lot of duplicated content between related presets. For example, when you factor in the Debug and Release versions of the four presets I mentioned, you now have eight different presets that overlap with each other to some degree.

Maybe I can just remove all the presets except for two presets containing all the values required for the two 'options' CI jobs and you can see how it looks before we decide on the next steps.

Another idea would be to move all the layers of inheritance to CMakeUserPresets.json and keep CMakePresets.json mainly reserved for CI and examples. Normally the CMakeUserPresets.json file should be ignored by git, but in this case, I think an argument can be made for including it, in order to provide building blocks for users to create their own presets based on their own needs. The thing is that once there is a CMakePresets.json file in the project, IDEs like Visual Studio and Visual Studio Code will require you to select the desired configure and build preset before you can build the project, so you need at least a placeholder to get started. If we can provide that placeholder to users then I believe it would help them get their builds configured more quickly than if there were only a few CI related presets available to choose from.

For the CI jobs without options, I can certainly revert back to the original commands. Generally being able to specify a preset when building on command line is considered a benefit, but I can understand the desire to ensure that the traditional way of doing things still works for the majority of users.

@lballabio
Copy link
Owner

Hmm, adding CMakeUserPresets.json to source control wouldn't work — users are supposed to change it, and it would be a source of conflicts (and of things like "oops, your changes to CMakeUserPresets.json got included into your PR, would you mind reverting them?")

Would the four presets work without specifying "debug" or "release"? What would the default be, if any?

@sweemer
Copy link
Contributor Author

sweemer commented Oct 13, 2021

This is what the CMakePresets.json file looks like for the four combinations I mentioned at the top of this merge request along with Debug and Release configurations:

https://gist.github.com/sweemer/c86f205058ad4d3de54b33ad2afb827e

As you can see I also removed all the inheritance (i.e. hidden presets) so that you can evaluate each preset in isolation.

A few things to note:

  • The generator field is actually required with version 2 of the schema, so Unix Makefiles must be included for Linux presets. With version 3 of the schema, the generator field can "be omitted to fall back to regular generator discovery procedure".
  • According to the official documentation[1][2] version 3 of the schema is only supported in CMake version 3.21 and above. However it seems to work with CMake version 3.20.21032501-MSVC_2 bundled with VS 2019 (v16.11.4), and it's unclear to me whether it was backported specifically for VS 2019 or whether CMake 3.20 also supports version 3 of the schema - I haven't checked yet on Linux.
  • In any case, I think it's a bit dangerous to omit the generator field even with version 3 of the schema, because you basically have to know whether the default is a single-config generator or a multi-config generator. This is something I didn't fully grasp in my original merge request, but if you look at the gist, you can see that CMAKE_BUILD_TYPE is omitted in the configure preset for multi-config generators, and is included in the configuration field of the build preset instead. If you don't know whether the default generator found on your platform is single-config or multi-config then the preset probably will not work, or will produce code for the wrong configuration.
  • Likewise, I have explicitly specified the CMAKE_CXX_COMPILER on all Linux presets, because even though the default is probably g++, it might be configured to another compiler based on the user's environment, so it's best to be consistent.

Let me know what you think of the gist so that I can push new commits to the merge request accordingly.

[1] https://cmake.org/cmake/help/v3.20/manual/cmake-presets.7.html
[2] https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html

@lballabio
Copy link
Owner

Yup, I like this a lot better. Thanks!

@pkovacs
Copy link
Contributor

pkovacs commented Oct 13, 2021

I would use this preset-based approach for #1205 instead of hard-coding cmake settiings.

@lballabio
Copy link
Owner

Agreed.

@ralfkonrad
Copy link
Contributor

ralfkonrad commented Oct 13, 2021

I would use this preset-based approach for #1205 instead of hard-coding cmake settiings.

For me that's not idiomatic at all.

The default cmake..; cmake --build . should show the default warnings. Nothing more, but also nothing less. That's what #1205 is about.

@pkovacs
Copy link
Contributor

pkovacs commented Oct 13, 2021

With the preset functionality, I would go even further and remove all compile flags from the Platform.cmake file, reducing it to a nominal no-flag baseline, and then create a series of useful presets, adding flags via presets. I think @lballabio knows where we both stand on this point now -- let's just let him arbitrate.

@lballabio
Copy link
Owner

I'd try to make the experience smooth for people that just say "oh, there's a CMakeLists" and runs cmake .. ; cmake --build without a preset, so I'd keep in Platform.cmake whatever is required for that to work and create the library.

Other stuff can go into presets, or even user presets.

@sweemer
Copy link
Contributor Author

sweemer commented Oct 14, 2021

I've pushed up some more commits with the following changes:

  • Apply my 'inheritance-free' gist from yesterday
  • Add 'options' CI presets for Linux and Windows
  • Revert the 'non-options' CI jobs to no presets

Could you approve the checks workflows one more time for me @lballabio?

I am also now marking this pull request as ready for review. From my perspective, I'm fine with merging this as a first round of migrating existing build configurations over to presets. You mentioned possibly doing the same thing for some of the example projects, which I propose we do in a separate merge request.

Regarding a smooth user experience, I think you have two main types of users - those that use CMake on the command line, and those that use IDEs. I agree that the classic approach of cmake .. ; cmake --build is more natural to command line folks - especially those who have been using CMake for a long time - but at the same time, the presets approach seems to be quickly becoming the new standard. Both Visual Studio and Visual Studio Code have excellent support for presets, and anyone who opens up quantlib in one of those IDEs for the first time just has to select their preferred preset to be up and running. Anyway I think what we have now is a good start and I'm more than happy to help add or change presets based on the community's needs in the future.

@sweemer sweemer marked this pull request as ready for review October 14, 2021 13:17
@sweemer
Copy link
Contributor Author

sweemer commented Oct 23, 2021

Hi @lballabio, do you have any other comments before this change gets merged?

@lballabio lballabio added this to the 1.25 release milestone Oct 23, 2021
@lballabio
Copy link
Owner

No, thanks — I might try and think of a more explicit name for the CI builds, but if I do I can change it myself.

@lballabio lballabio merged commit 63b1392 into lballabio:master Oct 29, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Oct 29, 2021

Congratulations on your first merged pull request!

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

Successfully merging this pull request may close these issues.

Add CMake Presets
6 participants