-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[GitHub] Bump clang-tidy version to 21th in CI #160551
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.
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
Conversation
@llvm/pr-subscribers-github-workflow Author: Baranov Victor (vbvictor) ChangesFull diff: https://github.com/llvm/llvm-project/pull/160551.diff 1 Files Affected:
diff --git a/.github/workflows/pr-code-lint.yml b/.github/workflows/pr-code-lint.yml
index bc70933147bd2..caa2a134c521d 100644
--- a/.github/workflows/pr-code-lint.yml
+++ b/.github/workflows/pr-code-lint.yml
@@ -19,8 +19,6 @@ jobs:
defaults:
run:
shell: bash
- container:
- image: 'ghcr.io/llvm/ci-ubuntu-24.04:latest'
timeout-minutes: 60
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
@@ -47,10 +45,13 @@ jobs:
echo "Changed files:"
echo "$CHANGED_FILES"
+ # The clang tidy version should always be upgraded to the first version
+ # of a release cycle (x.1.0) or the last version of a release cycle, or
+ # if there have been relevant clang-format backports.
- name: Install clang-tidy
uses: aminya/setup-cpp@17c11551771948abc5752bbf3183482567c7caf0 # v1.1.1
with:
- clang-tidy: 20.1.8
+ clang-tidy: 21.1.0
- name: Setup Python env
uses: actions/setup-python@42375524e23c412d93fb67b49958b491fce71c38 # v5.4.0
@@ -59,6 +60,13 @@ jobs:
- name: Install Python dependencies
run: python3 -m pip install -r llvm/utils/git/requirements_linting.txt
+
+ - name: Setup ccache
+ uses: hendrikmuhs/ccache-action@a1209f81afb8c005c13b4296c32e363431bffea5 # v1.2.17
+ with:
+ max-size: 2G
+ key: premerge-clang-tidy
+ variant: sccache
# TODO: create special mapping for 'codegen' targets, for now build predefined set
# TODO: add entrypoint in 'compute_projects.py' that only adds a project and its direct dependencies
|
.github/workflows/pr-code-lint.yml
Outdated
@@ -59,6 +60,13 @@ jobs: | |||
|
|||
- name: Install Python dependencies | |||
run: python3 -m pip install -r llvm/utils/git/requirements_linting.txt | |||
|
|||
- name: Setup ccache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One problem I faced with sccache
is that it doesn't store/restore results when I run multiple the job multiple times here.
The warnings are:
Warning: Cache not found for keys: sccache-premerge-clang-tidy-
Warning: Saving cache failed: Error: Path Validation Error: Path(s) specified in the action for caching do(es) not exist, hence no cache is being saved.
I suspect I need to create this key in somewhere or it will start to work after PR is merged into main?
I can see in other workflows similar setup of sccache
,
llvm-project/.github/workflows/ci-post-commit-analyzer.yml
Lines 46 to 59 in 4d4cb75
- name: Setup ccache | |
uses: hendrikmuhs/ccache-action@a1209f81afb8c005c13b4296c32e363431bffea5 # v1.2.17 | |
with: | |
# A full build of llvm, clang, lld, and lldb takes about 250MB | |
# of ccache space. There's not much reason to have more than this, | |
# because we usually won't need to save cache entries from older | |
# builds. Also, there is an overall 10GB cache limit, and each | |
# run creates a new cache entry so we want to ensure that we have | |
# enough cache space for all the tests to run at once and still | |
# fit under the 10 GB limit. | |
# Default to 2G to workaround: https://github.com/hendrikmuhs/ccache-action/issues/174 | |
max-size: 2G | |
key: post-commit-analyzer | |
variant: sccache |
Apart from this problem, shall I create a dedicated key for "clang-tidy" runs or reuse some existing one? In theory this ccache
should store a little data because all it only compile codegen targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isolating just the version bump into this change would probably be a good idea. That is straight forward and can just land.
Caching + container changes are going to require more discussion.
@@ -19,8 +19,6 @@ jobs: | |||
defaults: | |||
run: | |||
shell: bash | |||
container: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we dropping this? The default compiler in Github Actions is going to be much slower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When setup-cpp action is run, it "fails" with such error:
Installing [email protected] via npm...
Error: Failed to install the [email protected] CLI: Error: Command failed with ENOENT: npm install -g [email protected]
spawn npm ENOENT. Ignoring...
✅ clang-tidy was installed successfully:
"fails" is quoted because it clang-tidy
still is installed in the path. The only drawback is this annotation in UI:
From logs, it seems there are some problems with systemd
during installation:
/usr/lib/tmpfiles.d/systemd-network.conf:10: Failed to resolve user 'systemd-network': No such process
/usr/lib/tmpfiles.d/systemd-network.conf:11: Failed to resolve user 'systemd-network': No such process
/usr/lib/tmpfiles.d/systemd-network.conf:12: Failed to resolve user 'systemd-network': No such process
/usr/lib/tmpfiles.d/systemd-network.conf:13: Failed to resolve user 'systemd-network': No such process
/usr/lib/tmpfiles.d/systemd.conf:22: Failed to resolve group 'systemd-journal': No such process
/usr/lib/tmpfiles.d/systemd.conf:23: Failed to resolve group 'systemd-journal': No such process
/usr/lib/tmpfiles.d/systemd.conf:28: Failed to resolve group 'systemd-journal': No such process
/usr/lib/tmpfiles.d/systemd.conf:29: Failed to resolve group 'systemd-journal': No such process
/usr/lib/tmpfiles.d/systemd.conf:30: Failed to resolve group 'systemd-journal': No such process
This leads to this topic, but I can't understand how given answer correlate to us because it said "The host was Ubuntu 20.04 and the image was 24.04.", but we run ubuntu-24 docker image on ubuntu-24 runner.
I found that if I remove this container (which is not used in clang-format
) everything installs correctly, and I didn't see significant downgrade of compilation speed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird. We should probably be setting up a container with just clang-tidy and just clang-format since the setup-cpp action takes forever. That's a completely separate action item that needs some refactoring to be done first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably be setting up a container with just clang-tidy and just clang-format
Very much in favor of it
.github/workflows/pr-code-lint.yml
Outdated
@@ -59,6 +60,13 @@ jobs: | |||
|
|||
- name: Install Python dependencies | |||
run: python3 -m pip install -r llvm/utils/git/requirements_linting.txt | |||
|
|||
- name: Setup ccache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not going to give you any caching across PRs due to how the caches are namespaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's unfortunate. Is there a way to make it work across PRs? Build time of codegen targets is 4-5mins and it is sometimes similar to the whole CI-Linux run which builds whole clang;clang-tools-extra
and run tests. I suspect CI checks are fast because they run on llvm-premerge-linux-runners
with many cores, but sccache
must do a big job too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to also build against main. Probably not something that we want to do, but we could discuss doing it.
1a060de
to
2f3e0b4
Compare
I removed non-related changes from current PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
No description provided.