Skip to content

https://github.com/intel/llvm/issues/6651: Fix lint #6652

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
merged 1 commit into from
Aug 29, 2022
Merged

https://github.com/intel/llvm/issues/6651: Fix lint #6652

Show file tree
Hide file tree
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
4 changes: 3 additions & 1 deletion .github/workflows/sycl_precommit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ jobs:
container:
image: ghcr.io/intel/llvm/sycl_ubuntu2004_nightly:no-drivers
steps:
- name: 'PR commits + 1'
run: echo "PR_FETCH_DEPTH=$(( ${{ github.event.pull_request.commits }} + 1 ))" >> "${GITHUB_ENV}"
- uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
persist-credentials: false
fetch-depth: 2
fetch-depth: ${{ env.PR_FETCH_DEPTH }}
Comment on lines 29 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach pulls a lot of unnecessary commits. We need to pull just 2 commits: head of tested branch and merge-base with sycl branch. Before your changes we used pull request merge commit, so using fetch-depth: 2 was reasonable way to pull these two required commits.
Now as we don't use merge commit, we should probably use fetch-depth: 1, but fetch only required refs.

We have pulldowns from the community, which add a lot of commits to the sycl branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tagging @apstasen

Copy link
Contributor Author

@apstasen apstasen Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We only fetch commits that belong PR branch plus one parent commit from sycl branch (from the point when it was branched from sycl branch). If we use fetch depth 1, then git tree used in git clang format will not work properly since sycl HEAD and PR HEAD will not have common history. PR_FETCH_DEPTH is minimum possible number of commits we need.

- name: Run clang-format
uses: ./devops/actions/clang-format

Expand Down
3 changes: 2 additions & 1 deletion devops/actions/clang-format/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ runs:
shell: bash {0}
run: |
git config --global --add safe.directory /__w/llvm/llvm
git clang-format ${GITHUB_SHA}^1
git fetch origin sycl
git clang-format ${GITHUB_SHA}
git diff > ./clang-format.patch
# Add patch with formatting fixes to CI job artifacts
- uses: actions/upload-artifact@v1
Expand Down