Add Clang-format checker GitHub Action#46
Add Clang-format checker GitHub Action#46kanglant wants to merge 6 commits intogoogle-ml-infra:mainfrom
Conversation
|
I've tested this change using a simple workflow that uses this composite action: https://github.com/kanglant/experiments-repo/blob/main/.github/workflows/clang_format.yml. Three test scenarios:
|
|
Additionally please add actions tests to this repo to test the functionality, i see you did some already on your own repo to prove it works. For this repo a minimal set just proving it works would be ideal. We can store files somewhere like tests/(this action name)/* off the root of the repo if necessary. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ci_clangformat/action.yaml
Outdated
| - name: "Checking out repository" | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| fetch-depth: 0 # Fetch full history for accurate diffing |
There was a problem hiding this comment.
Do we need all history or would fetch-depth: 2 work here?
There was a problem hiding this comment.
fetch-depth: 2 fetches the current commit (HEAD) and its immediate parent commit (HEAD^). If the PR branch diverged from main more than 1 commit, git diff origin/main HEAD will fail.
There are 3 options:
- Use
fetch-depth: 0. It fetches the entire Git history for all branches and tags. - Use
fetch-depth: 2. Add a step to fetch the${{ github.base_ref }}branch, usuallymain. - Use
fetch-depth: 2andgit diff ... HEAD^ HEAD. Instead of checking all changes in the PR relative to its base branch, we'll be checking only the changes in the last commit of the PR, comparing the current commit (HEAD) against its immediate parent (HEAD^).
There was a problem hiding this comment.
Did you run into this problem? I am mainly pushing more on this so i can update my own understanding of how actions operates here if i am mistaken.
My understanding was in the case of an action running in a PR actions does something somewhat tricky and actually creates a merge commit as the running commit. Which should mean HEAD is the merge commit and its immediate parent is the base branch.
There was a problem hiding this comment.
Ah, I get why fetch-depth: 2 and HEAD~1 works for PRs now (thanks for explaining the merge commit).
However, for push events involving multiple commits, HEAD~1 only represents the second-to-last commit, leaving github.event.before unreachable. I've added a git fetch for push events to ensure the full diff range is available. Please review the latest PR #138
| if: | | ||
| github.event.sender.type == 'User' || | ||
| contains(github.event.pull_request.body, 'RUN_CLANG_FORMAT') |
There was a problem hiding this comment.
Why have this if check? Shouldn't we want this to always run?
There was a problem hiding this comment.
This is a just test workflow. I referred xla's workflow and think this is a good practice as it ensures the workflow runs for user-initiated (not bot-created) PRs and can be explicitly triggered by including RUN_CLANG_FORMAT in the PR description.
There was a problem hiding this comment.
In this case we would still want bot created PRs to invoke (dependabot is our only bot at the moment)
There was a problem hiding this comment.
I am wondering if XLA added that case specifically because of copybara being a bot. That would make a big difference in number of executions
There was a problem hiding this comment.
I am wondering if XLA added that case specifically because of copybara being a bot. That would make a big difference in number of executions
That's what I thought. TF/XLA have an internal clang-format check, so there's no need to run it on copybara-created PRs.
In this case we would still want bot created PRs to invoke (dependabot is our only bot at the moment)
Okay, let me remove the constraint.
| clang_format_version: | ||
| description: 'The clang-format version to use.' | ||
| required: true | ||
| default: "20.1.5" |
There was a problem hiding this comment.
Are we able to sha lock this?
There was a problem hiding this comment.
I think we can keep using the version number as uvx attempts to find and download clang-format from PyPI.
uvx clang-format@<version> installs clang-format at a specific version into a temporary isolated environment and invokes it. This is fine as we just need an one-off run of clang-format. Here is the uvx doc. uvx doesn't directly support a --hash flag. If hash-based verification is critical, we'd need to consider a pip compile and installation approach.
Or we can use uvx --from git+https://github.com/ssciwr/clang-format-wheel@<commit sha> clang-format ... to install it from the git repo at a commit, but this would require deps to build the tool.
There was a problem hiding this comment.
Is this the minimal set of files we can use for this purpose?
If so we should document where it comes from and how to update it to the live version if necessary
There was a problem hiding this comment.
Cleaned up the test environment; only 4 files remain for testing purposes.
| ACTION_DEFAULT_CLANG_FORMAT: ${{ github.action_path }}.clang-format.default | ||
| CLANG_FORMAT_VERSION: ${{ inputs.clang_format_version }} | ||
| TARGET_BRANCH: ${{ github.base_ref }} | ||
| run: | |
There was a problem hiding this comment.
I just realized but this execution seems only good for PRs.
We should decide what needs done if we run on a push event
There was a problem hiding this comment.
I've added logic for push events in PR #138 . Ready for further discussion over there. I also included the 'on-error' test case we discussed.
2 participants
Add a new GitHub Composite Action to automatically enforce C/C++ code formatting using
clang-formatin pull requests.It provides a reusable and standardized action for CI/CD, eliminating the need for individual teams or users to duplicate clang-format setup in their workflows.