diff mbox series

[bug#35790,4/4] lint: Separate checkers by dependence on the internet.

Message ID 20190715194558.13804-4-mail@cbaines.net
State Accepted
Headers show
Series [bug#35790,1/4] scripts: lint: Handle warnings with a record type. | expand

Commit Message

Christopher Baines July 15, 2019, 7:45 p.m. UTC 
I think there are a couple of potential uses for this. It's somewhat a
separation in to what checkers are just checking the contents of the
repository (line length for example), and other checkers which are bringing in
external information which could change.

I'm thinking particularly, about treating network dependent checkers
differently when automatically running them, but this commit also adds a
--no-network flag to guix lint, which selects the checkers that don't access
the network, which could be useful if no network access is available.

* guix/lint.scm (%checkers): Rename to %all-checkers.
(%local-checkers, %network-dependent-checkers): New variables.
* guix/scripts/lint.scm (run-checkers): Make the checkers argument mandatory.
(list-checkers-and-exit): Handle the checkers as an argument.
(%options): Adjust for changes to %checkers, add a --no-network option, and
change how the --list-checkers option is handled.
(guix-lint): Adjust indentation, and update how the checkers are handled.
---
 guix/lint.scm         | 63 ++++++++++++++++++++++++-------------------
 guix/scripts/lint.scm | 49 ++++++++++++++++++++-------------
 2 files changed, 66 insertions(+), 46 deletions(-)

Comments

Ludovic Courtès July 15, 2019, 8:17 p.m. UTC | #1 
Hi!

It seems to me we’re all set now.

Thanks a lot for all the work and for your patience!

Ludo’.
Christopher Baines July 15, 2019, 10:23 p.m. UTC | #2 
Ludovic Courtès <ludo@gnu.org> writes:

> Hi!
>
> It seems to me we’re all set now.

Great, I've pushed these to master now.

> Thanks a lot for all the work and for your patience!

No problem :)

In terms of next steps, I think this is a big bit of the work needed to
get lint warnings in to the Guix Data Service done, but there's still a
big chunk to do.

I hope to start looking at actually trying to load in the lint warnings
soon. This might involve extending the inferior API if that's helpful. I
also want to attempt to store translations for the lint warnings in one
way or another, as that'll begin to address the lack of localisation in
the Guix Data Service.

There's also some thinking about how to manage the network dependent
checkers. I'd like to get that information in anyway, but also, I think
it might be possible to maybe separate out the network independant parts
of the checkers that are currently in the network dependent list. For
example, the synopsis checker is only in there as it attempts to connect
to the network to check if packages are a GNU package, and I'm wondering
if that can be avoided.

Anyway, hopefully the code refactoring is generally helpful, and maybe
the --no-network option for guix lint will come in useful as well.

Chris
Ludovic Courtès July 16, 2019, 9:34 p.m. UTC | #3 
Hi!

Christopher Baines <mail@cbaines.net> skribis:

> Great, I've pushed these to master now.

Yay!  \o/

> I hope to start looking at actually trying to load in the lint warnings
> soon. This might involve extending the inferior API if that's helpful. I
> also want to attempt to store translations for the lint warnings in one
> way or another, as that'll begin to address the lack of localisation in
> the Guix Data Service.

Regarding inferiors, you could always build up an “inferior lint
warning” API, similar to what’s done for <inferior-package>, and
likewise for l10n.

It needs some thought because we don’t want to mirror every single Guix
API with an inferior equivalent.  So perhaps you can run a large part of
the processing in the inferior.

> There's also some thinking about how to manage the network dependent
> checkers. I'd like to get that information in anyway, but also, I think
> it might be possible to maybe separate out the network independant parts
> of the checkers that are currently in the network dependent list. For
> example, the synopsis checker is only in there as it attempts to connect
> to the network to check if packages are a GNU package, and I'm wondering
> if that can be avoided.

Dunno, but I don’t think it’s super important either.

What might be more useful is to indicate how critical a warning is: lack
of source code is critical, but missing-space-after-period less so.

> Anyway, hopefully the code refactoring is generally helpful, and maybe
> the --no-network option for guix lint will come in useful as well.

Definitely.

Thanks!

Ludo’.
diff mbox series

Patch

diff --git a/guix/lint.scm b/guix/lint.scm
index c2c0914958..2542a81a2d 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -91,7 +91,9 @@ 
             lint-warning-message-data
             lint-warning-location
 
-            %checkers
+            %local-checkers
+            %network-dependent-checkers
+            %all-checkers
 
             lint-checker
             lint-checker?
@@ -1146,16 +1148,12 @@  them for PACKAGE."
 ;;; List of checkers.
 ;;;
 
-(define %checkers
+(define %local-checkers
   (list
    (lint-checker
      (name        'description)
      (description "Validate package descriptions")
      (check       check-description-style))
-   (lint-checker
-     (name        'gnu-description)
-     (description "Validate synopsis & description of GNU packages")
-     (check       check-gnu-synopsis+description))
    (lint-checker
      (name        'inputs-should-be-native)
      (description "Identify inputs that should be native inputs")
@@ -1164,14 +1162,6 @@  them for PACKAGE."
      (name        'inputs-should-not-be-input)
      (description "Identify inputs that shouldn't be inputs at all")
      (check       check-inputs-should-not-be-an-input-at-all))
-   (lint-checker
-     (name        'patch-file-names)
-     (description "Validate file names and availability of patches")
-     (check       check-patch-file-names))
-   (lint-checker
-     (name        'home-page)
-     (description "Validate home-page URLs")
-     (check       check-home-page))
    (lint-checker
      (name        'license)
      ;; TRANSLATORS: <license> is the name of a data type and must not be
@@ -1179,18 +1169,10 @@  them for PACKAGE."
      (description "Make sure the 'license' field is a <license> \
 or a list thereof")
      (check       check-license))
-   (lint-checker
-     (name        'source)
-     (description "Validate source URLs")
-     (check       check-source))
    (lint-checker
      (name        'mirror-url)
      (description "Suggest 'mirror://' URLs")
      (check       check-mirror-url))
-   (lint-checker
-     (name        'github-url)
-     (description "Suggest GitHub URLs")
-     (check       check-github-url))
    (lint-checker
      (name        'source-file-name)
      (description "Validate file names of sources")
@@ -1203,10 +1185,37 @@  or a list thereof")
      (name        'derivation)
      (description "Report failure to compile a package to a derivation")
      (check       check-derivation))
+   (lint-checker
+    (name        'patch-file-names)
+    (description "Validate file names and availability of patches")
+    (check       check-patch-file-names))
+   (lint-checker
+     (name        'formatting)
+     (description "Look for formatting issues in the source")
+     (check       check-formatting))))
+
+(define %network-dependent-checkers
+  (list
    (lint-checker
      (name        'synopsis)
      (description "Validate package synopses")
      (check       check-synopsis-style))
+   (lint-checker
+     (name        'gnu-description)
+     (description "Validate synopsis & description of GNU packages")
+     (check       check-gnu-synopsis+description))
+   (lint-checker
+     (name        'home-page)
+     (description "Validate home-page URLs")
+     (check       check-home-page))
+   (lint-checker
+     (name        'source)
+     (description "Validate source URLs")
+     (check       check-source))
+   (lint-checker
+     (name        'github-url)
+     (description "Suggest GitHub URLs")
+     (check       check-github-url))
    (lint-checker
      (name        'cve)
      (description "Check the Common Vulnerabilities and Exposures\
@@ -1215,8 +1224,8 @@  or a list thereof")
    (lint-checker
      (name        'refresh)
      (description "Check the package for new upstream releases")
-     (check       check-for-updates))
-   (lint-checker
-     (name        'formatting)
-     (description "Look for formatting issues in the source")
-     (check       check-formatting))))
+     (check       check-for-updates))))
+
+(define %all-checkers
+  (append %local-checkers
+          %network-dependent-checkers))
diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 1c46fba16b..98ee469501 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -52,7 +52,7 @@ 
                (lint-warning-message lint-warning))))
    warnings))
 
-(define* (run-checkers package #:optional (checkers %checkers))
+(define (run-checkers package checkers)
   "Run the given CHECKERS on PACKAGE."
   (let ((tty? (isatty? (current-error-port))))
     (for-each (lambda (checker)
@@ -68,14 +68,14 @@ 
       (format (current-error-port) "\x1b[K")
       (force-output (current-error-port)))))
 
-(define (list-checkers-and-exit)
+(define (list-checkers-and-exit checkers)
   ;; Print information about all available checkers and exit.
   (format #t (G_ "Available checkers:~%"))
   (for-each (lambda (checker)
               (format #t "- ~a: ~a~%"
                       (lint-checker-name checker)
                       (G_ (lint-checker-description checker))))
-            %checkers)
+            checkers)
   (exit 0))
 
 
@@ -111,26 +111,33 @@  run the checkers on all packages.\n"))
   ;;                                  'certainty'.
   (list (option '(#\c "checkers") #t #f
                 (lambda (opt name arg result)
-                  (let ((names (map string->symbol (string-split arg #\,))))
+                  (let ((names (map string->symbol (string-split arg #\,)))
+                        (checker-names (map lint-checker-name %all-checkers)))
                     (for-each (lambda (c)
-                                (unless (memq c
-                                              (map lint-checker-name
-                                                   %checkers))
+                                (unless (memq c checker-names)
                                   (leave (G_ "~a: invalid checker~%") c)))
                               names)
                     (alist-cons 'checkers
                                 (filter (lambda (checker)
                                           (member (lint-checker-name checker)
                                                   names))
-                                        %checkers)
+                                        %all-checkers)
                                 result))))
+        (option '(#\n "no-network") #f #f
+                (lambda (opt name arg result)
+                  (alist-cons 'checkers
+                              %local-checkers
+                              (alist-delete 'checkers
+                                            result))))
         (option '(#\h "help") #f #f
                 (lambda args
                   (show-help)
                   (exit 0)))
         (option '(#\l "list-checkers") #f #f
-                (lambda args
-                   (list-checkers-and-exit)))
+                (lambda (opt name arg result)
+                  (alist-cons 'list?
+                              #t
+                              result)))
         (option '(#\V "version") #f #f
                 (lambda args
                   (show-version-and-exit "guix lint")))))
@@ -148,13 +155,17 @@  run the checkers on all packages.\n"))
 
   (let* ((opts (parse-options))
          (args (filter-map (match-lambda
-                            (('argument . value)
-                             value)
-                            (_ #f))
+                             (('argument . value)
+                              value)
+                             (_ #f))
                            (reverse opts)))
-         (checkers (or (assoc-ref opts 'checkers) %checkers)))
-     (if (null? args)
-          (fold-packages (lambda (p r) (run-checkers p checkers)) '())
-          (for-each (lambda (spec)
-                      (run-checkers (specification->package spec) checkers))
-                    args))))
+         (checkers (or (assoc-ref opts 'checkers) %all-checkers)))
+    (cond
+     ((assoc-ref opts 'list?)
+      (list-checkers-and-exit checkers))
+     ((null? args)
+      (fold-packages (lambda (p r) (run-checkers p checkers)) '()))
+     (else
+      (for-each (lambda (spec)
+                  (run-checkers (specification->package spec) checkers))
+                args)))))