[PATCH 0/3] gitlab: add jobs for checking paches

[PATCH 0/3] gitlab: add jobs for checking paches

[Daniel P. Berrangé]

This introduces two new jobs to GitLab. The first runs "checkpatch.pl"
across all patches, while the second is a dedicated DCO signoff check.

While checkpatch.pl does validate DCO signoff, it is sub-optimal as we
need to allow the checkpatch.pl job to fail as there are always patches
which intentionally violate some rules, and we've no mechanism for marking
permitted exceptions in code. Thus the checkpatch.pl jobs needs to be
non-fatal allowing failure.

By having a separate DCO job, we can make that particular job mandatory.

Checking patches themselves in GitLab CI is a little difficult, as the
CI job receives no indication of what the base ancestor was for the
branch being tested. To work around this, we add the master QEMU git
repo as a new remote and ask git to find the common ancestor vs the
branch being tested.

An example pipeline showing failure of these two jobs is here:

  https://gitlab.com/berrange/qemu/-/pipelines/191219666

The checkpatch.pl job failure output:

  https://gitlab.com/berrange/qemu/-/jobs/743439455

And the DCO signoff job failure output:

  https://gitlab.com/berrange/qemu/-/jobs/743439456

I think the latter shows the benefit of having a dedicated DCO signoff
job checker, as the info presented to the user is much clearer about
what they did wrong and how & why they must address it.

_+#          base:  master

Daniel P. Berrangé (3):
  gitlab: add a CI job for running checkpatch.pl
  gitlab: add a CI job to validate the DCO sign off
  gitlab: assign python helper files to GitLab maintainers section

 .gitlab-ci.d/check-dco.py   | 94 +++++++++++++++++++++++++++++++++++++
 .gitlab-ci.d/check-patch.py | 48 +++++++++++++++++++
 .gitlab-ci.yml              | 22 +++++++++
 MAINTAINERS                 |  1 +
 4 files changed, 165 insertions(+)
 create mode 100755 .gitlab-ci.d/check-dco.py
 create mode 100755 .gitlab-ci.d/check-patch.py

-- 
2.26.2

[PATCH 1/3] gitlab: add a CI job for running checkpatch.pl

[Daniel P. Berrangé]

This job is advisory since it is expected that certain patches will fail
the style checks and checkpatch.pl provides no way to mark exceptions to
the rules.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 .gitlab-ci.d/check-patch.py | 48 +++++++++++++++++++++++++++++++++++++
 .gitlab-ci.yml              | 12 ++++++++++
 2 files changed, 60 insertions(+)
 create mode 100755 .gitlab-ci.d/check-patch.py

diff --git a/.gitlab-ci.d/check-patch.py b/.gitlab-ci.d/check-patch.py
new file mode 100755
index 0000000000..5a14a25b13
--- /dev/null
+++ b/.gitlab-ci.d/check-patch.py
@@ -0,0 +1,48 @@
+#!/usr/bin/env python3
+#
+# check-patch.py: run checkpatch.pl across all commits in a branch
+#
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import os
+import os.path
+import sys
+import subprocess
+
+namespace = "qemu-project"
+if len(sys.argv) >= 2:
+    namespace = sys.argv[1]
+
+cwd = os.getcwd()
+reponame = os.path.basename(cwd)
+repourl = "https://gitlab.com/%s/%s.git" % (namespace, reponame)
+
+# GitLab CI environment does not give us any direct info about the
+# base for the user's branch. We thus need to figure out a common
+# ancestor between the user's branch and current git master.
+subprocess.check_call(["git", "remote", "add", "check-patch", repourl])
+subprocess.check_call(["git", "fetch", "check-patch", "master"],
+                      stdout=subprocess.DEVNULL,
+                      stderr=subprocess.DEVNULL)
+
+ancestor = subprocess.check_output(["git", "merge-base",
+                                    "check-patch/master", "HEAD"],
+                                   universal_newlines=True)
+
+ancestor = ancestor.strip()
+
+subprocess.check_call(["git", "remote", "rm", "check-patch"])
+
+errors = False
+
+print("\nChecking all commits since %s...\n" % ancestor)
+
+ret = subprocess.run(["scripts/checkpatch.pl", ancestor + "..."])
+
+if ret.returncode != 0:
+    print("    ❌ FAIL one or more commits failed scripts/checkpatch.pl")
+    sys.exit(1)
+
+sys.exit(0)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index a18e18b57e..3ed724c720 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -369,3 +369,15 @@ check-crypto-only-gnutls:
   variables:
     IMAGE: centos7
     MAKE_CHECK_ARGS: check
