Karthik Nayak <karthik.188@xxxxxxxxx> writes: > We don't run style checks on our CI, even though we have a > '.clang-format' setup in the repository. Let's add one, the job will > validate only against the new commits added and will only run on merge > requests. I hope "against new commits" means what I think it does ;-) I am worried about the case where a new commit touches a file that has existing style violations but the commit does not make the situation any worse at all. OK, "git clang-format" is used to for this thing and it is designed to address only lines that differ, so hopefully that would be good. > Since we're introducing it for the first time, let's allow > this job to fail, so we can validate if this is useful and eventually > enforce it. Very good idea. > Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx> > --- > .github/workflows/check-style.yml | 29 +++++++++++++++++++++++++++++ > .gitlab-ci.yml | 12 ++++++++++++ > ci/install-dependencies.sh | 2 +- > ci/run-style-check.sh | 8 ++++++++ > 4 files changed, 50 insertions(+), 1 deletion(-) > create mode 100644 .github/workflows/check-style.yml > create mode 100755 ci/run-style-check.sh > > diff --git a/.github/workflows/check-style.yml b/.github/workflows/check-style.yml > new file mode 100644 > index 0000000000..27276dfe5e > --- /dev/null > +++ b/.github/workflows/check-style.yml > @@ -0,0 +1,29 @@ > +name: check-style > + > +# Get the repository with all commits to ensure that we can analyze > +# all of the commits contributed via the Pull Request. > + > +on: > + pull_request: > + types: [opened, synchronize] > + > +# Avoid unnecessary builds. Unlike the main CI jobs, these are not > +# ci-configurable (but could be). > +concurrency: > + group: ${{ github.workflow }}-${{ github.ref }} > + cancel-in-progress: true > + > +jobs: > + check-style: > + runs-on: ubuntu-latest > + steps: > + - uses: actions/checkout@v4 > + with: > + fetch-depth: 0 > + > + - name: git clang-format > + continue-on-error: true > + id: check_out > + run: | > + ./ci/run-style-check.sh \ > + "${{github.event.pull_request.base.sha}}" > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index 37b991e080..65fd261e5e 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -123,6 +123,18 @@ check-whitespace: > rules: > - if: $CI_PIPELINE_SOURCE == 'merge_request_event' > > +check-style: > + image: ubuntu:latest > + allow_failure: true > + variables: > + CC: clang > + before_script: > + - ./ci/install-dependencies.sh > + script: > + - ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA" > + rules: > + - if: $CI_PIPELINE_SOURCE == 'merge_request_event' > + > documentation: > image: ubuntu:latest > variables: > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh > index 6ec0f85972..46fe12a690 100755 > --- a/ci/install-dependencies.sh > +++ b/ci/install-dependencies.sh > @@ -43,7 +43,7 @@ ubuntu-*) > make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \ > tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \ > libemail-valid-perl libio-pty-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \ > - ${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE > + ${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE clang-format > > mkdir --parents "$CUSTOM_PATH" > wget --quiet --directory-prefix="$CUSTOM_PATH" \ > diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh > new file mode 100755 > index 0000000000..9d4c4089c1 > --- /dev/null > +++ b/ci/run-style-check.sh > @@ -0,0 +1,8 @@ > +#!/usr/bin/env sh Under ci/ hierarchy we are very inconsistent. Most use the bog standard "#!/bin/sh" (which is my preference by the way), but see what we have here right now: $ git grep -e '^#![a-z/]*/bin/[a-z]*sh' -e '^#![a-z/]*bin/env ' ci | sort -t: -k2 ci/check-directional-formatting.bash:#!/bin/bash ci/install-dependencies.sh:#!/bin/sh ci/make-test-artifacts.sh:#!/bin/sh ci/mount-fileshare.sh:#!/bin/sh ci/print-test-failures.sh:#!/bin/sh ci/run-build-and-minimal-fuzzers.sh:#!/bin/sh ci/run-build-and-tests.sh:#!/bin/sh ci/run-docker-build.sh:#!/bin/sh ci/run-docker.sh:#!/bin/sh ci/run-static-analysis.sh:#!/bin/sh ci/run-test-slice.sh:#!/bin/sh ci/util/extract-trash-dirs.sh:#!/bin/sh ci/check-whitespace.sh:#!/usr/bin/env bash ci/test-documentation.sh:#!/usr/bin/env bash Unless you have a strong reason to suspect that in your CI environment /bin/sh is an unusuably broken shell, please do not spread the inconsistencies. I think the consensus from the last discussion we had was to allow scripts that rely on bash-isms to say "#!/usr/bin/env bash" because we know /bin/sh can legitimately be not bash and we assume bash may not be installed as /bin/bash. As all of them would run in the CI environment that we have some control over what required packages are installed at what path, it is OK to assume that "bash" would be found on the $PATH by using /usr/bin/env (but it does assume everybody installs "env" there, not /bin/env or /usr/local/bin/env, with a bit of chicken-and-egg issue). > +# > +# Perform style check > +# > + > +baseCommit=$1 > + > +git clang-format --style file --diff --extensions c,h "$baseCommit" OK, "git clang-format" compares the working tree with the named commit, so if we have the tip of the topic branch proposed to be merged checked out and compare it with the base commit of the topic, that would give us exactly what we want. Nice. Thanks.