[PATCH] WIP: Output linters

[PATCH] WIP: Output linters

[ericbavier]

From: Eric Bavier <bavier@member.fsf.org>

I very much appreciate all that 'guix lint' can do, and thought that we could
get some benefit from extending its coverage to package outputs.  I wanted to
share this WIP patch to get some feedback on the idea.

The current patch just adds a simple check for the presence of build directory
strings in the output, which may affect build reproducibility across machines.
Other checks that might be useful might include checks:

* for "recent" timestamps, which might indicate use of __DATE__ or `date`,

* for presence of '.DIR' or other empty directories,

* for proper placement of documentation,

* for documentation that might best be moved to a "doc" output, or

* for self-contained pkg-config files, etc.

Any such checks obviously rely on the package outputs being in the store.  On
the one hand both local builds and substitutes are expensive.  But on the
other hand we'd like 'guix lint' to be run before someone submits a patch or
pushes their commits.  Being a good submitter, they hopefully went through the
trouble to test that the package builds, so the package outputs are mostly
likely in the store anyhow, and 'guix lint' wouldn't have any extra work to
do.

I'd like to hear from others whether they think this WIP has enough merit to
include in 'guix lint', and if so what other checks might be worth including.

Eric Bavier (1):
  gnu: lint: Check package outputs.

 guix/scripts/lint.scm | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

-- 
2.9.0

[PATCH] gnu: lint: Check package outputs.

[ericbavier]

From: Eric Bavier <bavier@member.fsf.org>

* guix/scripts/lint.scm (check-output): New procedure.
(%checkers): Add it.
---
 guix/scripts/lint.scm | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index b4fdb6f..64d4d76 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -29,6 +29,7 @@
   #:use-module (guix packages)
   #:use-module (guix licenses)
   #:use-module (guix records)
+  #:use-module (guix derivations)
   #:use-module (guix ui)
   #:use-module (guix utils)
   #:use-module (guix combinators)
@@ -45,6 +46,7 @@
                 #:select (maybe-expand-mirrors
                           open-connection-for-uri
                           close-connection))
+  #:use-module (guix build utils)
   #:use-module (web request)
   #:use-module (web response)
   #:use-module (srfi srfi-1)
@@ -581,6 +583,53 @@ descriptions maintained upstream."
                     (format #f (_ "failed to create derivation: ~s~%")
                             args)))))
 
