Skip to content

[ci] Fail CI if missing override instead of just warning #19092

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

Merged
merged 4 commits into from
Jul 2, 2025

Conversation

ferdymercury
Copy link
Collaborator

@ferdymercury ferdymercury commented Jun 19, 2025

Currently, many override warnings remain unnoticed since the gcc-problem-matcher of the GitHub CI does not work correctly, and it's also easy to forget those annotations if you do not visit the "Files changed" tab.

If some devs later use dev=ON and testing=ON, this leads to failures for compilers that auto-enable -Winconsistent-missing-override.

To prevent this, we enable the suggest-override warning in the build of debian with dev=ON instead of in the one from alma, this way we are sure of always failing if some does a PR that forgets an override, forcing pull-requesters to fix their issues before merging.

Within this PR: also fix missing overrides uncovered by turning ON this flag in the dev=ON build.

@ferdymercury
Copy link
Collaborator Author

@pcanal I see a fatal error: module file '/github/home/ROOT-CI/build/lib/MultiProc.pcm' is out of date and needs to be rebuilt: module file out of date in https://github.com/root-project/root/actions/runs/15753057412/job/44402174612?pr=19092

Copy link

github-actions bot commented Jun 19, 2025

Test Results

    20 files      20 suites   3d 8h 43m 57s ⏱️
 3 091 tests  3 091 ✅ 0 💤 0 ❌
60 219 runs  60 219 ✅ 0 💤 0 ❌

Results for commit 0c8d97e.

♻️ This comment has been updated with latest results.

@ferdymercury ferdymercury requested review from linev and guitargeek June 19, 2025 12:23
@ferdymercury ferdymercury reopened this Jun 19, 2025
@ferdymercury ferdymercury requested a review from pcanal June 19, 2025 21:12
@linev
Copy link
Member

linev commented Jun 23, 2025

Yes, this is also my observation.
But normally such error appears only when converting from old Makefiles to cmake-based testing.
Then normal compiled options are applied - and on some CI platforms warnings automatically causing errors.

@ferdymercury
Copy link
Collaborator Author

Thanks @linev for the feedback.

Btw, do you approve the changes in roottest/io ?

@linev
Copy link
Member

linev commented Jun 24, 2025

Btw, do you approve the changes in roottest/io ?

For this part clearly Philippe @pcanal is responsible

@pcanal pcanal closed this Jul 1, 2025
@pcanal pcanal reopened this Jul 1, 2025
Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

Thank you!

@pcanal pcanal merged commit af9cda8 into root-project:master Jul 2, 2025
45 of 49 checks passed
@ferdymercury ferdymercury deleted the patch-7 branch July 2, 2025 09:45
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.

4 participants