[PATCH] lint: Add 'check-git-protocol' checker.

  • Open
  • quality assurance status badge
Details
4 participants
  • Leo Famulari
  • Ludovic Courtès
  • Maxim Cournoyer
  • zimoun
Owner
unassigned
Submitted by
Leo Famulari
Severity
normal
L
L
Leo Famulari wrote on 30 Jan 2021 02:04
(address . guix-patches@gnu.org)
f9137838eca39b768e49f4ee7852dd32edce7e8c.1611968623.git.leo@famulari.name
We could also make it warn about use of the HTTP protocol (as opposed to
HTTPS). Your thoughts?

* guix/lint.scm (check-git-protocol): New procedure.
(%local-checkers): Add 'git-protocol' checker.
* doc/guix.texi (Invoking guix lint): Document it.
---
doc/guix.texi | 6 +++++-
guix/lint.scm | 25 ++++++++++++++++++++++++-
2 files changed, 29 insertions(+), 2 deletions(-)

Toggle diff (90 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index ff9e8da2e0..d17e2f2e96 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -28,7 +28,7 @@ Copyright @copyright{} 2014, 2015, 2016 Alex Kost@*
Copyright @copyright{} 2015, 2016 Mathieu Lirzin@*
Copyright @copyright{} 2014 Pierre-Antoine Rault@*
Copyright @copyright{} 2015 Taylan Ulrich Bayırlı/Kammer@*
-Copyright @copyright{} 2015, 2016, 2017, 2019, 2020 Leo Famulari@*
+Copyright @copyright{} 2015, 2016, 2017, 2019, 2020, 2021 Leo Famulari@*
Copyright @copyright{} 2015, 2016, 2017, 2018, 2019, 2020 Ricardo Wurmus@*
Copyright @copyright{} 2016 Ben Woodcroft@*
Copyright @copyright{} 2016, 2017, 2018 Chris Marusich@*
@@ -11736,6 +11736,10 @@ Parse the @code{source} URL to determine if a tarball from GitHub is
autogenerated or if it is a release tarball. Unfortunately GitHub's
autogenerated tarballs are sometimes regenerated.
+@item git-protocol
+Check if the package's source code is fetched using the insecure @code{git://}
+protocol.
+
@item derivation
Check that the derivation of the given packages can be successfully
computed for all the supported systems (@pxref{Derivations}).
diff --git a/guix/lint.scm b/guix/lint.scm
index 311bc94cc3..5a609b0454 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -11,6 +11,7 @@
;;; Copyright © 2018, 2019 Arun Isaac <arunisaac@systemreboot.net>
;;; Copyright © 2020 Chris Marusich <cmmarusich@gmail.com>
;;; Copyright © 2020 Timothy Sample <samplet@ngyro.com>
+;;; Copyright © 2021 Leo Famulari <leo@famulari.name>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -51,7 +52,7 @@
#:use-module (guix gnu-maintenance)
#:use-module (guix cve)
#:use-module ((guix swh) #:hide (origin?))
- #:autoload (guix git-download) (git-reference?
+ #:autoload (guix git-download) (git-reference? git-fetch
git-reference-url git-reference-commit)
#:use-module (guix import stackage)
#:use-module (ice-9 match)
@@ -84,6 +85,7 @@
check-source
check-source-file-name
check-source-unstable-tarball
+ check-git-protocol
check-mirror-url
check-github-url
check-license
@@ -918,6 +920,23 @@ descriptions maintained upstream."
(origin-uris origin))
'())))
+(define (check-git-protocol package)
+ "Emit a warning if PACKAGE's source URI protocol is 'git://'."
+ (define (check-source-uri-scheme uri)
+ (if (eqv? (uri-scheme uri) 'git)
+ (list
+ (make-warning package
+ (G_ "the source URI should not use the git:// protocol")
+ #:field 'source))
+ '()))
+
+ (let ((origin (package-source package)))
+ (if (and (origin? origin)
+ (eqv? (origin-method origin) git-fetch))
+ (check-source-uri-scheme
+ (string->uri (git-reference-url (origin-uri origin))))
+ '())))
+
(define (check-mirror-url package)
"Check whether PACKAGE uses source URLs that should be 'mirror://'."
(define (check-mirror-uri uri) ;XXX: could be optimized
@@ -1476,6 +1495,10 @@ or a list thereof")
(name 'source-unstable-tarball)
(description "Check for autogenerated tarballs")
(check check-source-unstable-tarball))
+ (lint-checker
+ (name 'git-protocol)
+ (description "Check for use of the git:// protocol")
+ (check check-git-protocol))
(lint-checker
(name 'derivation)
(description "Report failure to compile a package to a derivation")
--
2.30.0
Z
Z
zimoun wrote on 11 Mar 2021 01:14
(name . Leo Famulari)(address . leo@famulari.name)(address . 46182@debbugs.gnu.org)
86a6rabl7a.fsf@gmail.com
Hi Leo,

Giving a look to the bug tracker for the next release, I see this
bug. :-)


On Fri, 29 Jan 2021 at 20:04, Leo Famulari <leo@famulari.name> wrote:
Toggle quote (7 lines)
> We could also make it warn about use of the HTTP protocol (as opposed to
> HTTPS). Your thoughts?
>
> * guix/lint.scm (check-git-protocol): New procedure.
> (%local-checkers): Add 'git-protocol' checker.
> * doc/guix.texi (Invoking guix lint): Document it.

The doc/ does not apply anymore.


Instead of these ’eqv?’

Toggle quote (5 lines)
> +(define (check-git-protocol package)
> + "Emit a warning if PACKAGE's source URI protocol is 'git://'."
> + (define (check-source-uri-scheme uri)
> + (if (eqv? (uri-scheme uri) 'git)

[...]

Toggle quote (7 lines)
> + (let ((origin (package-source package)))
> + (if (and (origin? origin)
> + (eqv? (origin-method origin) git-fetch))
> + (check-source-uri-scheme
> + (string->uri (git-reference-url (origin-uri origin))))
> + '())))

I propose ’match’ which is more coherent with the Guix style. Well,
from my understanding. :-)

Patch attached. Well, it could be nice to add a test in
tests/guix-lint.sh for that. WDYT?


Cheers,
simon
Toggle diff (59 lines)
diff --git a/guix/lint.scm b/guix/lint.scm
index 311bc94cc3..980f77c736 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -51,7 +51,7 @@
#:use-module (guix gnu-maintenance)
#:use-module (guix cve)
#:use-module ((guix swh) #:hide (origin?))
- #:autoload (guix git-download) (git-reference?
+ #:autoload (guix git-download) (git-reference? git-fetch
git-reference-url git-reference-commit)
#:use-module (guix import stackage)
#:use-module (ice-9 match)
@@ -84,6 +84,7 @@
check-source
check-source-file-name
check-source-unstable-tarball
+ check-git-protocol
check-mirror-url
check-github-url
check-license
@@ -918,6 +919,26 @@ descriptions maintained upstream."
(origin-uris origin))
'())))
+(define (check-git-protocol package)
+ "Emit a warning if PACKAGE's source URI protocol is 'git://'."
+ (define (check-source-uri-scheme uri)
+ (match (uri-scheme uri)
+ ('git
+ (list
+ (make-warning package
+ (G_ "the source URI should not use the git:// protocol")
+ #:field 'source)))
+ (_ '())))
+
+ (match (package-source package)
+ ((? origin? origin)
+ (match (origin-method origin)
+ (git-fetch
+ (check-source-uri-scheme
+ (string->uri (git-reference-url (origin-uri origin)))))
+ (_ '())))
+ (_ '())))
+
(define (check-mirror-url package)
"Check whether PACKAGE uses source URLs that should be 'mirror://'."
(define (check-mirror-uri uri) ;XXX: could be optimized
@@ -1476,6 +1497,10 @@ or a list thereof")
(name 'source-unstable-tarball)
(description "Check for autogenerated tarballs")
(check check-source-unstable-tarball))
+ (lint-checker
+ (name 'git-protocol)
+ (description "Check for use of the git:// protocol")
+ (check check-git-protocol))
(lint-checker
(name 'derivation)
(description "Report failure to compile a package to a derivation")
L
L
Leo Famulari wrote on 11 Mar 2021 02:46
(name . zimoun)(address . zimon.toutoune@gmail.com)(address . 46182@debbugs.gnu.org)
YEl2fHXnDm6+GzFZ@jasmine.lan
On Thu, Mar 11, 2021 at 01:14:33AM +0100, zimoun wrote:
Toggle quote (3 lines)
> I propose ’match’ which is more coherent with the Guix style. Well,
> from my understanding. :-)

I have heard that before, but I don't know how to use it 🤷

If this new patch is working for you, please push!
Z
Z
zimoun wrote on 11 Mar 2021 10:44
(name . Leo Famulari)(address . leo@famulari.name)(address . 46182@debbugs.gnu.org)
86k0qe9g8u.fsf@gmail.com
Hi Leo,

On Wed, 10 Mar 2021 at 20:46, Leo Famulari <leo@famulari.name> wrote:
Toggle quote (6 lines)
> On Thu, Mar 11, 2021 at 01:14:33AM +0100, zimoun wrote:
>> I propose ’match’ which is more coherent with the Guix style. Well,
>> from my understanding. :-)
>
> I have heard that before, but I don't know how to use it 🤷

The section [1] in the manual is worth to read because running and
playing with the examples gives a good feeling on how to use it. :-)

Toggle quote (2 lines)
> If this new patch is working for you, please push!

I do not have this power. :-)


Cheers,
simon
L
L
Ludovic Courtès wrote on 11 Mar 2021 23:29
Re: bug#46182: [PATCH] lint: Add 'check-git-protocol' checker.
(name . Leo Famulari)(address . leo@famulari.name)(address . 46182@debbugs.gnu.org)
87zgz9uxwu.fsf@gnu.org
Hi!

Leo Famulari <leo@famulari.name> skribis:

Toggle quote (7 lines)
> We could also make it warn about use of the HTTP protocol (as opposed to
> HTTPS). Your thoughts?
>
> * guix/lint.scm (check-git-protocol): New procedure.
> (%local-checkers): Add 'git-protocol' checker.
> * doc/guix.texi (Invoking guix lint): Document it.

Nice! I think it’s OK to use ‘eqv?’ here (‘eq?’, even).

One nit: it would be nice to add a positive and a negative test in
tests/lint.scm. You can run “make check TESTS=tests/lint.scm” then.

Otherwise LGTM!

Thanks,
Ludo’.
M
M
Maxim Cournoyer wrote on 22 May 2022 06:15
(name . Leo Famulari)(address . leo@famulari.name)
87sfp26w4m.fsf_-_@gmail.com
tags 46182 +moreinfo
thanks

Hello,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (18 lines)
> Hi!
>
> Leo Famulari <leo@famulari.name> skribis:
>
>> We could also make it warn about use of the HTTP protocol (as opposed to
>> HTTPS). Your thoughts?
>>
>> * guix/lint.scm (check-git-protocol): New procedure.
>> (%local-checkers): Add 'git-protocol' checker.
>> * doc/guix.texi (Invoking guix lint): Document it.
>
> Nice! I think it’s OK to use ‘eqv?’ here (‘eq?’, even).
>
> One nit: it would be nice to add a positive and a negative test in
> tests/lint.scm. You can run “make check TESTS=tests/lint.scm” then.
>
> Otherwise LGTM!

Gentle ping to Leo :-).

It looks near ready.

Thank you!

Maxim
?