| Message ID | 20240614155718.10902-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Kieran Bingham (2024-06-14 16:57:18) > 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. > A couple of test/examples: Successful pipeline: https://gitlab.freedesktop.org/kbingham/libcamera/-/pipelines/1201492 Failed pipeline: https://gitlab.freedesktop.org/kbingham/libcamera/-/pipelines/1201497 -- Kieran > Signed-off-by: Kieran Bingham <kieran.bingham@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@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@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 > + > + > # ------------------------------------------------------------------------------ > # Test stage - Run unit tests and hardware tests > # ------------------------------------------------------------------------------ > -- > 2.34.1 >
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@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@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@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 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 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + > # ------------------------------------------------------------------------------ > # Test stage - Run unit tests and hardware tests > # ------------------------------------------------------------------------------
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@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@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@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@commit.com>, which even publicly records that the action was taken to bypass a check. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks. Kieran > > > + > > + > > # ------------------------------------------------------------------------------ > > # Test stage - Run unit tests and hardware tests > > # ------------------------------------------------------------------------------ > > -- > Regards, > > 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@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@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@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@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@ideasonboard.com> > > > > + > > > + > > > # ------------------------------------------------------------------------------ > > > # Test stage - Run unit tests and hardware tests > > > # ------------------------------------------------------------------------------
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@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@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 + + # ------------------------------------------------------------------------------ # Test stage - Run unit tests and hardware tests # ------------------------------------------------------------------------------
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@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