Skip to content

ci: use clang-format from apt llvm #1042

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

Merged
merged 1 commit into from
Jul 15, 2025
Merged

Conversation

Bambooin
Copy link
Contributor

Pull request

Issue tracker

Fixes will automatically close the related issue

Most of C++ developers prefer Unix box, so we should use the Unix for style check.

  1. The lint time decrease from 1m 47s to 28s which is an improvement of 73.83%.
  2. Upgrade clang-format is easier with one parameter.
  3. llvm apt support Debian and Ubuntu distribution.
  4. Improve CI waiting time for developer.

Feature

Describe feature of pull request

Unit test

  • Done

Manual test

  • Done

Code Review

  1. Unit and manual test pass
  2. GitHub Action CI pass
  3. At least one contributor reviews and votes
  4. Can be merged clean without conflicts
  5. PR will be merged by rebase upstream base

Additional Info

Most of C++ developers prefer Unix box, so we should use the Unix for style check.
1. The lint time decrease from 1m 47s to 28s which is an improvement of 73.83%.
2. Upgrade clang-format is easier with one parameter.
3. llvm apt support Debian and Ubuntu distribution.
4. Improve CI waiting time for developer.
@fxliang
Copy link
Contributor

fxliang commented Jul 14, 2025

only clang-format-18 is required, it would be faster like bellow(22.04 jammy for example)

          wget https://apt.llvm.org/llvm-snapshot.gpg.key
          sudo gpg --dearmor < llvm-snapshot.gpg.key > /usr/share/keyrings/llvm-archive-keyring.gpg
          echo "deb [signed-by=/usr/share/keyrings/llvm-archive-keyring.gpg] http://apt.llvm.org/jammy/ llvm-toolchain-jammy-18 main" | sudo tee /etc/apt/sources.list.d/llvm-toolchain-jammy-18.list
          sudo apt-get update
          sudo apt-get install -y clang-format-18
          sudo ln -sf /usr/bin/clang-format-18 /usr/local/bin/clang-format
          clang-format --version

@Bambooin
Copy link
Contributor Author

Yeah, in the initial draft I want to install clang-format package too.

The current lint time 27s in one task[1] which is faster enough for us, IMO.

The current script is robust and maitained by LLVM team which support all debian derivation distributions and all clang version.

  1. https://github.com/Bambooin/librime/actions/runs/16234549398/job/45842887260

@lotem
Copy link
Member

lotem commented Jul 14, 2025

安裝步驟也太複雜了。
個人開發者如何能做到與 CI 環境同步呢?

畢竟只是個輔助性的開發工具,我覺得沒必要做複雜的優化。
最好用默認的,大家都普遍裝備的版本。
有沒有一條命令(apt install?),能安裝該軟件的默認版本?

@Bambooin
Copy link
Contributor Author

The current clang-format 18 and 19 on Ubunut 24.04 has issue with GitHub runner image.

The lint failed with weird error on [1] with clang-format-18 and on [2] with clang-format-19.

On my local machine box with openSUSE, both clang-format 18 and 19 are fine.

The clang-format 18 and 19 on both llvm, openSUSE and latest Ubuntu image with Podman are fine.

If we want use apt install clang-format, part of rolling updated distribution will install clang-format-20.

We need to fix one format style with clang-format-20 which will break old distribution with older clang-format.

So I picked the llvm apt at final, fix for the weird clang format error under GitHub runner iamge are welcome.

  1. https://github.com/Bambooin/librime/actions/runs/16232976340/job/45838997180
  2. https://github.com/Bambooin/librime/actions/runs/16233132706/job/45839368372

@Bambooin
Copy link
Contributor Author

Only one style violation on clang-format-20 under openSUSE.

index 85cac5ce..5137d77c 100644
--- a/src/rime/dict/corrector.cc
+++ b/src/rime/dict/corrector.cc
@@ -207,9 +207,10 @@ Distance EditDistanceCorrector::RestrictedDistance(const std::string& s1,
   for (size_t i = 1; i <= len1; ++i) {
     auto min_d = threshold + 1;
     for (size_t j = 1; j <= len2; ++j) {
-      d[index(i, j)] = (std::min)(
-          {d[index(i - 1, j)] + 2, d[index(i, j - 1)] + 2,
-           d[index(i - 1, j - 1)] + SubstCost(s1[i - 1], s2[j - 1])});
+      d[index(i, j)] =
+          (std::min)({d[index(i - 1, j)] + 2, d[index(i, j - 1)] + 2,
+                      d[index(i - 1, j - 1)] +
+                          SubstCost(s1[i - 1], s2[j - 1])});
       if (i > 1 && j > 1 && s1[i - 2] == s2[j - 1] && s1[i - 1] == s2[j - 2]) {
         d[index(i, j)] = (std::min)(d[index(i, j)], d[index(i - 2, j - 2)] + 2);
       }```

Copy link
Member

@lotem lotem left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@lotem lotem merged commit 1a1fbbe into rime:master Jul 15, 2025
10 checks passed
@Bambooin Bambooin deleted the clang-format branch July 16, 2025 11:18
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.

3 participants