+
+
+check-patch:
+  stage: test
+  image: $CI_REGISTRY_IMAGE/qemu/centos8:latest
+  script: .gitlab-ci.d/check-patch.py
+  except:
+    variables:
+      - $CI_PROJECT_NAMESPACE == 'qemu-project' && $CI_COMMIT_BRANCH == 'master'
+  variables:
+    GIT_DEPTH: 1000
+  allow_failure: true
-- 
2.26.2

[PATCH 2/3] gitlab: add a CI job to validate the DCO sign off

[Daniel P. Berrangé]

While checkpatch.pl can validate DCO sign off that job must always be
advisory only since it is expected that certain patches will fail some
code style rules.

We require the DCO sign off to be mandatory for all commits though, so
it benefits from being validated in a standalone job.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 .gitlab-ci.d/check-dco.py | 94 +++++++++++++++++++++++++++++++++++++++
 .gitlab-ci.yml            | 10 +++++
 2 files changed, 104 insertions(+)
 create mode 100755 .gitlab-ci.d/check-dco.py

diff --git a/.gitlab-ci.d/check-dco.py b/.gitlab-ci.d/check-dco.py
new file mode 100755
index 0000000000..632c8bcce8
--- /dev/null
+++ b/.gitlab-ci.d/check-dco.py
@@ -0,0 +1,94 @@
+#!/usr/bin/env python3
+#
+# check-dco.py: validate all commits are signed off
+#
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import os
+import os.path
+import sys
+import subprocess
+
+namespace = "qemu-project"
+if len(sys.argv) >= 2:
+    namespace = sys.argv[1]
+
+cwd = os.getcwd()
+reponame = os.path.basename(cwd)
+repourl = "https://gitlab.com/%s/%s.git" % (namespace, reponame)
+
+subprocess.check_call(["git", "remote", "add", "check-dco", repourl])
+subprocess.check_call(["git", "fetch", "check-dco", "master"],
+                      stdout=subprocess.DEVNULL,
+                      stderr=subprocess.DEVNULL)
+
+ancestor = subprocess.check_output(["git", "merge-base",
+                                    "check-dco/master", "HEAD"],
+                                   universal_newlines=True)
+
+ancestor = ancestor.strip()
+
+subprocess.check_call(["git", "remote", "rm", "check-dco"])
+
+errors = False
+
+print("\nChecking for 'Signed-off-by: NAME <EMAIL>' " +
+      "on all commits since %s...\n" % ancestor)
+
+log = subprocess.check_output(["git", "log", "--format=%H %s",
+                               ancestor + "..."],
+                              universal_newlines=True)
+
+if log == "":
+    commits = []
+else:
+    commits = [[c[0:40], c[41:]] for c in log.strip().split("\n")]
+
+for sha, subject in commits:
+
+    msg = subprocess.check_output(["git", "show", "-s", sha],
+                                  universal_newlines=True)
+    lines = msg.strip().split("\n")
+
+    print("🔍 %s %s" % (sha, subject))
+    sob = False
+    for line in lines:
+        if "Signed-off-by:" in line:
+            sob = True
+            if "localhost" in line:
+                print("    ❌ FAIL: bad email in %s" % line)
+                errors = True
+
+    if not sob:
+        print("    ❌ FAIL missing Signed-off-by tag")
+        errors = True
+
+if errors:
+    print("""
+
+❌ ERROR: One or more commits are missing a valid Signed-off-By tag.
+
+
+This project requires all contributors to assert that their contributions
+are provided in compliance with the terms of the Developer's Certificate
+of Origin 1.1 (DCO):
+
+  https://developercertificate.org/
+
+To indicate acceptance of the DCO every commit must have a tag
+
+  Signed-off-by: REAL NAME <EMAIL>
+
+This can be achieved by passing the "-s" flag to the "git commit" command.
+
+To bulk update all commits on current branch "git rebase" can be used:
+
+  git rebase -i master -x 'git commit --amend --no-edit -s'
+
+""")
+
+    sys.exit(1)
+
+sys.exit(0)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 3ed724c720..b672f4ff23 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -381,3 +381,13 @@ check-patch:
   variables:
     GIT_DEPTH: 1000
   allow_failure: true
