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

restructure cmake workflow presets #62

Merged

Conversation

ClausKlein
Copy link
Collaborator

@ClausKlein ClausKlein commented Nov 2, 2024

Add codespell config file and fix some typos too

Prevent build errors with gcc on Darwin (macOS) CI

Cleanup the cmake files

Add the cmake workflow presets for Windows too.

@ClausKlein ClausKlein marked this pull request as draft November 2, 2024 17:27
@ClausKlein
Copy link
Collaborator Author

@dietmarkuehl Would you like presets like this https://github.com/friendlyanon/cmake-init-shared-static/blob/master/CMakePresets.json

I would prefer this one: https://github.com/ClausKlein/cpp-lib-template/blob/develop/CMakePresets.json

As toolchain for Windows CI build with ninja normally I use the WindowsToolchain as git submodule or as copy?

@dietmarkuehl
Copy link
Collaborator

@dietmarkuehl Would you like presets like this https://github.com/friendlyanon/cmake-init-shared-static/blob/master/CMakePresets.json

I would prefer this one: https://github.com/ClausKlein/cpp-lib-template/blob/develop/CMakePresets.json

As toolchain for Windows CI build with ninja normally I use the WindowsToolchain as git submodule or as copy?

@ClausKlein I don't have an opinion on the presets! The ones I have were just copied from beman.exemplar when I created the repository. I don't think I normally build using the presets, either (it is my understanding your previous PRs changed that). At some point I should probably understand what they are and how they work.

In general, I think Beman projects should follow what beman.exemplar does. I like consistency. There may be some reasons to deviate. On the other hand, if things are done consistently it is easier to layer additional tool on top. For example, I know there is a desire to package things with Conan - another tool I haven't used and I don't know what it actually does.

@dietmarkuehl
Copy link
Collaborator

It seems the delete(message) is causing grief with LLVM 19: I wouldn’t object to disabling the conditional check using the feature test macro for now. I can restore that in a different PR.

Use 'Ninja Multi-Config' with Windows preset
@ClausKlein ClausKlein force-pushed the feature/prepare-cmake-workflow-presets branch 2 times, most recently from 1c56ae8 to 269b880 Compare November 4, 2024 20:50
@ClausKlein ClausKlein force-pushed the feature/prepare-cmake-workflow-presets branch from 336759b to be46000 Compare November 4, 2024 21:07
@ClausKlein ClausKlein marked this pull request as ready for review November 4, 2024 21:15
Changed the code to disable use of C++26 features. The way to restore that is probably adding/removing warning suppression in `suppress_push.hpp` and `suppress_pop.hpp` but I think this PR isn't the place to do that.
... it seems to cause grief. Again, something to get restored. I tried to replicate this problem and I didn't manage to do so.
@ClausKlein ClausKlein force-pushed the feature/prepare-cmake-workflow-presets branch from c2099e0 to 64ea7c4 Compare November 5, 2024 07:09
Copy link
Collaborator

@dietmarkuehl dietmarkuehl left a comment

Choose a reason for hiding this comment

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

First off: thank you very much for this work! There are number of things here I wanted to do for a while but didn't get to do, yet!

I didn't spot anything critical. There are a few comments but I'd be OK to land this fairly large PR and possibly address them later. The only one I consider really odd is the use of sudo sudo [without a user] although I don't think it causes any harm.

It seems the presets stuff needs fairly recent versions of cmake, certainly a more recent version than on the Debian 12 Linux I'm using (it has cmake 3.25 installed). Using a non-presets build does work, though.

As there are some comments, I won't merge the PR: feel free to merge (or let me know if you can't or want me to merge).

WarningsAsErrors: 'clang*'
FormatStyle: file

CheckOptions:
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I read that correctly, the readability options are somewhat at odd with C++ standard library naming. However, the previous section seems to have disabled all of these for now.

Having a clang-tidy config is good - it will likely require some clean-up, probably discover bugs.

@@ -29,8 +29,8 @@ jobs:

matrix:
sanitizer: [debug, release]
# TODO: compiler: [g++-14, clang++-19]
compiler: [clang++-18]
# TODO: compiler: [g++, clang++-19]
Copy link
Collaborator

Choose a reason for hiding this comment

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

The TODO mentions clang++-19 instead of clang++-18 - otherwise it could go . Maybe it can still go.

Comment on lines +2 to +7
"version": 9,
"cmakeMinimumRequired": {
"major": 3,
"minor": 30,
"patch": 0
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

These recent versions prevent me from building on Debian 12 which installs cmake version 3.25.1. I'd think others would have the same problem.

@dietmarkuehl
Copy link
Collaborator

Looks good - can this PR be merged?

Copy link
Collaborator

@dietmarkuehl dietmarkuehl left a comment

Choose a reason for hiding this comment

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

Looks good

@ClausKlein ClausKlein merged commit a760a29 into bemanproject:main Nov 7, 2024
9 checks passed
@ClausKlein ClausKlein deleted the feature/prepare-cmake-workflow-presets branch November 16, 2024 03:50
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.

2 participants