Skip to content

CI: Add format check - #42

Merged
michalhosna merged 2 commits into
mainfrom
mh/format-ci
Mar 10, 2026
Merged

CI: Add format check#42
michalhosna merged 2 commits into
mainfrom
mh/format-ci

Conversation

@michalhosna

@michalhosna michalhosna commented Mar 9, 2026

Copy link
Copy Markdown
Member

Add --check flag to scripts/format.sh rather than calling clang-format
directly in ci.yml, so the set of matched files stays consistent between
local formatting and CI verification.

Replace rg with find in the script to avoid requiring ripgrep in CI.

Rename workflow from "build" to "ci" to match the filename and reflect
that it now covers more than building.

The format job uses ubuntu-24.04 (unlike the build jobs) to get a
reasonably modern clang-format, even though we target older compilers
for building.


This change is Reviewable

@michalhosna
michalhosna marked this pull request as draft March 9, 2026 14:40
@michalhosna
michalhosna force-pushed the mh/format-ci branch 2 times, most recently from 3d9ff1b to 23f57af Compare March 9, 2026 15:39
@michalhosna

Copy link
Copy Markdown
Member Author

+reviewer:@gmarzot +reviewer:@afrind

@michalhosna
michalhosna requested a review from gmarzot March 9, 2026 15:41
@michalhosna
michalhosna marked this pull request as ready for review March 9, 2026 15:42

@afrind afrind left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we make format.sh a commit hook too?

@afrind reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on gmarzot).

@michalhosna michalhosna mentioned this pull request Mar 9, 2026

@michalhosna michalhosna left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Git commit hooks are local only (AFAIK).

We can commit something like .githooks/ (no special git meaning, just a normal folder), and everyone must locally set that as a hook folder git config core.hooksPath .githooks.

But then you cannot add your local Git hooks easily. There are tools that can manage that, and you can write a local wrapper to merge .githooks and your hooks.

Given that there is no fully automated out-of-the-box way. I would suggest everyone set their hooks; that's easy with the scripts we already provide:

ln -s ../../scripts/format.sh .git/hooks/pre-commit

Should I add that to a readme? (Do people read that?)

@michalhosna made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on gmarzot).

@gmarzot

gmarzot commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

.github/workflows/ci.yml line 1 at r1 (raw file):

name: ci

objection to "ci" it is too general (so was build and not complete) - i spent some time on picking descriptive names for workflow files, actions and job names. check out the latest and we can discuss

@gmarzot

gmarzot commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

.github/workflows/ci.yml line 20 at r1 (raw file):

      - name: Check formatting
        run: bash scripts/format.sh --check

lgtm

@gmarzot

gmarzot commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

.github/workflows/ci.yml line 1 at r1 (raw file):

Previously, gmarzot (Giovanni Marzot) wrote…

objection to "ci" it is too general (so was build and not complete) - i spent some time on picking descriptive names for workflow files, actions and job names. check out the latest and we can discuss

Still there. ci.yml exists on main in o-rly. PR #40 renames the workflow name inside the file from build to verify, but the filename itself stays ci.yml.

In moxygen we renamed the file (omoq-ci.ymlomoq-verify.yml). In o-rly the file was never renamed.

@gmarzot

gmarzot commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

.github/workflows/ci.yml line 1 at r1 (raw file):

Previously, gmarzot (Giovanni Marzot) wrote…

Still there. ci.yml exists on main in o-rly. PR #40 renames the workflow name inside the file from build to verify, but the filename itself stays ci.yml.

In moxygen we renamed the file (omoq-ci.ymlomoq-verify.yml). In o-rly the file was never renamed.

hoping we can follow the pattern from moxygen

@michalhosna
michalhosna marked this pull request as draft March 9, 2026 17:57
@michalhosna
michalhosna force-pushed the mh/format-ci branch 2 times, most recently from d084c6c to 6e28717 Compare March 9, 2026 18:12
Add --check flag to scripts/format.sh rather than calling clang-format
directly in ci.yml, so the set of matched files stays consistent between
local formatting and CI verification.

Replace rg with find in the script to avoid requiring ripgrep in CI.

The format job runs in a debian:trixie container to pin clang-format 19.
The script checks the version at startup so local dev catches mismatches
early.

Version 19 is not a particularly specific choice — it ships in Debian
stable, Homebrew, and EPEL, so it is widely available. It is recent
enough to support the style options we use but not the newest release.

@michalhosna michalhosna left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@michalhosna made 2 comments, resolved 1 discussion, and dismissed @gmarzot from a discussion.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on afrind and gmarzot).


.github/workflows/ci.yml line 1 at r1 (raw file):
Reverted.

Still there.
We yeah, I didn't say its fixed between your two comments. Not sure where the confusion is coming from. Did I do something wrong?


a discussion (no related file):
Okay, clang-format breaks stuff with every version :(. So we need to fix on one.
I didn't go with what's in ubuntu-22, as that's fairly old and not supporting stuff we currently have in .clang-format. I also didn't really want to go with cutting-edge if it isn't easily avaible somewhere. 19. seems like a helthy middle ground.

@afrind afrind left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure, just tell me and my agent what to do

@afrind reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on gmarzot).

@gmarzot

gmarzot commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

.github/workflows/ci.yml line 1 at r1 (raw file):

Previously, michalhosna (Michal Hošna) wrote…

Reverted.

Still there.
We yeah, I didn't say its fixed between your two comments. Not sure where the confusion is coming from. Did I do something wrong?

no just over explaining

@gmarzot gmarzot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@gmarzot reviewed 3 files and all commit messages, and resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on michalhosna).

@michalhosna
michalhosna marked this pull request as ready for review March 10, 2026 10:18
@michalhosna
michalhosna merged commit 4eae426 into main Mar 10, 2026
6 checks passed
@michalhosna
michalhosna deleted the mh/format-ci branch March 10, 2026 10:36
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.

3 participants