diff mbox series

[7/8] ci: run style check on GitHub and GitLab

Message ID 20240708092317.267915-8-karthik.188@gmail.com (mailing list archive)
State Superseded
Headers show
Series clang-format: add more rules and enable on CI | expand

Commit Message

karthik nayak July 8, 2024, 9:23 a.m. UTC
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.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .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

Comments

Junio C Hamano July 8, 2024, 5:15 p.m. UTC | #1
Karthik Nayak <karthik.188@gmail.com> 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@gmail.com>
> ---
>  .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.
Justin Tobler July 8, 2024, 6:10 p.m. UTC | #2
On 24/07/08 11:23AM, Karthik Nayak wrote:
> 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.

[snip]

> 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"

One downside to using $CI_MERGE_REQUEST_DIFF_BASE_SHA is that for GitLab
merge pipeines, commits from the merge that are not part of the MR
changes are also included. This could lead to somewhat confusing
failures.

Example failure occuring on this patch series:
https://gitlab.com/gitlab-org/git/-/jobs/7284442220

It might be best to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA instead.

> +  rules:
> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
> +
karthik nayak July 8, 2024, 9:05 p.m. UTC | #3
Junio C Hamano <gitster@pobox.com> writes:
[snip]
>> 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).
>

I must admit, I didn't put any thought into this while writing. The
usage of '/usr/bin/env' is mostly muscle memory and since I didn't need
any bash-isms, I defaulted to /bin/sh.

I'll change it, thanks!

>> +#
>> +# 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.
karthik nayak July 8, 2024, 9:16 p.m. UTC | #4
Justin Tobler <jltobler@gmail.com> writes:

> On 24/07/08 11:23AM, Karthik Nayak wrote:
>> 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.
>
> [snip]
>
>> 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"
>
> One downside to using $CI_MERGE_REQUEST_DIFF_BASE_SHA is that for GitLab
> merge pipeines, commits from the merge that are not part of the MR
> changes are also included. This could lead to somewhat confusing
> failures.
>

I'm not sure I follow.

> Example failure occuring on this patch series:
> https://gitlab.com/gitlab-org/git/-/jobs/7284442220
>

If you notice this job, it points to the commit: 1c6551488, and the
parent commit of that commit is: 614dff2011.

The parent commit [1] is a test commit I added to check the failures. So
isn't this working as expected?

> It might be best to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA instead.
>

I actually started with $CI_MERGE_REQUEST_TARGET_BRANCH_SHA, it didn't
work, because the value was undefined.

See: https://gitlab.com/gitlab-org/git/-/jobs/7283724903

This is why I also decided to fix and change the whitespace check.

>> +  rules:
>> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>> +

[1]: https://gitlab.com/gitlab-org/git/-/commit/614dff20119aa325661424a9fcef552e242a95d9
Justin Tobler July 9, 2024, 12:22 a.m. UTC | #5
On 24/07/08 02:16PM, Karthik Nayak wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > On 24/07/08 11:23AM, Karthik Nayak wrote:
> >> 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.
> >
> > [snip]
> >
> >> 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"
> >
> > One downside to using $CI_MERGE_REQUEST_DIFF_BASE_SHA is that for GitLab
> > merge pipeines, commits from the merge that are not part of the MR
> > changes are also included. This could lead to somewhat confusing
> > failures.
> >
> 
> I'm not sure I follow.
> 
> > Example failure occuring on this patch series:
> > https://gitlab.com/gitlab-org/git/-/jobs/7284442220
> >
> 
> If you notice this job, it points to the commit: 1c6551488, and the
> parent commit of that commit is: 614dff2011.
> 
> The parent commit [1] is a test commit I added to check the failures. So
> isn't this working as expected?

Ah ok, I misunderstood the setup of that CI job, but the problem is
still present. Here is an example CI job I've run demonstrating it:

CI - https://gitlab.com/gitlab-org/git/-/jobs/7291829941
MR - https://gitlab.com/gitlab-org/git/-/merge_requests/174

For the MR that spawned this CI job, This patch series is the source
branch and the target branch is a version of master one commit ahead
containing a clang format error. Because this is a merge pipeline, using 
$CI_MERGE_REQUEST_DIFF_BASE_SHA will include changes from either side of
the base commit. This means it would be possible for the CI job to fail
due to commits ahead in the target branch, but not in the source branch.
For the check-whitespace CI job, I specifically chose 
$CI_MERGE_REQUEST_TARGET_BRANCH_SHA for this reason.j

