Skip to content

[Github] Simplify Getting Changed Files in Code Formatting Workflow #133023

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.

Sign up for GitHub

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
boomanaiden154 merged 1 commit into from
Mar 29, 2025
Merged

[Github] Simplify Getting Changed Files in Code Formatting Workflow #133023

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions .github/workflows/pr-code-format.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,16 @@ jobs:
- name: Fetch LLVM sources
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
ref: ${{ github.event.pull_request.head.sha }}

- name: Checkout through merge base
uses: rmacklin/fetch-through-merge-base@bfe4d03a86f9afa52bc1a70e9814fc92a07f7b75 # v0.3.0
with:
base_ref: ${{ github.event.pull_request.base.ref }}
head_ref: ${{ github.event.pull_request.head.sha }}
deepen_length: 500
fetch-depth: 2

- name: Get changed files
id: changed-files
uses: step-security/changed-files@3dbe17c78367e7d60f00d78ae6781a35be47b4a1 # v45.0.1
with:
separator: ","
skip_initial_fetch: true
base_sha: 'HEAD~1'
sha: 'HEAD'

# We need to pull the script from the main branch, so that we ensure
# we get the latest version of this script.
Expand Down Expand Up @@ -89,8 +84,8 @@ jobs:
--write-comment-to-file \
--token ${{ secrets.GITHUB_TOKEN }} \
--issue-number $GITHUB_PR_NUMBER \
--start-rev $(git merge-base $START_REV $END_REV) \
--end-rev $END_REV \
--start-rev HEAD~1 \
--end-rev HEAD \
Comment on lines +87 to +88
Copy link
Contributor

@vbvictor vbvictor Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, could you please tell why the script works if a person creates a PR with more than 1 commit, e.g:

  1. Person creates PR with 3 commits:
    main -> A -> B -> C

  2. CI premerge check starts and makes git diff C B and only check formatting between those 2 commits. But what we want is git diff C main to properly check formatting.

I'm trying to make a clang-tidy CI job similar to this one and this part doesn't work for me: I only get diff C B which produces not proper diff to analyze.

Copy link
Contributor Author

@boomanaiden154 boomanaiden154 Aug 18, 2025

Choose a reason for hiding this comment

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

Github tests PRs as if they were merged into main. So you end up with main and then a merge commit at HEAD containing all of the changes in the PR. So you only need to diff the merge commit with its base.

Anything clang-tidy should also probably be integrated into the code format job. Using a separate job means we need extra things like an additional clone of LLVM every time.

Copy link
Contributor

@vbvictor vbvictor Aug 18, 2025

Choose a reason for hiding this comment

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

Thank you! I guess in my local runs I have slightly different setup.

Anything clang-tidy should also probably be integrated into the code format job. Using a separate job means we need extra things like an additional clone of LLVM every time.

I'm afraid that code formatting job will become too bloated with all these changes.
For clang-tidy CI, we need to additionally cmake configure, ninja build tablegen and run clang-tidy. I'll look into integration with formatting job.

Copy link
Contributor Author

@boomanaiden154 boomanaiden154 Aug 18, 2025

Choose a reason for hiding this comment

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

Ah, forgot that clang-tidy needs to see a fully preprocessed file. Keeping it separate might be better then.

--changed-files "$CHANGED_FILES"

- uses: actions/upload-artifact@26f96dfa697d77e81fd5907df203aa23a56210a8 #v4.3.0
Expand Down