GNU bug report logs - #63263
[PATCH] gexp: Stop generating unreadable builder scripts.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 63263 in the body.
You can then email your comments to 63263 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: guix-patches <at> gnu.org
Subject: [PATCH] gexp: Stop generating unreadable builder scripts.
Date: Thu,  4 May 2023 12:24:48 +0100

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

Message #8 received at 63263 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 63263 <at> debbugs.gnu.org
Subject: Re: bug#63263: [PATCH] gexp: Stop generating unreadable builder
 scripts.
Date: Thu, 04 May 2023 14:47:23 +0200

Hi,

Christopher Baines <mail <at> 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.)

Message #11 received at 63263 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 63263 <at> debbugs.gnu.org
Subject: Re: bug#63263: [PATCH] gexp: Stop generating unreadable builder
 scripts.
Date: Thu, 04 May 2023 13:57:38 +0100

[Message part 1 (text/plain, inline)]

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

> Hi,
>
> Christopher Baines <mail <at> 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 (application/pgp-signature, inline)]

Message #14 received at 63263 <at> debbugs.gnu.org (full text, mbox):

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: Christopher Baines <mail <at> cbaines.net>, Ludovic Courtès
 <ludo <at> gnu.org>
Cc: 63263 <at> debbugs.gnu.org
Subject: Re: [bug#63263] [PATCH] gexp: Stop generating unreadable builder
 scripts.
Date: Thu, 04 May 2023 21:14:05 +0200

[Message part 1 (text/plain, inline)]

Hi Chris and Ludo,

Christopher Baines <mail <at> 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

[signature.asc (application/pgp-signature, inline)]

Message #17 received at 63263 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 63263 <at> debbugs.gnu.org
Subject: Re: bug#63263: [PATCH] gexp: Stop generating unreadable builder
 scripts.
Date: Fri, 05 May 2023 23:45:44 +0200

Hello!

Christopher Baines <mail <at> cbaines.net> skribis:

> Ludovic Courtès <ludo <at> 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’.

Message #20 received at 63263 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 63263 <at> debbugs.gnu.org
Subject: Re: bug#63263: [PATCH] gexp: Stop generating unreadable builder
 scripts.
Date: Sat, 06 May 2023 08:39:21 +0100

[Message part 1 (text/plain, inline)]

Ludovic Courtès <ludo <at> 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

[signature.asc (application/pgp-signature, inline)]

Message #23 received at 63263 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Josselin Poiret <dev <at> jpoiret.xyz>
Cc: 63263 <at> debbugs.gnu.org
Subject: Re: [bug#63263] [PATCH] gexp: Stop generating unreadable builder
 scripts.
Date: Sat, 06 May 2023 09:05:43 +0100

[Message part 1 (text/plain, inline)]

Josselin Poiret <dev <at> jpoiret.xyz> writes:

> Christopher Baines <mail <at> 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.

[signature.asc (application/pgp-signature, inline)]

Message #26 received at 63263 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 63263 <at> debbugs.gnu.org
Subject: Re: bug#63263: [PATCH] gexp: Stop generating unreadable builder
 scripts.
Date: Wed, 10 May 2023 17:22:46 +0200

Christopher Baines <mail <at> cbaines.net> skribis:

> Ludovic Courtès <ludo <at> 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’.

Message #29 received at 63263 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 63263 <at> debbugs.gnu.org
Subject: Re: bug#63263: [PATCH] gexp: Stop generating unreadable builder
 scripts.
Date: Wed, 10 May 2023 17:02:22 +0100

[Message part 1 (text/plain, inline)]

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

> Christopher Baines <mail <at> cbaines.net> skribis:
>
>> Ludovic Courtès <ludo <at> 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

[signature.asc (application/pgp-signature, inline)]

Message #32 received at 63263 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Christopher Baines <mail <at> cbaines.net>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 63263 <at> debbugs.gnu.org
Subject: Re: bug#63263: [PATCH] gexp: Stop generating unreadable builder
 scripts.
Date: Fri, 01 Sep 2023 10:16:47 -0400

Hi Christopher,

Christopher Baines <mail <at> 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

Message #37 received at 63263-close <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 63263-close <at> debbugs.gnu.org
Subject: Re: bug#63263: [PATCH] gexp: Stop generating unreadable builder
 scripts.
Date: Sat, 02 Sep 2023 11:36:43 +0100

[Message part 1 (text/plain, inline)]

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

> Hi Christopher,
>
> Christopher Baines <mail <at> 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.

[signature.asc (application/pgp-signature, inline)]

Message #40 received at 63263-close <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 63263-close <at> debbugs.gnu.org
Subject: Re: bug#63263: [PATCH] gexp: Stop generating unreadable builder
 scripts.
Date: Sun, 03 Sep 2023 10:54:16 -0400

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.