bemanproject / exemplar

Example Beman project
Other
11 stars 16 forks source link

Create CMake preset #44

Closed wusatosi closed 1 month ago

wusatosi commented 1 month ago

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 (https://github.com/beman-project/exemplar/issues/17#issuecomment-2362899915) 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:

Assumes C++ 20 per: https://github.com/beman-project/exemplar/pull/38#issuecomment-2391421004 , 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

bretbrownjr commented 1 month ago

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.

wusatosi commented 1 month ago

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?

camio commented 1 month ago

I tried this on MacOS and got an error.

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.

wusatosi commented 1 month ago

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.

What would that workflow be different from gcc-debug?

wusatosi commented 1 month ago

[It's not necessary to document failure to run gcc-debug on macos due to symlinking gcc with clang]

Noted! I've updated the documentation. This pr is ready to merge if only this two presets is desired.

I could add clang-debug/release to this pr too, or I could also do this via another PR.

wusatosi commented 1 month ago

In the README, we includes support for turning off build test. However, cmake does not support passing additional parameters to workflows till (late) 3.25.

How should I update the documentations? Should I

  1. Remove the section entirely
  2. Add a no build test variant to the preset
  3. Indicate that workflow will not support this, and include commands to manually build the project with BUILD_TEST flag?

@camio , cc @bretbrownjr as that paragraph was introduced in #15 .

JeffGarland commented 1 month ago

This looks like it's ready to close?

wusatosi commented 1 month ago

This looks like it's ready to close?

There's a pending documentation change, see above comment.

JeffGarland commented 1 month ago

There's a lot of PR's open here so hopefully we can close them soon :)

wusatosi commented 1 month ago

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?

camio commented 1 month ago

I think this is ready to merge.

How should I update the documentations? Should I

  • Remove the section entirely

I think this is a good idea. Can be done in a different PR.

  • Add a no build test variant to the preset

I don't think we need a preset for this as it is a specialized need.

Indicate that workflow will not support this, and include commands to manually build the project with BUILD_TEST flag?

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)