🚸 Adapt C++ workflows to require CMake presets#363
Conversation
4869ed9 to
f0fdd1f
Compare
burgholzer
left a comment
There was a problem hiding this comment.
I really like how clean this has become!
This is only missing a changelog entry, and upgrade guide note, and the actual release preparation. I think it would be fine to already do that as part of this PR.
The title could also use an update since it is now no longer just support, but we switched to CMake presets for the C++ workflows.
Everyone using the C++ workflows now has to provide such presets.
Is that enough cause for this to actually be a major version?
cmake-presets
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Done.
Also done. I agree that it makes sense to make this a major release. 😌 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR migrates reusable C++ GitHub Actions workflows to require CMake presets ( ChangesCMake Presets Migration for Reusable C++ Workflows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 5
🤖 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/reusable-cpp-linter.yml:
- Around line 160-161: The "Build" step currently always runs the cmake build
command (cmake --build --preset lint) which ignores the workflow's build gating;
update the "Build" step to respect the reusable workflow input by adding a
conditional (e.g., if: ${{ inputs.build-project == 'true' }} or the repository's
equivalent) so the step only executes when build-project is enabled, keeping the
step name "Build" and the build command unchanged.
In @.github/workflows/reusable-cpp-tests-macos.yml:
- Around line 44-47: The workflow input "preset-name" is currently optional
because it has default: ""; make it required at the workflow boundary by
removing the default and adding required: true to the input definition so
callers must supply it; keep the existing description and type fields and ensure
the input block for preset-name contains description, type: string, and
required: true.
In @.github/workflows/reusable-cpp-tests-ubuntu.yml:
- Around line 44-47: The workflow input "preset-name" currently has a default
empty string which allows invalid runtime calls; update the "preset-name" input
definition (symbol: preset-name) to make it required by removing the default and
adding required: true so the workflow fails fast on missing value and prevents
executing cmake --preset "".
In @.github/workflows/reusable-cpp-tests-windows.yml:
- Around line 42-45: The input "preset-name" is currently optional due to
`default: ""`; make it required so callers fail at validation time by removing
the `default` key (or its empty value) and adding `required: true` under the
`preset-name` input definition in the workflow; update the `description`/`type`
block for `preset-name` and ensure `required: true` is present so workflow-call
validation enforces the input.
- Line 84: Replace the brittle substring check that sets the debug output
("debug: ${{ contains(inputs.preset-name, 'debug') }}") with an explicit,
canonical flag or deterministic mapping: add or use a dedicated input like
inputs.mlir-debug (boolean) or normalize and compare inputs.preset-name against
a whitelist of debug preset names (e.g., toLower(inputs.preset-name) == 'debug'
or membership check against known debug presets), and then set debug using that
explicit input (e.g., "debug: ${{ inputs.mlir-debug }}" or the whitelist
expression) so MLIR debug mode is not inferred by substring matching.
🪄 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: f12b951c-4d0c-4592-b161-1e95abe96a8d
📒 Files selected for processing (7)
.github/workflows/reusable-cpp-coverage.yml.github/workflows/reusable-cpp-linter.yml.github/workflows/reusable-cpp-tests-macos.yml.github/workflows/reusable-cpp-tests-ubuntu.yml.github/workflows/reusable-cpp-tests-windows.ymlCHANGELOG.mdUPGRADING.md
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
burgholzer
left a comment
There was a problem hiding this comment.
Looks almost ready to go in.
What I am wondering as well: Since this is going to be a major release now; are there any other things that we would want to get in that would be considered majorly breaking?
cmake-presetsCo-authored-by: Lukas Burgholzer <burgholzer@me.com> Signed-off-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com>
I honestly cannot think of anything. 🤔 |
burgholzer
left a comment
There was a problem hiding this comment.
Nice! One more suggestion to go, then this can go in and we can release 😌
Probably worth running renovate once more before the release, but that should be it then.
Co-authored-by: Lukas Burgholzer <burgholzer@me.com> Signed-off-by: Daniel Haag <121057143+denialhaag@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@UPGRADING.md`:
- Line 12: The documentation mentions the wrong flag name; update UPGRADING.md
to use the actual workflow input name `mlir-debug` (to match the input in
reusable-cpp-tests-windows.yml) so examples and migration notes reference
`mlir-debug` instead of `debug-mlir`, ensuring consistency between UPGRADING.md
and the reusable-cpp-tests-windows.yml workflow input.
🪄 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: aa256d2d-f080-4cc2-b265-f9234054c683
📒 Files selected for processing (6)
.github/workflows/reusable-cpp-linter.yml.github/workflows/reusable-cpp-tests-macos.yml.github/workflows/reusable-cpp-tests-ubuntu.yml.github/workflows/reusable-cpp-tests-windows.ymlCHANGELOG.mdUPGRADING.md
CodeRabbit did not approve the PR after resolving its final comment
Description
This PR adapts the C++ workflows to require CMake presets.
Fixes #352
Checklist
I have added appropriate tests that cover the new/changed functionality.I have updated the documentation to reflect these changes.