| Message ID | 20240718081605.452366-5-karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | : add more rules and enable on CI | expand |
Karthik Nayak <karthik.188@gmail.com> writes: > + # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged > + # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should > + # be defined in all pipelines. > + script: > + - | > + R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}} > + > + if test -z "$R" > + then > + echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!" > + exit 1 > + fi If you update the assignment to R to R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} an unset CI_MERGE_REQUEST_DIFF_BASE_SHA1 or an empty value in it will be rejected there. Then you can lose the if/then/fi here. > + ./ci/run-style-check.sh "$R" > + 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..fb34ced8f0 100755 > --- a/ci/install-dependencies.sh > +++ b/ci/install-dependencies.sh > @@ -87,6 +87,10 @@ macos-*) > esac > > case "$jobname" in > +ClangFormat) > + sudo apt-get -q update > + sudo apt-get -q -y install clang-format > + ;; > StaticAnalysis) > sudo apt-get -q update > sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \ > diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh > new file mode 100755 > index 0000000000..76dd37d22b > --- /dev/null > +++ b/ci/run-style-check.sh > @@ -0,0 +1,8 @@ > +#!/bin/sh > +# > +# Perform style check > +# > + > +baseCommit=$1 > + > +git clang-format --style file --diff --extensions c,h "$baseCommit"
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > ... >> + R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}} >> + >> + if test -z "$R" >> + then >> + echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!" >> + exit 1 >> + fi > > If you update the assignment to R to > > R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} > > an unset CI_MERGE_REQUEST_DIFF_BASE_SHA1 or an empty value in it > will be rejected there. Then you can lose the if/then/fi here. Actually, you also need to pay attention to the exit status of the assignment, i.e. R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA:?}} || exit
diff --git a/.github/workflows/check-style.yml b/.github/workflows/check-style.yml new file mode 100644 index 0000000000..c052a5df23 --- /dev/null +++ b/.github/workflows/check-style.yml @@ -0,0 +1,34 @@ +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: + env: + CC: clang + jobname: ClangFormat + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - run: ci/install-dependencies.sh + + - 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..817266226e 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -123,6 +123,30 @@ check-whitespace: rules: - if: $CI_PIPELINE_SOURCE == 'merge_request_event' +check-style: + image: ubuntu:latest + allow_failure: true + variables: + CC: clang + jobname: ClangFormat + before_script: + - ./ci/install-dependencies.sh + # Since $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is only defined for merged + # pipelines, we fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA, which should + # be defined in all pipelines. + script: + - | + R=${CI_MERGE_REQUEST_TARGET_BRANCH_SHA-${CI_MERGE_REQUEST_DIFF_BASE_SHA?}} + + if test -z "$R" + then + echo "CI_MERGE_REQUEST_DIFF_BASE_SHA should always exist!" + exit 1 + fi + ./ci/run-style-check.sh "$R" + 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..fb34ced8f0 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -87,6 +87,10 @@ macos-*) esac case "$jobname" in +ClangFormat) + sudo apt-get -q update + sudo apt-get -q -y install clang-format + ;; StaticAnalysis) sudo apt-get -q update sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \ diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh new file mode 100755 index 0000000000..76dd37d22b --- /dev/null +++ b/ci/run-style-check.sh @@ -0,0 +1,8 @@ +#!/bin/sh +# +# Perform style check +# + +baseCommit=$1 + +git clang-format --style file --diff --extensions c,h "$baseCommit"
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. 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. For GitHub, we allow the job to pass by adding 'continue-on-error: true' to the workflow. This means the job would show as passed, even if the style check failed. To know the status of the job, users have to manually check the logs. For GitLab, we allow the job to pass by adding 'allow_failure: true', to the job. Unlike GitHub, here the job will show as failed with a yellow warning symbol, but the pipeline would still show as passed. Also for GitLab, we use the 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA' variable by default to obtain the base SHA of the merged pipeline (which is only available for merged pipelines [1]). Otherwise we use the 'CI_MERGE_REQUEST_DIFF_BASE_SHA' variable. [1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- .github/workflows/check-style.yml | 34 +++++++++++++++++++++++++++++++ .gitlab-ci.yml | 24 ++++++++++++++++++++++ ci/install-dependencies.sh | 4 ++++ ci/run-style-check.sh | 8 ++++++++ 4 files changed, 70 insertions(+) create mode 100644 .github/workflows/check-style.yml create mode 100755 ci/run-style-check.sh