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

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

[Christopher Baines]

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.
---
 guix/gexp.scm | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/guix/gexp.scm b/guix/gexp.scm
index 0fe4f1c98a..7af9302ccf 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -1215,9 +1215,18 @@ (define (add-modules exp modules)
                                                          #:target target)
                                        (return #f)))
                        (guile -> (lowered-gexp-guile lowered))
-                       (builder  (text-file script-name
-                                            (sexp->string
-                                             (lowered-gexp-sexp lowered)))))
+                       (builder  (text-file
+                                  script-name
+                                  (let ((builder-string
+                                         (sexp->string
+                                          (lowered-gexp-sexp lowered))))
+                                    (catch 'read-error
+                                      (lambda ()
+                                        (call-with-input-string builder-string
+                                          read)
+                                        builder-string)
+                                      (lambda (key . args)
+                                        (error "invalid gexp" name exp  args)))))))
     (mbegin %store-monad
       (set-grafting graft?)                       ;restore the initial setting
       (raw-derivation name
-- 
2.39.1

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

[Ludovic Courtès]

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:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> ,lower (computed-file "foo" #~(list #$(current-module)))
While executing meta-command:
ERROR:
  1. &gexp-input-error: #<directory (guile-user) 7f26d5918c80>
--8<---------------cut here---------------end--------------->8---

… 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?

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:

--8<---------------cut here---------------start------------->8---
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>
--8<---------------cut here---------------end--------------->8---

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.

Thanks,
Ludo’.

¹ Packages would get long lists/trees in their ‘arguments’ field.
  Traversing them in search of lowerable objects is costly, and
  ‘sexp->gexp’ was introduced precisely do we don’t have to traverse the
  sexp.  (Gexps are designed so that no such traversal is necessary at
  run time.)

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

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 3743 bytes --]


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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

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

[Josselin Poiret via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]

Hi Chris and Ludo,

Christopher Baines <mail@cbaines.net> writes:

> 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.

I guess you already read the derivations from the data service to find
out what has changed, right?  Would you also be able to try to read the
builder script from there, before trying to build?  And if the
derivation is bad, signal it somehow and flag it for some sort of gc?
Although then, all other derivations depending on it would also need to
be gc'd, which might be annoying.

I don't know if the data service's architecture would allow this to be
done before trying to build derivations though, sorry in advance if that
would be too much work.

Best,
-- 
Josselin Poiret

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 682 bytes --]

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

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]


Josselin Poiret <dev@jpoiret.xyz> writes:

> Christopher Baines <mail@cbaines.net> writes:
>
>> 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.
>
> I guess you already read the derivations from the data service to find
> out what has changed, right?  Would you also be able to try to read the
> builder script from there, before trying to build?  And if the
> derivation is bad, signal it somehow and flag it for some sort of gc?
> Although then, all other derivations depending on it would also need to
> be gc'd, which might be annoying.
>
> I don't know if the data service's architecture would allow this to be
> done before trying to build derivations though, sorry in advance if that
> would be too much work.

It would do, but I'm not sure this would be as reliable as doing the
check from Guix, especially since the version of Guile used for the
checking might be different.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

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

[Ludovic Courtès]

Hello!

Christopher Baines <mail@cbaines.net> skribis:

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

[...]

>> 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.

Yeah.

Another option is ‘guix style -S arguments’:
<https://issues.guix.gnu.org/63320>.  Not an immediate fix, but a tool
that would let us move away more quickly from a situation that’s prone
to this kind of error.

[...]

> 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).

Woow, that’s a lot!  Could you send a sample of that list, for one
system type, to get an idea of what’s going on?

Thanks,
Ludo’.

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

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]


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

>> 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).
>
> Woow, that’s a lot!  Could you send a sample of that list, for one
> system type, to get an idea of what’s going on?

I think pretty much all the i586-gnu derivations were broken in this
way, on core-updates but then after the merge to master too. I think
I've "fixed" it now, although I think my change [1] needs some
improvement (I fixed some issues in [2], but I saw an error when cross
building).

1: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=08acdd0765b5f4fbfafa699a823ea7985d4d35a7
2: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=08acdd0765b5f4fbfafa699a823ea7985d4d35a7

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

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

[Ludovic Courtès]

Christopher Baines <mail@cbaines.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>>> 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).
>>
>> Woow, that’s a lot!  Could you send a sample of that list, for one
>> system type, to get an idea of what’s going on?
>
> I think pretty much all the i586-gnu derivations

Oh, I see.  I’m less surprised then.  :-)

> were broken in this way, on core-updates but then after the merge to
> master too. I think I've "fixed" it now, although I think my change
> [1] needs some improvement (I fixed some issues in [2], but I saw an
> error when cross building).

I think we’re good now, right?

Ludo’.

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

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]


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

> Christopher Baines <mail@cbaines.net> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>>>> 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).
>>>
>>> Woow, that’s a lot!  Could you send a sample of that list, for one
>>> system type, to get an idea of what’s going on?
>>
>> I think pretty much all the i586-gnu derivations
>
> Oh, I see.  I’m less surprised then.  :-)
>
>> were broken in this way, on core-updates but then after the merge to
>> master too. I think I've "fixed" it now, although I think my change
>> [1] needs some improvement (I fixed some issues in [2], but I saw an
>> error when cross building).
>
> I think we’re good now, right?

I spotted problems again today. I'm not sure if they're new, or just
things I missed in the last round of fixes.

I'm waiting for the data service to give it's opinion on these changes:

  https://issues.guix.gnu.org/63416

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

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

[Maxim Cournoyer]

Hi Christopher,

Christopher Baines <mail@cbaines.net> writes:

[...]

>>> I think pretty much all the i586-gnu derivations
>>
>> Oh, I see.  I’m less surprised then.  :-)
>>
>>> were broken in this way, on core-updates but then after the merge to
>>> master too. I think I've "fixed" it now, although I think my change
>>> [1] needs some improvement (I fixed some issues in [2], but I saw an
>>> error when cross building).
>>
>> I think we’re good now, right?
>
> I spotted problems again today. I'm not sure if they're new, or just
> things I missed in the last round of fixes.
>
> I'm waiting for the data service to give it's opinion on these changes:
>
>   https://issues.guix.gnu.org/63416

Was the problem resolved?  If so, can we close this issue?

-- 
Thanks,
Maxim

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

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1012 bytes --]


Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Hi Christopher,
>
> Christopher Baines <mail@cbaines.net> writes:
>
> [...]
>
>>>> I think pretty much all the i586-gnu derivations
>>>
>>> Oh, I see.  I’m less surprised then.  :-)
>>>
>>>> were broken in this way, on core-updates but then after the merge to
>>>> master too. I think I've "fixed" it now, although I think my change
>>>> [1] needs some improvement (I fixed some issues in [2], but I saw an
>>>> error when cross building).
>>>
>>> I think we’re good now, right?
>>
>> I spotted problems again today. I'm not sure if they're new, or just
>> things I missed in the last round of fixes.
>>
>> I'm waiting for the data service to give it's opinion on these changes:
>>
>>   https://issues.guix.gnu.org/63416
>
> Was the problem resolved?  If so, can we close this issue?

Nope, but the issue covering this is #62051.

We can close this patch issue though, as I think there were objections
to the approach.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

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

[Maxim Cournoyer]

Hi,

[...]

> Nope, but the issue covering this is #62051.
>
> We can close this patch issue though, as I think there were objections
> to the approach.

Thank you.  By the way, I think '-close' has been obsoleted by '-done'
in Debbugs; not sure what it does differently, if anything.

-- 
Thanks,
Maxim