> 
> > It might be best to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA instead.
> >
> 
> I actually started with $CI_MERGE_REQUEST_TARGET_BRANCH_SHA, it didn't
> work, because the value was undefined.
> 
> See: https://gitlab.com/gitlab-org/git/-/jobs/7283724903
> 
> This is why I also decided to fix and change the whitespace check.

I'm not seeing $CI_MERGE_REQUEST_TARGET_BRANCH_SHA as undefined in the
job. Here is a modified version on the check-style CI job printing the
environment variables:

https://gitlab.com/gitlab-org/git/-/jobs/7291792329#L2470

Do you have an example of the check-whitespace job failing in GitLab CI?
Maybe I'm missing something, but I don't see a problem.

-Justin
karthik nayak July 9, 2024, 8:44 a.m. UTC | #6
Justin Tobler <jltobler@gmail.com> writes:

> On 24/07/08 02:16PM, Karthik Nayak wrote:
>> Justin Tobler <jltobler@gmail.com> writes:
>>
>> > On 24/07/08 11:23AM, Karthik Nayak wrote:
>> >> 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.
>> >
>> > [snip]
>> >
>> >> 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"
>> >
>> > One downside to using $CI_MERGE_REQUEST_DIFF_BASE_SHA is that for GitLab
>> > merge pipeines, commits from the merge that are not part of the MR
>> > changes are also included. This could lead to somewhat confusing
>> > failures.
>> >
>>
>> I'm not sure I follow.
>>
>> > Example failure occuring on this patch series:
>> > https://gitlab.com/gitlab-org/git/-/jobs/7284442220
>> >
>>
>> If you notice this job, it points to the commit: 1c6551488, and the
>> parent commit of that commit is: 614dff2011.
>>
>> The parent commit [1] is a test commit I added to check the failures. So
>> isn't this working as expected?
>
> Ah ok, I misunderstood the setup of that CI job, but the problem is
> still present. Here is an example CI job I've run demonstrating it:
>
> CI - https://gitlab.com/gitlab-org/git/-/jobs/7291829941
> MR - https://gitlab.com/gitlab-org/git/-/merge_requests/174
>
> For the MR that spawned this CI job, This patch series is the source
> branch and the target branch is a version of master one commit ahead
> containing a clang format error. Because this is a merge pipeline, using
> $CI_MERGE_REQUEST_DIFF_BASE_SHA will include changes from either side of
> the base commit. This means it would be possible for the CI job to fail
> due to commits ahead in the target branch, but not in the source branch.
> For the check-whitespace CI job, I specifically chose
> $CI_MERGE_REQUEST_TARGET_BRANCH_SHA for this reason.j
>

You're right indeed. I did some more reading about this and I think the
solution lies somewhere in between..

>>
>> > It might be best to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA instead.
>> >
>>
>> I actually started with $CI_MERGE_REQUEST_TARGET_BRANCH_SHA, it didn't
>> work, because the value was undefined.
>>
>> See: https://gitlab.com/gitlab-org/git/-/jobs/7283724903
>>
>> This is why I also decided to fix and change the whitespace check.
>
> I'm not seeing $CI_MERGE_REQUEST_TARGET_BRANCH_SHA as undefined in the
> job. Here is a modified version on the check-style CI job printing the
> environment variables:
>

You can see the output

    $ ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
    fatal: ambiguous argument '': unknown revision or path not in the
working tree.
    Use '--' to separate paths from revisions, like this:
    'git <command> [<revision>...] -- [<file>...]'

This only happens if "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is empty.

> https://gitlab.com/gitlab-org/git/-/jobs/7291792329#L2470
>
> Do you have an example of the check-whitespace job failing in GitLab CI?
> Maybe I'm missing something, but I don't see a problem.
>
> -Justin

So I think I get the issue, GitLab has two kinds of pipelines it runs:
1. merge pipeline: Here the pipeline runs on the source branch (the
feature branch which has to be merged).
2. merged pipeline: Here the pipeline creates a merge commit using the
source and target branch and then runs the pipeline on the merged
commit.