+(define (check-output package)
+  "Emit warnings about common issues with a package's output.  This check is
+potentially very expensive; it may require a package to be built if the
+output is not already in the store."
+  (define check-build-dir
+    ;; Check for references to a temp build directory
+    (let ((build-dir-rx
+           (make-regexp "guix-build-[[:graphic:]]*\\.drv-[[:digit:]]+")))
+      (lambda (out)
+        (for-each
+         (lambda (file)
+           (call-with-input-file file
+             (lambda (port)
+               (let loop ((line-number 0))
+                 (let ((line (read-line port)))
+                   (unless (eof-object? line)
+                     (match (regexp-exec build-dir-rx
+                                         ;; (ice-9 regex) cannot handle
+                                         ;; strings with #\nul characters, so
+                                         ;; replace with something else.
+                                         (string-map
+                                          (λ (x) (if (eq? x #\nul) #\x01 x))
+                                          line))
+                       (#f
+                        (loop (1+ line-number)))
+                       (m
+                        (emit-warning package
+                                      (format #f (_ "build directory '~a' ~
+                                                     reference at ~a:~d:~d")
+                                              (match:substring m 0)
+                                              file line-number
+                                              (match:start m 0)))
+                        (loop (1+ line-number))))))))))
+         (find-files out #:directories? #f)))))
+
+  (define validate-output
+    (match-lambda
+      ((name . path)
+       (check-build-dir path))))
+
+  (with-store store
+    (let* ((drv (package-derivation store package #:graft? #f))
+           (outputs (derivation->output-paths drv)))
+      (build-derivations store (list drv))
+      ;; Now validate each output
+      (for-each validate-output outputs))))
+
 (define (check-license package)
   "Warn about type errors of the 'license' field of PACKAGE."
   (match (package-license package)
@@ -792,6 +841,10 @@ or a list thereof")
      (description "Report failure to compile a package to a derivation")
      (check       check-derivation))
    (lint-checker
+     (name        'output)
+     (description "Validate package output(s)")
+     (check       check-output))
+   (lint-checker
      (name        'synopsis)
      (description "Validate package synopses")
      (check       check-synopsis-style))
-- 
2.9.0

Re: [PATCH] WIP: Output linters

[Ludovic Courtès]

Hi!

ericbavier@openmailbox.org skribis:

> The current patch just adds a simple check for the presence of build directory
> strings in the output, which may affect build reproducibility across machines.
> Other checks that might be useful might include checks:
>
> * for "recent" timestamps, which might indicate use of __DATE__ or `date`,
>
> * for presence of '.DIR' or other empty directories,
>
> * for proper placement of documentation,
>
> * for documentation that might best be moved to a "doc" output, or
>
> * for self-contained pkg-config files, etc.

All good ideas!  This reminds me that Taylan had posted a .pc file
parser to check for dependencies that should be propagated; this could
be used as one of the checks.

> Any such checks obviously rely on the package outputs being in the store.  On
> the one hand both local builds and substitutes are expensive.  But on the
> other hand we'd like 'guix lint' to be run before someone submits a patch or
> pushes their commits.  Being a good submitter, they hopefully went through the
> trouble to test that the package builds, so the package outputs are mostly
> likely in the store anyhow, and 'guix lint' wouldn't have any extra work to
> do.
>
> I'd like to hear from others whether they think this WIP has enough merit to
> include in 'guix lint', and if so what other checks might be worth including.

So far such checks were done as extra build phases: ‘validate-runpath’
and ‘validate-documentation-location’.  The advantage is that they
cannot be skipped unwillingly; the build succeeds if and only if all the
checks passed.  The downside is that adding or modifying such a phase
leads to a full rebuild.  Something that is both an advantage and a
downside is that you get to test the rules on all the packages, so you
can (have to :-)) make sure they work well.

I think I prefer keeping such checks as build phases, although perhaps
there are cases where this is inconvenient.

WDYT?

Thanks,
Ludo’.

Re: [PATCH] WIP: Output linters

[Eric Bavier]

On Wed, 13 Jul 2016 12:32:48 +0200
ludo@gnu.org (Ludovic Courtès) wrote:

> Hi!
> 
> ericbavier@openmailbox.org skribis:
> 
> > The current patch just adds a simple check for the presence of build directory
> > strings in the output, which may affect build reproducibility across machines.
> > Other checks that might be useful might include checks:
> >
> > * for "recent" timestamps, which might indicate use of __DATE__ or `date`,
> >
> > * for presence of '.DIR' or other empty directories,
> >
> > * for proper placement of documentation,
> >
> > * for documentation that might best be moved to a "doc" output, or
> >
> > * for self-contained pkg-config files, etc.  
> 
> All good ideas!  This reminds me that Taylan had posted a .pc file
> parser to check for dependencies that should be propagated; this could
> be used as one of the checks.
> 
> > Any such checks obviously rely on the package outputs being in the store.  On
> > the one hand both local builds and substitutes are expensive.  But on the
> > other hand we'd like 'guix lint' to be run before someone submits a patch or
> > pushes their commits.  Being a good submitter, they hopefully went through the
> > trouble to test that the package builds, so the package outputs are mostly
> > likely in the store anyhow, and 'guix lint' wouldn't have any extra work to
> > do.
> >
> > I'd like to hear from others whether they think this WIP has enough merit to
> > include in 'guix lint', and if so what other checks might be worth including.  
> 
> So far such checks were done as extra build phases: ‘validate-runpath’
> and ‘validate-documentation-location’.  The advantage is that they
> cannot be skipped unwillingly; the build succeeds if and only if all the
> checks passed.  The downside is that adding or modifying such a phase
> leads to a full rebuild.  Something that is both an advantage and a
> downside is that you get to test the rules on all the packages, so you
> can (have to :-)) make sure they work well.
> 
> I think I prefer keeping such checks as build phases, although perhaps
> there are cases where this is inconvenient.

I agree with putting as many checks into build phases as we can.

Linter checks seem to be a good place for checking
issues that are non-fatal or of an "FYI" nature.  I would put in that
group checks for reproducibility issues, since currently we don't
consider non-reproducible builds to be a fatal issue (i.e. we don't
perform every build with --rounds=2). If we eventually declare that
every package needs to build cleanly with --rounds=2, then we could move
the relevant linter checks into build phases (to possibly catch issues
sooner in the build process), or remove the checks altogether (since
issues would presumably be caught with --rounds=2).

As an analogy: checks that would fall under Lintian's "Errors" type
should be a candidate for a build phase, and checks that would be
"Warnings" or "Info" should probably be in 'guix lint'.

https://lintian.debian.org/manual/section-2.3.html

`~Eric

Re: [PATCH] WIP: Output linters

[Ludovic Courtès]

Eric Bavier <ericbavier@openmailbox.org> skribis:

> On Wed, 13 Jul 2016 12:32:48 +0200
> ludo@gnu.org (Ludovic Courtès) wrote:
>
>> Hi!
>> 
>> ericbavier@openmailbox.org skribis:
>> 
>> > The current patch just adds a simple check for the presence of build directory
>> > strings in the output, which may affect build reproducibility across machines.
>> > Other checks that might be useful might include checks:
>> >
>> > * for "recent" timestamps, which might indicate use of __DATE__ or `date`,
>> >
>> > * for presence of '.DIR' or other empty directories,
>> >
>> > * for proper placement of documentation,
>> >
>> > * for documentation that might best be moved to a "doc" output, or
>> >
>> > * for self-contained pkg-config files, etc.  
>> 
>> All good ideas!  This reminds me that Taylan had posted a .pc file
>> parser to check for dependencies that should be propagated; this could
>> be used as one of the checks.
>> 
>> > Any such checks obviously rely on the package outputs being in the store.  On
>> > the one hand both local builds and substitutes are expensive.  But on the
>> > other hand we'd like 'guix lint' to be run before someone submits a patch or
>> > pushes their commits.  Being a good submitter, they hopefully went through the
>> > trouble to test that the package builds, so the package outputs are mostly
>> > likely in the store anyhow, and 'guix lint' wouldn't have any extra work to
>> > do.
>> >
>> > I'd like to hear from others whether they think this WIP has enough merit to
>> > include in 'guix lint', and if so what other checks might be worth including.  
>> 
>> So far such checks were done as extra build phases: ‘validate-runpath’
>> and ‘validate-documentation-location’.  The advantage is that they
>> cannot be skipped unwillingly; the build succeeds if and only if all the
>> checks passed.  The downside is that adding or modifying such a phase
>> leads to a full rebuild.  Something that is both an advantage and a
>> downside is that you get to test the rules on all the packages, so you
>> can (have to :-)) make sure they work well.
>> 
>> I think I prefer keeping such checks as build phases, although perhaps
>> there are cases where this is inconvenient.
>
> I agree with putting as many checks into build phases as we can.
>
> Linter checks seem to be a good place for checking
> issues that are non-fatal or of an "FYI" nature.  I would put in that
> group checks for reproducibility issues, since currently we don't
> consider non-reproducible builds to be a fatal issue (i.e. we don't
> perform every build with --rounds=2). If we eventually declare that
> every package needs to build cleanly with --rounds=2, then we could move
> the relevant linter checks into build phases (to possibly catch issues
> sooner in the build process), or remove the checks altogether (since
> issues would presumably be caught with --rounds=2).

I see.

I think checking for “/tmp/guix-build” references could well be a build
phase, though we need to test to see how well it would work (for
instance, the “debug” outputs should be excluded by default from this
check since they embed source file names, which are in
/tmp/guix-build-*).

But yeah, maybe we can start with a lint “checker” and eventually
migrate it to a build phase if that seems doable.  So your patch is a
step in the right direction.

A question is whether the checker should build the thing or simply skip
the test if the result isn’t already in the store.

Also, we’re starting to have checkers of different severity, and with
different computational costs.  We might start categorizing them so that
one can run just the “cheap” and “important” tests, say.

Thoughts?

Thanks,
Ludo’.