Skip to content

[AIROCMLIR-197] Move C++ format/tidy checks to GitHub Actions#2356

Open
bogdan-petkovic wants to merge 18 commits intoROCm:developfrom
bogdan-petkovic:bogdan-petkovic/move-lint-to-gh-actions
Open

[AIROCMLIR-197] Move C++ format/tidy checks to GitHub Actions#2356
bogdan-petkovic wants to merge 18 commits intoROCm:developfrom
bogdan-petkovic:bogdan-petkovic/move-lint-to-gh-actions

Conversation

@bogdan-petkovic
Copy link
Copy Markdown
Contributor

@bogdan-petkovic bogdan-petkovic commented Apr 22, 2026

Motivation

Move C/C++ formatting and diff-based clang-tidy checks out of Jenkins so GPU CI isn't blocked by style issues, and enforce these checks earlier/cheaper in GitHub Actions.

Technical Details

  • Add a new GitHub Actions job (cpp-static-checks) that runs the existing Jenkins premerge script mlir/utils/jenkins/static-checks/premerge-checks.py inside the rocm/mlir:rocm7.2-latest container, reusing the same tooling and --ignore-external flag as Jenkins.
  • Generate build/compile_commands.json via CMake configure (-DCMAKE_EXPORT_COMPILE_COMMANDS=ON, -DMLIR_ENABLE_ROCM_RUNNER=1) and symlink compile_commands.json as expected by the script.
  • Add caching for the build/ directory to reduce repeated configure cost on PR updates.
  • Job runs only on pull_request events to avoid no-op diffs on direct pushes to develop.
  • No changes to Jenkinsfile — Jenkins preMergeCheck continues to run in parallel during the validation period.
  • Simplify local git hooks: consolidate all C/C++ formatting into .githooks/pre-commit

Test Plan

  • Validate the GitHub Actions workflow runs premerge-checks.py (clang-format diff + clang-tidy-diff) against the PR base ref.
  • Confirm Jenkins continues to execute preMergeCheck() on the GPU PR lane unchanged.
  • Confirm .githooks/pre-commit catches C/C++ formatting issues on staged files.

Test Result

Not run locally; relies on GitHub Actions and Jenkins pipeline execution after submission.

Submission Checklist

@bogdan-petkovic bogdan-petkovic force-pushed the bogdan-petkovic/move-lint-to-gh-actions branch from fcb2e9d to ff2cfcf Compare April 22, 2026 09:46
Comment thread .githooks/pre-push Outdated
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.

I would keep preMergeCheck in the Jenkinsfile for at least 1–2 weeks after merging this PR, to ensure that the new GitHub Action works correctly with incoming PRs. During this testing phase, we would run both Jenkins and GitHub Actions in parallel. I would validate the setup by opening PRs that are expected to fail preMergeCheck and then compare the outputs from Jenkins and GitHub Actions. Once we confirm it works as expected, I would remove it from the Jenkinsfile in a follow-up PR. We don’t want checks that don’t fully enforce formatting (e.g., clang-format) on the develop branch

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.

But check for this with @umangyadav @mirza-halilcevic @dhernandez0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, running them in parallel for a week or two is a good call

@bogdan-petkovic bogdan-petkovic force-pushed the bogdan-petkovic/move-lint-to-gh-actions branch from 73aed8e to cb4015c Compare April 23, 2026 14:06
@bogdan-petkovic bogdan-petkovic marked this pull request as ready for review April 24, 2026 11:59
…ng on formatting

Signed-off-by: bogdan-petkovic <bogdan.petkovic@htecgroup.com>
Signed-off-by: bogdan-petkovic <bogdan.petkovic@htecgroup.com>
Signed-off-by: bogdan-petkovic <bogdan.petkovic@htecgroup.com>
Signed-off-by: bogdan-petkovic <bogdan.petkovic@htecgroup.com>
Signed-off-by: bogdan-petkovic <bogdan.petkovic@htecgroup.com>
@bogdan-petkovic bogdan-petkovic force-pushed the bogdan-petkovic/move-lint-to-gh-actions branch from cb4015c to d953156 Compare April 24, 2026 13:30
@umangyadav
Copy link
Copy Markdown
Member

@bogdan-petkovic can you review using AI agents first ? it is reporting some major problems

@dorde-antic dorde-antic requested a review from Copilot April 24, 2026 13:43
@dorde-antic
Copy link
Copy Markdown
Contributor

@bogdan-petkovic can you review using AI agents first ? it is reporting some major problems

@umangyadav I've assigned copilot for that

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR moves C/C++ diff-based clang-format and clang-tidy checks out of Jenkins and into GitHub Actions by introducing a dedicated cpp-static-checks job that runs the existing mlir/utils/jenkins/static-checks/premerge-checks.py inside the rocm/mlir:rocm7.2-latest container.

Changes:

  • Add a new cpp-static-checks GitHub Actions job to run diff-based clang-format and clang-tidy checks.
  • Generate and expose compile_commands.json via CMake configure (-DCMAKE_EXPORT_COMPILE_COMMANDS=ON) for the tidy-diff step.
  • Add caching for the build/ directory to reduce repeated CMake configure cost.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
bogdan-petkovic and others added 8 commits April 27, 2026 09:37
Signed-off-by: bogdan-petkovic <bogdan.petkovic@htecgroup.com>
Signed-off-by: bogdan-petkovic <bogdan.petkovic@htecgroup.com>
…NER=1

Signed-off-by: bogdan-petkovic <bogdan.petkovic@htecgroup.com>
Signed-off-by: bogdan-petkovic <bogdan.petkovic@htecgroup.com>
…time

Signed-off-by: bogdan-petkovic <bogdan.petkovic@htecgroup.com>
Signed-off-by: bogdan-petkovic <bogdan.petkovic@htecgroup.com>
Comment thread .github/workflows/ci.yml
@@ -11,6 +11,8 @@ on:
- "!external/llvm-project/**"
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.

we excluded external/llvm-project/** for py checks, with current change the exclusion applies to both py ones and cpp-static-checks. Check with @umangyadav / @dhernandez0 whether it should stay as it is

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, and I looked into this and if we want to properly include external LLVM patches in the C++ check we actually have 3 options:

  1. Split into 2 workflow files, a small cpp-static-checks.yml with its own path filter that includes external/llvm-project/**, and keep ci.yml for Python.
  2. Remove the !external/llvm-project/** exclusion. C++ and Python jobs both trigger on LLVM-only PRs. Python only checks files under mlir/ so it won't touch LLVM Python files, but it's some overhead per run for nothing.
  3. Leave as it is. C++ checks will catch LLVM patches when a PR has both rocMLIR and LLVM changes, but LLVM-only PRs won't trigger the job at all.

My vote is option 1, a separate cpp-static-checks.yml would be just a small file with its own path filter. What do you guys think? @umangyadav @dhernandez0

Comment thread .github/workflows/ci.yml Outdated
- ".clang-tidy"
jobs:
cpp-static-checks:
name: C/C++ clang-format + clang-tidy (diff-based)
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.

Image

can we change C/C++ clang-format + clang-tidy (diff-based) to C/C++ premerge checks so that it's similar with the current stage in Jenkins ? Now it looks somehow messy

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.

and do we run diff-based one as well on Jenkins? or we do for everything?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, same on Jenkins. It calls the exact same premerge-checks.py script with --base-commit=origin/${targetBranch}, so it's always diff-based. The script only checks changes relative to the base branch, never the full codebase

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.

5 participants