Re: [PATCH] WIP: Output linters

[Ludovic Courtès]

Eric Bavier <address@hidden> skribis:

> On Wed, 13 Jul 2016 12:32:48 +0200
> address@hidden (Ludovic Courtès) wrote:
>
>> Hi!
>> 
>> address@hidden 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’.