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

[Laurent Pinchart]

On Sat, Jun 15, 2024 at 01:55:02PM +0100, Kieran Bingham wrote:
> 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.

I like that idea.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > > +
> > > +
> > >  # ------------------------------------------------------------------------------
> > >  # Test stage - Run unit tests and hardware tests
> > >  # ------------------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart