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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 9, 2024, 08:44 UTC
Message-ID: <CAOLa=ZQiHcCU9G6BFO98Y1VAPYgTDdJzbWC5bnMQwx9jsTJ0tg@mail.gmail.com>
In-Reply-To: <lu7o24bwwh5ntarnpfjkmceuvzxs6rdykcrojonqkhfcvq7vka@7h3rev4n7wxr>

Justin Tobler <jltobler@gmail.com> writes:

Show 64 quoted lines

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

Show 15 quoted lines

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

Show 6 quoted lines

> 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