[PATCH] gexp: Stop generating unreadable builder scripts.

Done

Details

4 participants

Josselin Poiret
Ludovic Courtès
Christopher Baines
Maxim Cournoyer

Owner: unassigned

Submitted by: Christopher Baines

Severity: normal

Christopher Baines wrote on 4 May 2023 13:24

Recipients:(address . guix-patches@gnu.org)

Message-ID:20230504112448.22462-1-mail@cbaines.net

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

Toggle diff (28 lines)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

Ludovic Courtès wrote on 4 May 2023 14:47

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 63263@debbugs.gnu.org)

Message-ID:87zg6kqn50.fsf@gnu.org

Hi,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (11 lines)> 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:

Toggle snippet (6 lines)

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?

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:

Toggle snippet (6 lines)

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.

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

Christopher Baines wrote on 4 May 2023 14:57

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 63263@debbugs.gnu.org)

Message-ID:875y98nryn.fsf@cbaines.net

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

Toggle quote (39 lines)> 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.

Toggle quote (19 lines)> 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.

Josselin Poiret wrote on 4 May 2023 21:14

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

Recipients:(address . 63263@debbugs.gnu.org)

Message-ID:87y1m33o5e.fsf@jpoiret.xyz

Hi Chris and Ludo,

Christopher Baines <mail@cbaines.net> writes:

Toggle quote (9 lines)

> 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

Ludovic Courtès wrote on 5 May 2023 23:45

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

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 63263@debbugs.gnu.org)

Message-ID:87ild6mozb.fsf_-_@gnu.org

Hello!

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (2 lines)

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

[...]

Toggle quote (6 lines)

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

[...]

Toggle quote (5 lines)

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

Christopher Baines wrote on 6 May 2023 09:39

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 63263@debbugs.gnu.org)

Message-ID:87pm7d29cd.fsf@cbaines.net

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

Toggle quote (8 lines)

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

Christopher Baines wrote on 6 May 2023 10:05

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

Recipients:(name . Josselin Poiret)(address . dev@jpoiret.xyz)(address . 63263@debbugs.gnu.org)

Message-ID:87lei12820.fsf@cbaines.net

Josselin Poiret <dev@jpoiret.xyz> writes:

Toggle quote (22 lines)> 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.

Ludovic Courtès wrote on 10 May 2023 17:22

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

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 63263@debbugs.gnu.org)

Message-ID:87a5yc9po9.fsf@gnu.org

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (12 lines)> 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. :-)

Toggle quote (5 lines)

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

Christopher Baines wrote on 10 May 2023 18:02

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 63263@debbugs.gnu.org)

Message-ID:87354418dr.fsf@cbaines.net

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

Toggle quote (23 lines)> 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

Maxim Cournoyer wrote on 1 Sep 2023 16:16

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)

Message-ID:87v8cu2d2o.fsf_-_@gmail.com

Hi Christopher,

Christopher Baines <mail@cbaines.net> writes:

[...]

Toggle quote (18 lines)>>> 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

Christopher Baines wrote on 2 Sep 2023 12:36

Recipients:(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 63263-close@debbugs.gnu.org)

Message-ID:87ledoeu5p.fsf@cbaines.net

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

Toggle quote (26 lines)> 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.

Maxim Cournoyer wrote on 3 Sep 2023 16:54

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 63263-close@debbugs.gnu.org)

Message-ID:87bkejux2f.fsf@gmail.com

Hi,

[...]

Toggle quote (5 lines)

> 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

Your comment

This issue is archived.

To comment on this conversation send an email to 63263@debbugs.gnu.org

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date