| Message ID | 20230504112448.22462-1-mail@cbaines.net |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#63263] gexp: Stop generating unreadable builder scripts. | expand |
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.)
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.
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,
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’.
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
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.
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’.
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
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?
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.
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.
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