[PATCH] lint: Add pre-merge checks using pre-push hook

[Kieran Bingham]

Quoting Laurent Pinchart (2024-06-14 19:24:20)
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Jun 14, 2024 at 04:57:18PM +0100, Kieran Bingham wrote:
> > Wrap the existing libcamera pre-push hook into a lint task to make sure
> > that the rules we apply to merging are conveyed as part of the CI jobs.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  .gitlab-ci/lint-pre-push.sh | 27 +++++++++++++++++++++++++++
> >  gitlab-ci.yml               | 16 +++++++++++++++-
> >  2 files changed, 42 insertions(+), 1 deletion(-)
> >  create mode 100755 .gitlab-ci/lint-pre-push.sh
> > 
> > diff --git a/.gitlab-ci/lint-pre-push.sh b/.gitlab-ci/lint-pre-push.sh
> > new file mode 100755
> > index 000000000000..8a4283cc0f15
> > --- /dev/null
> > +++ b/.gitlab-ci/lint-pre-push.sh
> > @@ -0,0 +1,27 @@
> > +#!/bin/bash
> > +
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +# SPDX-FileCopyrightText: © 2023 Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > +#
> > +# Pre-merge checks
> > +
> > +set -e
> > +
> > +source "$(dirname "$0")/lib.sh"
> > +
> > +libcamera_premerge() {
> > +     echo "Running Pre-merge checks for $CI_COMMIT_REF_NAME ($base..$CI_COMMIT_SHA)"
> > +
> > +     # Wrap parameters as git would send them to the pre-push hooks directly.
> > +     echo "$CI_COMMIT_REF_NAME $CI_COMMIT_SHA refs/heads/integration/pre-push-lint-test $base" \
> > +             | ./utils/hooks/pre-push origin "$CI_DEFAULT_BRANCH"
> > +}
> > +
> > +base=$(find_base origin/$CI_DEFAULT_BRANCH $CI_COMMIT_SHA)
> > +
> > +if [[ $base == $CI_COMMIT_SHA ]] ; then
> > +     echo "No commit to test, skipping"
> > +     exit 0
> > +fi
> > +
> > +run libcamera_premerge
> > diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> > index 50b81e591458..d9b31ec8259e 100644
> > --- a/gitlab-ci.yml
> > +++ b/gitlab-ci.yml
> > @@ -311,7 +311,7 @@ build-package:cros:
> >        dotenv: env
> >  
> >  # ------------------------------------------------------------------------------
> > -# Lint stage - Run checkstyle.py
> > +# Lint stage - Run checkstyle.py and check merge suitability
> >  # ------------------------------------------------------------------------------
> >  
> >  lint:
> > @@ -330,6 +330,20 @@ lint:
> >    script:
> >      - $CI_PROJECT_DIR/.gitlab-ci/lint-libcamera.sh
> >  
> > +merge-check:
> > +  extends:
> > +    - .fdo.distribution-image at debian
> > +    - .history-jobs
> > +    - .libcamera-ci.debian:12
> > +    - .libcamera-ci.scripts
> > +  stage: lint
> > +  needs:
> > +    - job: container-debian:12
> > +      artifacts: false
> > +  script:
> > +    - $CI_PROJECT_DIR/.gitlab-ci/lint-pre-push.sh
> 
> Making this a hard failure means that we'll have to allow merging
> branches that fail CI in some cases (I'm thinking for instance of urgent
> fixes to the master branch that can't get a review in time). For now

I think if one of us is merging a patch without a review, they can
supply an 'Acked-by: tag' to acknowledge that responsibility.

That's how I get around the merge rule on release commits which I don't
get review tags for.

I can't imagine anything really being  'so urgent' it can't get an 'Ok
sure - go ahead and merge that'. We don't have hard-realtime constraints
on merging.

> that's not a problem as CI doesn't gate merging. If that changes in the
> future we'll have to either make sure we can merge with failed CI
> pipelines, or update this check someone. That's an issue with can deal
> with later, so

Yes, I think this is something we can consider if it comes up. And at
the moment, I think the likelyhood should be low - because the whole
point of the CI is to make the likelyhood lower that we merge anything
unsuitable in the first place.


And this specific instance can be resolved with an:

Acked-by: Author <of at commit.com>, which even publicly records that the
action was taken to bypass a check.

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Thanks.

Kieran


> 
> > +
> > +
> >  # ------------------------------------------------------------------------------
> >  # Test stage - Run unit tests and hardware tests
> >  # ------------------------------------------------------------------------------
> 
> -- 
> Regards,
> 
> Laurent Pinchart