Skip to content

[WIP] CI checks improvement: add clang-format check #7

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

Closed
neatudarius wants to merge 1 commit into from
Closed

[WIP] CI checks improvement: add clang-format check #7

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
58 changes: 38 additions & 20 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,26 @@ on:
branches: [ main ]

jobs:
lint:
Copy link
Member

@steve-downey steve-downey Jun 12, 2024

Choose a reason for hiding this comment

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

This https://cpp-linter.github.io/cpp-linter-action/ might be a friendlier linter interface?

Copy link
Member Author

@neatudarius neatudarius Jun 13, 2024

Choose a reason for hiding this comment

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

I tried your suggestion. I couldn't make it work and also it uses clang-12 https://github.com/beman-project/Optional26/actions/runs/9494415629/job/26164716972 :( .

I was also thinking about an market Git action, but then I realized this doesn't help me to apply changes locally. Do you think it's better to have a script (e.g., lint-all.sh - cpp, sh, md, yaml etc) and be able to run locally and from github workflow?

Copy link
Member

@steve-downey steve-downey Jun 13, 2024

Choose a reason for hiding this comment

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

I'm talking with some other developers locally about how they handle this for public github projects. We have a much nicer internal workflow tool, of course.
The weak consensus seems to be that for clang-format it's hard to justify a separate action that needs to build a docker image, and it's better to get the clang-format that you want, and then apply something like https://github.com/bloomberg/blazingmq/blob/d3bf9637db34a91455c9d83354c4844c178c3f1e/.github/workflows/formatting-check.yaml#L22

        run: |
          git clang-format-14 --diff -q origin/main | tee format_diff.txt
          if [ -s format_diff.txt ]; then exit 1; fi

Which is hard coded to clang-format-14 which is so ancient it may not understand C++ anymore. But we've already got some infrastructure for getting LLVM current added.

clang-tidy may be more complicated, since it often needs review to confirm applicability? One suggestion has been
https://github.com/marketplace/actions/clang-tidy-review

Which does allow current clang-tidy to be used.

Making anything we do in automated checks something that can be straightforwardly done locally is important to me, as well. There's little as frustrating as finding out 30 minutes later that you've done something that would have been trivial to fix as you were coding it. Shifting left in the business process vernacular.

name: Lint C++
runs-on: ubuntu-24.04
steps:
- name: Checkout
uses: actions/checkout@v3

- name: Install Clang Format
run: |
sudo apt-get update
sudo apt-get install clang-format
Copy link
Member

@steve-downey steve-downey Jun 12, 2024

Choose a reason for hiding this comment

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

clang-format gets stale fast for contemporary C++, we should make sure we're getting a current one.


- name: Run Clang Format
run: |
git diff --name-only main | grep -E '\.(h|hpp|hxx|c|cpp|cxx)$' | xargs clang-format -i

- name: Check Clang Format
run: |
git diff --name-only main | grep -E '\.(h|hpp|hxx|c|cpp|cxx)$' | xargs clang-format -output-replacements-xml | grep "<replacement " && exit 1 || exit 0

build:
name: ${{ matrix.config.name }}
runs-on: ${{ matrix.config.os }}
Expand All @@ -17,30 +37,30 @@ jobs:
matrix:
config:
- {
name: "Ubuntu Clang 17",
Copy link
Member Author

@neatudarius neatudarius Jun 12, 2024

Choose a reason for hiding this comment

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

For consistency with other blocks in the file.

os: ubuntu-24.04,
toolchain: "clang-17-toolchain.cmake",
clang_version: 17,
installed_clang_version: 14,
cmake_args: "-G \"Ninja Multi-Config\" -DCMAKE_CONFIGURATION_TYPES=\"RelWithDebInfo;Asan\" "
name: "Ubuntu Clang 17",
os: ubuntu-24.04,
toolchain: "clang-17-toolchain.cmake",
clang_version: 17,
installed_clang_version: 14,
cmake_args: "-G \"Ninja Multi-Config\" -DCMAKE_CONFIGURATION_TYPES=\"RelWithDebInfo;Asan\" "
}

- {
name: "Ubuntu Clang 18",
os: ubuntu-24.04,
toolchain: "clang-18-toolchain.cmake",
clang_version: 18,
installed_clang_version: 14,
cmake_args: "-G \"Ninja Multi-Config\" -DCMAKE_CONFIGURATION_TYPES=\"RelWithDebInfo;Asan\" "
name: "Ubuntu Clang 18",
os: ubuntu-24.04,
toolchain: "clang-18-toolchain.cmake",
clang_version: 18,
installed_clang_version: 14,
cmake_args: "-G \"Ninja Multi-Config\" -DCMAKE_CONFIGURATION_TYPES=\"RelWithDebInfo;Asan\" "
}

- {
name: "Ubuntu Clang 19",
os: ubuntu-24.04,
toolchain: "clang-19-toolchain.cmake",
clang_version: 19,
installed_clang_version: 14,
cmake_args: "-G \"Ninja Multi-Config\" -DCMAKE_CONFIGURATION_TYPES=\"RelWithDebInfo;Asan\" "
name: "Ubuntu Clang 19",
os: ubuntu-24.04,
toolchain: "clang-19-toolchain.cmake",
clang_version: 19,
installed_clang_version: 14,
cmake_args: "-G \"Ninja Multi-Config\" -DCMAKE_CONFIGURATION_TYPES=\"RelWithDebInfo;Asan\" "
}

- {
Expand Down Expand Up @@ -95,14 +115,12 @@ jobs:
sudo ./llvm.sh ${{matrix.config.clang_version}} all
sudo apt-get install libc++-dev libc++1 libc++abi-dev libc++abi1


- name: Install GCC 14
if: matrix.config.name == 'Ubuntu GCC 14'
run: |
sudo apt update
sudo apt-get install g++-14


- name: Configure
run: |
mkdir -p .build
Expand Down