And "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is only defined in the 'merged
pipeline'. If you see the pipelines for my branch on GitLab [1]. You'll
see only one of them is marked as 'merge results' and the others being
marked as 'merged results'. The former includes the job I mentioned
above, where "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is not defined.

I'm still not sure why it marked only one of the pipelines as such, but
this means there is chance that it could happen.

So I guess the best outcome is to use
"$CI_MERGE_REQUEST_TARGET_BRANCH_SHA", but fallback to
"$CI_MERGE_REQUEST_DIFF_BASE_SHA", if the former is not defined.

[1]: https://gitlab.com/gitlab-org/git/-/merge_requests/172/pipelines
Justin Tobler July 9, 2024, 2:44 p.m. UTC | #7
On 24/07/09 01:44AM, Karthik Nayak wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> 
> > On 24/07/08 02:16PM, Karthik Nayak wrote:
> >> Justin Tobler <jltobler@gmail.com> writes:
> >>
> >> > On 24/07/08 11:23AM, Karthik Nayak wrote:
> >> >> 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.
> >> >
> >> > [snip]
> >> >
> >> >> 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"
> >> >
> >> > One downside to using $CI_MERGE_REQUEST_DIFF_BASE_SHA is that for GitLab
> >> > merge pipeines, commits from the merge that are not part of the MR
> >> > changes are also included. This could lead to somewhat confusing
> >> > failures.
> >> >
> >>
> >> I'm not sure I follow.
> >>
> >> > Example failure occuring on this patch series:
> >> > https://gitlab.com/gitlab-org/git/-/jobs/7284442220
> >> >
> >>
> >> If you notice this job, it points to the commit: 1c6551488, and the
> >> parent commit of that commit is: 614dff2011.
> >>
> >> The parent commit [1] is a test commit I added to check the failures. So
> >> isn't this working as expected?
> >
> > Ah ok, I misunderstood the setup of that CI job, but the problem is
> > still present. Here is an example CI job I've run demonstrating it:
> >
> > CI - https://gitlab.com/gitlab-org/git/-/jobs/7291829941
> > MR - https://gitlab.com/gitlab-org/git/-/merge_requests/174
> >
> > For the MR that spawned this CI job, This patch series is the source
> > branch and the target branch is a version of master one commit ahead
> > containing a clang format error. Because this is a merge pipeline, using
> > $CI_MERGE_REQUEST_DIFF_BASE_SHA will include changes from either side of
> > the base commit. This means it would be possible for the CI job to fail
> > due to commits ahead in the target branch, but not in the source branch.
> > For the check-whitespace CI job, I specifically chose
> > $CI_MERGE_REQUEST_TARGET_BRANCH_SHA for this reason.j
> >
> 
> You're right indeed. I did some more reading about this and I think the
> solution lies somewhere in between..
> 
> >>
> >> > It might be best to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA instead.
> >> >
> >>
> >> I actually started with $CI_MERGE_REQUEST_TARGET_BRANCH_SHA, it didn't
> >> work, because the value was undefined.
> >>
> >> See: https://gitlab.com/gitlab-org/git/-/jobs/7283724903
> >>
> >> This is why I also decided to fix and change the whitespace check.
> >
> > I'm not seeing $CI_MERGE_REQUEST_TARGET_BRANCH_SHA as undefined in the
> > job. Here is a modified version on the check-style CI job printing the
> > environment variables:
> >
> 
> You can see the output
> 
>     $ ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
>     fatal: ambiguous argument '': unknown revision or path not in the
> working tree.
>     Use '--' to separate paths from revisions, like this:
>     'git <command> [<revision>...] -- [<file>...]'
> 
> This only happens if "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is empty.

Ya I noticed this failure, but was wondering if it was maybe due to
something else. I have been unable to reproduce it and in all the jobs I
was running resulted in a merge pipeline with the variable defined. But 
maybe sometimes a regular pipeline gets run for some reason and 
consequently $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is not defined? Was the
pipeline triggered directly from the source branch?

> 
> > https://gitlab.com/gitlab-org/git/-/jobs/7291792329#L2470
> >
> > Do you have an example of the check-whitespace job failing in GitLab CI?
> > Maybe I'm missing something, but I don't see a problem.
> >
> > -Justin
> 
> So I think I get the issue, GitLab has two kinds of pipelines it runs:
> 1. merge pipeline: Here the pipeline runs on the source branch (the
> feature branch which has to be merged).
> 2. merged pipeline: Here the pipeline creates a merge commit using the
> source and target branch and then runs the pipeline on the merged
> commit.
> 

