Re: [PATCH 1/2] scripts: add guix lint

[Ludovic Courtès]

Cyril Roelandt <address@hidden> skribis:

> * guix/scripts/lint.scm: New file. Defines a 'lint' tool for Guix packages.
> * Makefile.am (MODULES): Add it.

This is nice!

I think this should be modular so that, when possible, checkers can also
be invoked at macro expansion time.  For instance, style checkers for
synopses could run just when you build the file, without even having to
invoke ‘guix lint’ (but it’s nice to have both possibilities.)

Anyway, that’s a very good start, so the comment above is mostly food
for thought.

> +(define %default-options
> +  ;; Alist of default option values.
> +  `((format . , bytevector->nix-base32-string)))

Should be just '().

> +(define (emit-warning package s)
> + (format #t "~a: ~a~%" (package-full-name package) s))

There should be two indentation spaces, not one (the same problem shows
up in other places.)  Could you check that?

Besides, please and use ‘warning’ from (guix ui).

Also, it would be great to report location information, so that the
editor can directly jump to the offending line.  See how
build-aux/sync-descriptions.scm does it.

> +(define (list-checkers-and-exit)
> + (format #t "Available checkers:~%")

Should be (_ "Available checkers:~%").

> + (for-each (lambda (checker)
> +             (format #t "- ~a: ~a~%"
> +                     (lint-checker-name checker)
> +                     (lint-checker-description checker)))

Likewise, (gettext (lint-checker-description checker)).

This file also needs to be added to po/guix/POTFILES.in, and
po/guix/Makevars needs a --keyword=description as well.

> +(define (check-inputs-should-be-native package)
> + (let ((inputs (package-inputs package)))

Please add a docstring to this and other top-level procedures.

> +   (if (member "pkg-config" (map car inputs))
> +       (emit-warning package "pkg-config should probably be a native 
> input"))))

Please avoid looking at the ‘car’ of inputs, since it’s just a label.
Also, prefer ‘when’ to one-arm ‘if’.
So that should look like:

  (match inputs
    (((labels packages . _) ...)
     (when (member "pkg-config" (map package-name packages))
       (emit-warning ...))))

> +(define (check-synopsis-style package)
> +  (define (check-final-period synopsis)
> +    (if (string=? (string-take-right synopsis 1) ".")
> +        (emit-warning package
> +                      "No period allowed at the end of the synopsis.")))

The warning message should be lower-case, with no period.

> +       (emit-warning package
> +         "Filenames of patches should start by the package name"))))

“file names”, “start with”.

> +(define %checkers
> +  (list
> +   (make-lint-checker "inputs-should-be-native"
> +    "Identify inputs that should be native inputs"
> +    check-inputs-should-be-native)
> +   (make-lint-checker "patch-filenames"
> +    "Validate filenames of patches"
> +    check-patches)
> +   (make-lint-checker "synopsis"
> +    "Validate package synopsis"
> +    check-synopsis-style)))

Instead of ‘make-lint-checker’, use

  (lint-checker (name ...) (description "foo") ...)

This will allow xgettext to extract all the ‘description’ things, as
mentioned above.

> +(define (guix-lint . args)
> +  (define (parse-options)
> +    ;; Return the alist of option values.
> +    (args-fold* args %options
> +                (lambda (opt name arg result)
> +                  (leave (_ "~A: unrecognized option~%") name))
> +                (lambda (arg result)
> +                  (alist-cons 'argument arg result))
> +                %default-options))
> +
> +  (let* ((opts (parse-options))
> +         (args (filter-map (match-lambda
> +                            (('argument . value)
> +                             value)
> +                            (_ #f))
> +                           (reverse opts)))
> +         (fmt  (assq-ref opts 'format)))

‘fmt’ is unused, can be removed.

> + (if (null? args)
> +     (fold-packages (lambda (p r) (run-checkers p)) '())
> +     (for-each (lambda (name)
> +                 (let*-values (((name version)
> +                               (package-name->name+version name)))
> +                  (let ((packages (find-packages-by-name name version)))
> +                    (case (length packages)
> +                        ((0) (format #t "No such package")); XXX
> +                        ((1) (run-checkers (car packages)))
> +                        (else (format #t
> +                                      "More than one package found, be more 
> precise")))))); XXX

Please move ‘specification->package’ from (guix build scripts) to (guix
ui), and then use it instead of the above.

Thanks!

Ludo’.