[bug#63263] [PATCH] gexp: Stop generating unreadable builder scripts.

[Christopher Baines]

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

> Hi,
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> In Guile, it's possible to produce output from write that can't be read, and
>> this applies to the code staged through g-expressions for derivations.  This
>> commit detects this early when the derivation is being created, rather than
>> leaving the error to happen when the derivation is built.
>>
>> This is important as it means that tools like guix lint will indicate that
>> there's a problem, hopefully reducing the number of broken derivations in
>> Guix.
>>
>> * guix/gexp.scm (gexp->derivation): Check that the builder script can be 
>> read.
>
> Calling ‘read’ on every generated sexp is definitely not something we
> should do, performance-wise.
>
> Commit 24ab804ce11fe12ff49cd144a3d9c4bfcf55b41c addressed that to some
> extent.  It works in examples like this:
>
> scheme@(guile-user)> ,lower (computed-file "foo" #~(list #$(current-module)))
> While executing meta-command:
> ERROR:
>   1. &gexp-input-error: #<directory (guile-user) 7f26d5918c80>
>
>
> … where ‘current-module’ returns a non-serializable object.
>
> I think the problem you’re trying to address that we frequently
> encounter is old-style packages that end up splicing gexps inside sexps,
> as in:
>
>   (package
>     ;; …
>     (arguments `(#:phases (modify-phases whatever ,#~doh!))))
>
> Is that right?

I think so, I can't remember if I've seen any other ways that this
happens.

> The problem here is that ‘sexp->gexp’, which was added precisely as an
> optimization for build systems¹, does not check the sexp it’s given.
> Example:
>
> scheme@(guile-user)> ,lower (computed-file "foo" (sexp->gexp `(list a b c 
> ,(current-module))))
> $19 = #<derivation /gnu/store/j5rgrmdzk4mic67zkal4759bcm5xbk1c-foo.drv =>  
> 7f26baf56be0>
> scheme@(guile-user)> (sexp->gexp `(list a b c ,(current-module)))
> $20 = #<gexp (list a b c #<directory (guile-user) 7f26d5918c80>) 7f26bbf2f090>
>
> Oops!
>
> It would be tempting to change ‘sexp->gexp’ to traverse the sexp in
> search of non-serializable things… but that’d defeat the whole point of
> ‘sexp->gexp’.
>
> How about a linter instead, with the understanding that use of sexps in
> packages is vanishing?  Perhaps coupled with a ‘guix style’ automatic
> rewriter.

A linter might be helpful, but I'm not sure it'll help that much.

I think it's quite a lofty expectation for the linter to be run on
packages that are edited, let alone on the packages affected by those
changes (which is what's needed to catch this problem), so adding a
linter will mean we get lint warnings, but we'll still be living with
these broken derivations.

The builds for affected derivations fail immediately, and it's pretty
obvious from the log that the builder is unreadable, so it should
already be possible to spot this problem from looking at the effect of
package changes on builds, so I think the main way a linter will help is
that it would provide a way to find out what derivations are broken in
this way, without attempting to build all of them.

I guess my perspective on this is more from the operation of the guix
data service, which is carefully computing and storing all of these
broken derivations (and there's a lot, like 10,000+ per revision at the
moment, since they change every time you compute them).  This then
propagates down to the build coordinator as well, since there's builds
being submitted for all these broken derivations. I have considered
trying to detect these breakages in the data service, but I'm not sure
how to do it while removing the possibility of false positives.

signature.asc
Description: PGP signature