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

From: Justin Tobler <jltobler@gmail.com>
Date: Jul 9, 2024, 00:22 UTC
Message-ID: <lu7o24bwwh5ntarnpfjkmceuvzxs6rdykcrojonqkhfcvq7vka@7h3rev4n7wxr>
In-Reply-To: <CAOLa=ZTuBziBPY1B0P3gTWMhqThuyFsFW1aWtwUconYN4gH+aA@mail.gmail.com>

On 24/07/08 02:16PM, Karthik Nayak wrote:

Show 47 quoted lines

> 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

Show 10 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:

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