+
+check-dco:
+  stage: test
+  image: $CI_REGISTRY_IMAGE/qemu/centos8:latest
+  script: .gitlab-ci.d/check-dco.py
+  except:
+    variables:
+      - $CI_PROJECT_NAMESPACE == 'qemu-project' && $CI_COMMIT_BRANCH == 'master'
+  variables:
+    GIT_DEPTH: 1000
-- 
2.26.2

[PATCH 3/3] gitlab: assign python helper files to GitLab maintainers section

[Daniel P. Berrangé]

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3d17cad19a..b60981fb62 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3097,6 +3097,7 @@ R: Wainer dos Santos Moschetta <wainersm@redhat.com>
 S: Maintained
 F: .gitlab-ci.yml
 F: .gitlab-ci.d/crossbuilds.yml
+F: .gitlab-ci.d/*py
 
 Guest Test Compilation Support
 M: Alex Bennée <alex.bennee@linaro.org>
-- 
2.26.2

Re: [PATCH 0/3] gitlab: add jobs for checking paches

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200918132903.1848939-1-berrange@redhat.com/



Hi,

This series failed build test on FreeBSD host. Please find the details below.






The full log is available at
http://patchew.org/logs/20200918132903.1848939-1-berrange@redhat.com/testing.FreeBSD/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 1/3] gitlab: add a CI job for running checkpatch.pl

[Philippe Mathieu-Daudé]

On 9/18/20 3:29 PM, Daniel P. Berrangé wrote:
> This job is advisory since it is expected that certain patches will fail
> the style checks and checkpatch.pl provides no way to mark exceptions to
> the rules.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  .gitlab-ci.d/check-patch.py | 48 +++++++++++++++++++++++++++++++++++++
>  .gitlab-ci.yml              | 12 ++++++++++
>  2 files changed, 60 insertions(+)
>  create mode 100755 .gitlab-ci.d/check-patch.py
> 
> diff --git a/.gitlab-ci.d/check-patch.py b/.gitlab-ci.d/check-patch.py
> new file mode 100755
> index 0000000000..5a14a25b13
> --- /dev/null
> +++ b/.gitlab-ci.d/check-patch.py
> @@ -0,0 +1,48 @@
> +#!/usr/bin/env python3
> +#
> +# check-patch.py: run checkpatch.pl across all commits in a branch
> +#
> +# Copyright (C) 2020 Red Hat, Inc.
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +import os
> +import os.path
> +import sys
> +import subprocess
> +
> +namespace = "qemu-project"
> +if len(sys.argv) >= 2:
> +    namespace = sys.argv[1]
> +
> +cwd = os.getcwd()
> +reponame = os.path.basename(cwd)
> +repourl = "https://gitlab.com/%s/%s.git" % (namespace, reponame)
> +
> +# GitLab CI environment does not give us any direct info about the
> +# base for the user's branch. We thus need to figure out a common
> +# ancestor between the user's branch and current git master.
> +subprocess.check_call(["git", "remote", "add", "check-patch", repourl])
> +subprocess.check_call(["git", "fetch", "check-patch", "master"],
> +                      stdout=subprocess.DEVNULL,
> +                      stderr=subprocess.DEVNULL)
> +
> +ancestor = subprocess.check_output(["git", "merge-base",
> +                                    "check-patch/master", "HEAD"],
> +                                   universal_newlines=True)
> +
> +ancestor = ancestor.strip()
> +
> +subprocess.check_call(["git", "remote", "rm", "check-patch"])
> +
> +errors = False
> +
> +print("\nChecking all commits since %s...\n" % ancestor)
> +
> +ret = subprocess.run(["scripts/checkpatch.pl", ancestor + "..."])
> +
> +if ret.returncode != 0:
> +    print("    ❌ FAIL one or more commits failed scripts/checkpatch.pl")
> +    sys.exit(1)
> +
> +sys.exit(0)

Hmm I'm very tempted to add various check I've been reluctant to
add to checkpatch.pl here, and use check-patch.py instead...

Re: [PATCH 3/3] gitlab: assign python helper files to GitLab maintainers section

[Philippe Mathieu-Daudé]

On 9/18/20 3:29 PM, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3d17cad19a..b60981fb62 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3097,6 +3097,7 @@ R: Wainer dos Santos Moschetta <wainersm@redhat.com>
>  S: Maintained
>  F: .gitlab-ci.yml
>  F: .gitlab-ci.d/crossbuilds.yml
> +F: .gitlab-ci.d/*py

Squash in 1st patch?

Regardless:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 0/3] gitlab: add jobs for checking paches

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200918132903.1848939-1-berrange@redhat.com/



Hi,

This series failed build test on FreeBSD host. Please find the details below.






The full log is available at
http://patchew.org/logs/20200918132903.1848939-1-berrange@redhat.com/testing.FreeBSD/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/3] gitlab: add jobs for checking paches

[Thomas Huth]

On 18/09/2020 15.29, Daniel P. Berrangé wrote:
> This introduces two new jobs to GitLab. The first runs "checkpatch.pl"
> across all patches, while the second is a dedicated DCO signoff check.

This feels quite redundant since we're checking the patches with Patchew
already ... or are there plans to get rid of this check in Patchew?

 Thomas

Re: [PATCH 0/3] gitlab: add jobs for checking paches

[Philippe Mathieu-Daudé]

On 9/18/20 4:07 PM, Thomas Huth wrote:
> On 18/09/2020 15.29, Daniel P. Berrangé wrote:
>> This introduces two new jobs to GitLab. The first runs "checkpatch.pl"
>> across all patches, while the second is a dedicated DCO signoff check.
> 
> This feels quite redundant since we're checking the patches with Patchew
> already ... or are there plans to get rid of this check in Patchew?

The plan is to use GitLab-CI gating :)

Also this free patchew community resources and use the user's
resources instead. From a patchew sysadmin PoV this is a win
IMO.

> 
>  Thomas
> 

Re: [PATCH 0/3] gitlab: add jobs for checking paches

[Daniel P. Berrangé]

On Fri, Sep 18, 2020 at 04:07:22PM +0200, Thomas Huth wrote:
> On 18/09/2020 15.29, Daniel P. Berrangé wrote:
> > This introduces two new jobs to GitLab. The first runs "checkpatch.pl"
> > across all patches, while the second is a dedicated DCO signoff check.
> 
> This feels quite redundant since we're checking the patches with Patchew
> already ... or are there plans to get rid of this check in Patchew?

patchew only runs once the contributor has sent their patches to the
mailing list, whci his too late.

We want contributors to test their series in GitLab CI ahead of sending
it, so that patchew never has to report any failure, because the code is
already perfect once on the list (except if git master has moved and
causes conflicts of course).

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/3] gitlab: add jobs for checking paches

[罗勇刚(Yonggang Luo)]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

On Fri, Sep 18, 2020 at 10:16 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:
>
> On Fri, Sep 18, 2020 at 04:07:22PM +0200, Thomas Huth wrote:
> > On 18/09/2020 15.29, Daniel P. Berrangé wrote:
> > > This introduces two new jobs to GitLab. The first runs "checkpatch.pl"
> > > across all patches, while the second is a dedicated DCO signoff check.
> >
> > This feels quite redundant since we're checking the patches with Patchew
> > already ... or are there plans to get rid of this check in Patchew?
>
> patchew only runs once the contributor has sent their patches to the
> mailing list, whci his too late.
>
> We want contributors to test their series in GitLab CI ahead of sending
> it, so that patchew never has to report any failure, because the code is
> already perfect once on the list (except if git master has moved and
> causes conflicts of course).
>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-
https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
https://www.instagram.com/dberrange :|
>
>
agreed, and ineed patchew are broken now.

--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

[-- Attachment #2: Type: text/html, Size: 1862 bytes --]

Re: [PATCH 0/3] gitlab: add jobs for checking paches

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200918132903.1848939-1-berrange@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.






The full log is available at
http://patchew.org/logs/20200918132903.1848939-1-berrange@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 1/3] gitlab: add a CI job for running checkpatch.pl

[Thomas Huth]

On 18/09/2020 15.29, Daniel P. Berrangé wrote:
> This job is advisory since it is expected that certain patches will fail
> the style checks and checkpatch.pl provides no way to mark exceptions to
> the rules.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  .gitlab-ci.d/check-patch.py | 48 +++++++++++++++++++++++++++++++++++++
>  .gitlab-ci.yml              | 12 ++++++++++
>  2 files changed, 60 insertions(+)
>  create mode 100755 .gitlab-ci.d/check-patch.py
[...]
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index a18e18b57e..3ed724c720 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -369,3 +369,15 @@ check-crypto-only-gnutls:
>    variables:
>      IMAGE: centos7
>      MAKE_CHECK_ARGS: check
> +
> +
> +check-patch:
> +  stage: test

Would it be ok to move this to the "build" stage, so that the job does not
have to wait for all the slow build jobs to finish?
(same question applies for the next patch, too)

If you agree, I can do the change when picking up the patches, no need to
resend just because of this.

 Thomas

Re: [PATCH 1/3] gitlab: add a CI job for running checkpatch.pl

[Daniel P. Berrangé]

On Tue, Oct 13, 2020 at 10:08:56AM +0200, Thomas Huth wrote:
> On 18/09/2020 15.29, Daniel P. Berrangé wrote:
> > This job is advisory since it is expected that certain patches will fail
> > the style checks and checkpatch.pl provides no way to mark exceptions to
> > the rules.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  .gitlab-ci.d/check-patch.py | 48 +++++++++++++++++++++++++++++++++++++
> >  .gitlab-ci.yml              | 12 ++++++++++
> >  2 files changed, 60 insertions(+)
> >  create mode 100755 .gitlab-ci.d/check-patch.py
> [...]
> > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > index a18e18b57e..3ed724c720 100644
> > --- a/.gitlab-ci.yml
> > +++ b/.gitlab-ci.yml
> > @@ -369,3 +369,15 @@ check-crypto-only-gnutls:
> >    variables:
> >      IMAGE: centos7
> >      MAKE_CHECK_ARGS: check
> > +
> > +
> > +check-patch:
> > +  stage: test
> 
> Would it be ok to move this to the "build" stage, so that the job does not
> have to wait for all the slow build jobs to finish?
> (same question applies for the next patch, too)
> 
> If you agree, I can do the change when picking up the patches, no need to
> resend just because of this.

Sure, fine with me.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|