3rdparty/dev-tools

Standardize on `clang-format` version

Closed this issue · 4 comments

Our dev-tools code style check currently uses either clang-format-12 or clang-format-13, depending on the platform you run on:

# If you check the link above you will see that on Ubuntu 20.04
# we have preinstalled clang-format-10/11/12. By default the symlink
# to /usr/bin/clang-format-11 is set. That's why we will not be able
# to run successfully the checks for code style cause the minimum
# version required is `12` (see `dev-tools/check-code-style/check_style.sh`
# especially check_clang_format() function).
# That's why the main idea of this step is just setting the symlink to
# /usr/bin/clang-format-12, nothing more. Thus we will be able to run all
# checks with correct version of clang-format using GitHub Actions.
run: |
# Before we recreate the symlink we should delete
# the existing one.
sudo rm /etc/alternatives/clang-format
# Recreate symlink 'clang-format'
sudo ln -s /usr/bin/clang-format-12 /etc/alternatives/clang-format
shell: bash
# On macOS we should install clang-format.
- name: Install clang-format-13 if macOS
if: inputs.os == 'macos-latest'
run: |
brew install clang-format@13
shell: bash

Additionally, our internal codespaces currently have clang-format-13 installed.

Let's make this less confusing by standardizing on one version everywhere (maybe `clang-format-13)

(@while-false was confusing formatting errors that were only happening on the GitHub Actions Runner but weren't reproducing on his codespace)

@stephaniepalocz FYI

Here is how llvm thinks we should install it (which the Dockerfile was inspired by): https://apt.llvm.org/ (see the section Install (stable branch))

It turns out this clang-format version mismatch was exactly what was causing the errors that @while-false was seeing: locally running clang-format-13 produced no errors, but running clang-format-12 on the same code (which is what the GitHub Actions Runners were running) produced errors.

So yes, let's standardize on one version :)

@while-false I spent a bit of time on this and threw up #53, but was having some trouble getting it tested in context via https://github.com/reboot-dev/respect/pull/548 (something something submodule sync gone wrong). Please feel free to adopt these PRs as you find it helpful, or just make your own, or leave it for me to finish up in the morning.