Correct, this is my understanding.

> And "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is only defined in the 'merged
> pipeline'. If you see the pipelines for my branch on GitLab [1]. You'll
> see only one of them is marked as 'merge results' and the others being
> marked as 'merged results'. The former includes the job I mentioned
> above, where "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is not defined.
> 
> I'm still not sure why it marked only one of the pipelines as such, but
> this means there is chance that it could happen.

Huh, I'm guessing the CI job must have been triggered from the source
branch directly. Did you manually run the CI job? I wonder if that could
have caused it.

> 
> So I guess the best outcome is to use
> "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA", but fallback to
> "$CI_MERGE_REQUEST_DIFF_BASE_SHA", if the former is not defined.

This is exactly what I think we should do too. For merge pipelines we 
will want to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA so that only the 
commits included in the MR are scanned in CI. If that variable is not 
defined, it makes sense to fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA.
Since it's not a merge pipeline it will only scan commits included from
the MR and therefore work as expected.

This should handle both cases correctly. :)

-Justin
karthik nayak July 10, 2024, 1:38 p.m. UTC | #8
Justin Tobler <jltobler@gmail.com> writes:

[snip]

>>
>> You can see the output
>>
>>     $ ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA"
>>     fatal: ambiguous argument '': unknown revision or path not in the
>> working tree.
>>     Use '--' to separate paths from revisions, like this:
>>     'git <command> [<revision>...] -- [<file>...]'
>>
>> This only happens if "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is empty.
>
> Ya I noticed this failure, but was wondering if it was maybe due to
> something else. I have been unable to reproduce it and in all the jobs I
> was running resulted in a merge pipeline with the variable defined. But
> maybe sometimes a regular pipeline gets run for some reason and
> consequently $CI_MERGE_REQUEST_TARGET_BRANCH_SHA is not defined? Was the
> pipeline triggered directly from the source branch?
>

Just a regular push. Not sure at all why this happened. I was testing
different types of style issues on the CI and this happened once.

>>
>> > https://gitlab.com/gitlab-org/git/-/jobs/7291792329#L2470
>> >
>> > Do you have an example of the check-whitespace job failing in GitLab CI?
>> > Maybe I'm missing something, but I don't see a problem.
>> >
>> > -Justin
>>
>> So I think I get the issue, GitLab has two kinds of pipelines it runs:
>> 1. merge pipeline: Here the pipeline runs on the source branch (the
>> feature branch which has to be merged).
>> 2. merged pipeline: Here the pipeline creates a merge commit using the
>> source and target branch and then runs the pipeline on the merged
>> commit.
>>
>
> Correct, this is my understanding.
>
>> And "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is only defined in the 'merged
>> pipeline'. If you see the pipelines for my branch on GitLab [1]. You'll
>> see only one of them is marked as 'merge results' and the others being
>> marked as 'merged results'. The former includes the job I mentioned
>> above, where "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is not defined.
>>
>> I'm still not sure why it marked only one of the pipelines as such, but
>> this means there is chance that it could happen.
>
> Huh, I'm guessing the CI job must have been triggered from the source
> branch directly. Did you manually run the CI job? I wonder if that could
> have caused it.
>

Not that I remember.

>>
>> So I guess the best outcome is to use
>> "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA", but fallback to
>> "$CI_MERGE_REQUEST_DIFF_BASE_SHA", if the former is not defined.
>
> This is exactly what I think we should do too. For merge pipelines we
> will want to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA so that only the
> commits included in the MR are scanned in CI. If that variable is not
> defined, it makes sense to fallback to $CI_MERGE_REQUEST_DIFF_BASE_SHA.
> Since it's not a merge pipeline it will only scan commits included from
> the MR and therefore work as expected.
>
> This should handle both cases correctly. :)
>
> -Justin

Yeah seems like the best solution at this point, let me implement this.
diff mbox series

Patch

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
+#
+# Perform style check
+#
+
+baseCommit=$1
+
+git clang-format --style file --diff --extensions c,h "$baseCommit"