🚸 Provide CMakePresets.json#1660
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds CMakePresets.json and updates CI/CD/upstream workflows to newer reusable workflow revisions; CI C++ jobs now use preset matrices and forward ChangesCMake Presets Adoption and CI Workflow Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CMakePresets.json`:
- Around line 119-159: Add a new test preset named "lint" to the testPresets
array so the preset chain is complete: create an entry with "name": "lint",
"inherits": "base", "configurePreset": "lint", and "configuration": "Debug" so
that `cmake --preset lint` works for tests as well (mirror the pattern used by
"debug"/"release" presets).
- Around line 23-32: Add a brief clarifying description to the "base-windows"
preset to document that it intentionally omits a "generator" and relies on
CMake's default (Visual Studio/MSVC) on Windows; update the preset named
"base-windows" (the JSON object with "name": "base-windows" and "inherits":
"base") to include a "description" field like "Uses default generator (Visual
Studio/MSVC) on Windows" so future maintainers understand the generator strategy
and its alignment with CI.
- Around line 124-127: The test base preset in CMakePresets.json currently sets
a universal retry via the "execution.repeat" block ("mode": "until-pass",
"count": 3) which masks flaky tests; update the preset by removing or changing
this retry behavior (remove the "repeat" block or switch to a less forgiving
mode like "until-fail" or "after-timeout") and if you keep retries, add an
inline comment next to "execution.repeat" explaining the rationale and add a
corresponding note in docs/contributing.md or README about known instability,
plus implement or hook up logging/tracking of retry events so every retry is
recorded for monitoring.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47470566-7f3e-4a19-9d2d-491784fa1a80
📒 Files selected for processing (2)
.github/workflows/ci.ymlCMakePresets.json
|
The changelog and upgrade guide still need to be updated, but I'll do that after we've settled on the presets. 😌 |
|
The documentation update is postponed to munich-quantum-toolkit/templates#302. |
This reverts commit 4ed0235.
|
@burgholzer, I have reverted 4ed0235 because it lead to a build error on I'd rather worry about this in a separate conversation before we hold up this PR. 😬 |
|
Fairly sure that this is us either missing a link library or an include somewhere. |
burgholzer
left a comment
There was a problem hiding this comment.
This generally LGTM 👍🏼
I left two comments.
What I was still wondering is whether we would want to have a little documentation for the presets that explains when to use which one (e.g. lint). But this can wait for a follow-up PR.
This should be practically close to merge-ready.
Once the new workflows are released, we should be able to get this in.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 136-142: The extensive Windows CI job cpp-tests-extensive-windows
does not propagate the mlir-debug flag to setup-mlir; update its with: block to
include mlir-debug: ${{ matrix.preset == 'debug-windows' }} so setup-mlir will
use the debug MLIR build for the debug-windows preset (add the mlir-debug key
alongside runs-on, compiler, setup-mlir, llvm-version, and preset-name).
In `@UPGRADING.md`:
- Line 42: Replace the incorrect gerund phrase "to providing" with the
infinitive "to provide" in the new CMake presets sentence (the line that
currently reads "[CMake presets] have been added to providing a standardized and
reproducible way to configure builds across different platforms.") so it reads
"...have been added to provide a standardized and reproducible way to configure
builds across different platforms."
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca0ab246-14de-4cf7-962f-91039e6e2946
📒 Files selected for processing (6)
.github/workflows/cd.yml.github/workflows/ci.yml.github/workflows/upstream.ymlCHANGELOG.mdCMakePresets.jsonUPGRADING.md
burgholzer
left a comment
There was a problem hiding this comment.
Nice!
Do we want to backport this to the v3.x branch? Probably, right?
I'd say so! |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
## Description This PR defines a [`CMakePresets.json`](https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html) and updates the CI to utilize the defined presets. Fixes #1622 ## Checklist - [x] The pull request only contains commits that are focused and relevant to this change. - [x] I have added appropriate tests that cover the new/changed functionality. - [x] ~~I have updated the documentation to reflect these changes.~~ - [x] I have added entries to the changelog for any noteworthy additions, changes, fixes, or removals. - [x] I have added migration instructions to the upgrade guide (if needed). - [x] The changes follow the project's style guidelines and introduce no new warnings. - [x] The changes are fully tested and pass the CI checks. - [x] I have reviewed my own code changes. (cherry picked from commit a4c20b9) --------- Co-authored-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com>
Description
This PR defines a
CMakePresets.jsonand updates the CI to utilize the defined presets.Fixes #1622
Checklist
I have updated the documentation to reflect these changes.