| Message ID | 20230203221409.15886-3-maxim.cournoyer@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hello! Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > * guix/gexp.scm (computed-file): Set the default value of the #:guile argument > to that of the %guile-for-build parameter. [...] > (define* (computed-file name gexp > - #:key guile (local-build? #t) (options '())) > + #:key (guile (%guile-for-build)) > + (local-build? #t) (options '())) I think that would lead ‘computed-file’ to pick (%guile-for-build) at the wrong time (time of call instead of time of lowering). Commit ab25eb7caaf5571cc9f8d6397a1eae127d7e29d1 made it #f such that ‘gexp->derivation’ gets to resolve it at the “right” time. Does that make sense? But perhaps this approach isn’t suitable in the use case you’re looking at? HTH, Ludo’.
Hi Luvodic, Ludovic Courtès <ludo@gnu.org> writes: > Hello! > > Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > >> * guix/gexp.scm (computed-file): Set the default value of the #:guile argument >> to that of the %guile-for-build parameter. > > [...] > >> (define* (computed-file name gexp >> - #:key guile (local-build? #t) (options '())) >> + #:key (guile (%guile-for-build)) >> + (local-build? #t) (options '())) > > I think that would lead ‘computed-file’ to pick (%guile-for-build) at > the wrong time (time of call instead of time of lowering). > > Commit ab25eb7caaf5571cc9f8d6397a1eae127d7e29d1 made it #f such that > ‘gexp->derivation’ gets to resolve it at the “right” time. I see! I think you are right. Would making the change in the associated gexp compiler do the right thing? Currently it ignores the %guile-for-build fluid as set in the tests/pack.scm test suite for example. Something like this: --8<---------------cut here---------------start------------->8--- modified guix/gexp.scm @@ -584,7 +584,7 @@ (define-record-type <computed-file> (options computed-file-options)) ;list of arguments (define* (computed-file name gexp - #:key (guile (%guile-for-build)) + #:key guile (local-build? #t) (options '())) "Return an object representing the store item NAME, a file or directory computed by GEXP. When LOCAL-BUILD? is #t (the default), it ensures the @@ -601,7 +601,8 @@ (define-gexp-compiler (computed-file-compiler (file <computed-file>) ;; gexp. (match file (($ <computed-file> name gexp guile options) - (mlet %store-monad ((guile (lower-object (or guile (default-guile)) + (mlet %store-monad ((guile (lower-object (or guile (%guile-for-build) + (default-guile)) system #:target #f))) (apply gexp->derivation name gexp #:guile-for-build guile #:system system #:target target options))))) --8<---------------cut here---------------end--------------->8--- I've verified that 'make check TESTS=tests/pack.scm' is still happy (without such patch, with patch 3/5 applied, the "self-contained-tarball" would try to build a non-bootstrap guile and timeout (on my old machine). Thanks and enjoy FOSDEM!
Hi Maxim, Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > Ludovic Courtès <ludo@gnu.org> writes: > >> Hello! >> >> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: >> >>> * guix/gexp.scm (computed-file): Set the default value of the #:guile argument >>> to that of the %guile-for-build parameter. >> >> [...] >> >>> (define* (computed-file name gexp >>> - #:key guile (local-build? #t) (options '())) >>> + #:key (guile (%guile-for-build)) >>> + (local-build? #t) (options '())) >> >> I think that would lead ‘computed-file’ to pick (%guile-for-build) at >> the wrong time (time of call instead of time of lowering). >> >> Commit ab25eb7caaf5571cc9f8d6397a1eae127d7e29d1 made it #f such that >> ‘gexp->derivation’ gets to resolve it at the “right” time. > > I see! I think you are right. Would making the change in the > associated gexp compiler do the right thing? Currently it ignores the > %guile-for-build fluid as set in the tests/pack.scm test suite for > example. Something like this: I don’t fully understand the context. My preference would go to doing like the ‘computed-file’ tests in ‘tests/gexp.scm’, where we explicitly pass #:guile %bootstrap-guile. That said, it seems like patch #5 in this series doesn’t actually use ‘computed-file’ in ‘tests/pack.scm’, does it? Thanks, Ludo’, slowly catching up post-FOSDEM!
Hi Ludovic! Ludovic Courtès <ludo@gnu.org> writes: > Hi Maxim, > > Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > >> Ludovic Courtès <ludo@gnu.org> writes: >> >>> Hello! >>> >>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: >>> >>>> * guix/gexp.scm (computed-file): Set the default value of the #:guile argument >>>> to that of the %guile-for-build parameter. >>> >>> [...] >>> >>>> (define* (computed-file name gexp >>>> - #:key guile (local-build? #t) (options '())) >>>> + #:key (guile (%guile-for-build)) >>>> + (local-build? #t) (options '())) >>> >>> I think that would lead ‘computed-file’ to pick (%guile-for-build) at >>> the wrong time (time of call instead of time of lowering). >>> >>> Commit ab25eb7caaf5571cc9f8d6397a1eae127d7e29d1 made it #f such that >>> ‘gexp->derivation’ gets to resolve it at the “right” time. >> >> I see! I think you are right. Would making the change in the >> associated gexp compiler do the right thing? Currently it ignores the >> %guile-for-build fluid as set in the tests/pack.scm test suite for >> example. Something like this: > > I don’t fully understand the context. My preference would go to doing > like the ‘computed-file’ tests in ‘tests/gexp.scm’, where we explicitly > pass #:guile %bootstrap-guile. With the refactoring done in patch 3/5 ("pack: Extract populate-profile-root from self-contained-tarball/builder."), a computed-file is used in the factorized building block 'populate-profile-root'. Without this patch, the tests making use of it would attempt to build Guile & friends in the test store. > That said, it seems like patch #5 in this series doesn’t actually use > ‘computed-file’ in ‘tests/pack.scm’, does it? It does, indirectly. I hope that helps!
Hello! Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > Hi Ludovic! > > Ludovic Courtès <ludo@gnu.org> writes: > >> Hi Maxim, >> >> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: >> >>> Ludovic Courtès <ludo@gnu.org> writes: >>> >>>> Hello! >>>> >>>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: >>>> >>>>> * guix/gexp.scm (computed-file): Set the default value of the #:guile argument >>>>> to that of the %guile-for-build parameter. >>>> >>>> [...] >>>> >>>>> (define* (computed-file name gexp >>>>> - #:key guile (local-build? #t) (options '())) >>>>> + #:key (guile (%guile-for-build)) >>>>> + (local-build? #t) (options '())) >>>> >>>> I think that would lead ‘computed-file’ to pick (%guile-for-build) at >>>> the wrong time (time of call instead of time of lowering). >>>> >>>> Commit ab25eb7caaf5571cc9f8d6397a1eae127d7e29d1 made it #f such that >>>> ‘gexp->derivation’ gets to resolve it at the “right” time. >>> >>> I see! I think you are right. Would making the change in the >>> associated gexp compiler do the right thing? Currently it ignores the >>> %guile-for-build fluid as set in the tests/pack.scm test suite for >>> example. Something like this: >> >> I don’t fully understand the context. My preference would go to doing >> like the ‘computed-file’ tests in ‘tests/gexp.scm’, where we explicitly >> pass #:guile %bootstrap-guile. > > With the refactoring done in patch 3/5 ("pack: Extract > populate-profile-root from self-contained-tarball/builder."), a > computed-file is used in the factorized building block > 'populate-profile-root'. Without this patch, the tests making use of it > would attempt to build Guile & friends in the test store. > >> That said, it seems like patch #5 in this series doesn’t actually use >> ‘computed-file’ in ‘tests/pack.scm’, does it? > > It does, indirectly. > > I hope that helps! I’m really not sure what the impact of 68775338a510f84e63657ab09242d79e726fa457 is, nor whether it was the only solution to the problem. One thing that probably happens is that (default-guile) is now never used for <computed-file>, contrary to what was happening before. The spirit is that (default-guile) would be used as the default for all the declarative file-like objects; gexp compilers refer to (default-guile), not (%guile-for-build). Importantly, (%guile-for-build) is a derivation, possibly built for another system, whereas (default-guile) is a package, which allows ‘lower-object’ to return the derivation for the right system type. Overall, I think this change should be reverted but of course, we should find a solution to the problem you hit in the first place. I hope this makes sense to you. Ludo’.
Hi Ludovic, Ludovic Courtès <ludo@gnu.org> writes: > Hello! > > Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > >> Hi Ludovic! >> >> Ludovic Courtès <ludo@gnu.org> writes: >> >>> Hi Maxim, >>> >>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: >>> >>>> Ludovic Courtès <ludo@gnu.org> writes: >>>> >>>>> Hello! >>>>> >>>>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: >>>>> >>>>>> * guix/gexp.scm (computed-file): Set the default value of the #:guile argument >>>>>> to that of the %guile-for-build parameter. >>>>> >>>>> [...] >>>>> >>>>>> (define* (computed-file name gexp >>>>>> - #:key guile (local-build? #t) (options '())) >>>>>> + #:key (guile (%guile-for-build)) >>>>>> + (local-build? #t) (options '())) >>>>> >>>>> I think that would lead ‘computed-file’ to pick (%guile-for-build) at >>>>> the wrong time (time of call instead of time of lowering). >>>>> >>>>> Commit ab25eb7caaf5571cc9f8d6397a1eae127d7e29d1 made it #f such that >>>>> ‘gexp->derivation’ gets to resolve it at the “right” time. >>>> >>>> I see! I think you are right. Would making the change in the >>>> associated gexp compiler do the right thing? Currently it ignores the >>>> %guile-for-build fluid as set in the tests/pack.scm test suite for >>>> example. Something like this: >>> >>> I don’t fully understand the context. My preference would go to doing >>> like the ‘computed-file’ tests in ‘tests/gexp.scm’, where we explicitly >>> pass #:guile %bootstrap-guile. >> >> With the refactoring done in patch 3/5 ("pack: Extract >> populate-profile-root from self-contained-tarball/builder."), a >> computed-file is used in the factorized building block >> 'populate-profile-root'. Without this patch, the tests making use of it >> would attempt to build Guile & friends in the test store. >> >>> That said, it seems like patch #5 in this series doesn’t actually use >>> ‘computed-file’ in ‘tests/pack.scm’, does it? >> >> It does, indirectly. >> >> I hope that helps! > > I’m really not sure what the impact of > 68775338a510f84e63657ab09242d79e726fa457 is, nor whether it was the only > solution to the problem. > > One thing that probably happens is that (default-guile) is now never > used for <computed-file>, contrary to what was happening before. The > spirit is that (default-guile) would be used as the default for all the > declarative file-like objects; gexp compilers refer to (default-guile), > not (%guile-for-build). > > Importantly, (%guile-for-build) is a derivation, possibly built for > another system, whereas (default-guile) is a package, which allows > ‘lower-object’ to return the derivation for the right system type. I assumed the purpose of the %guile-for-build fluid was to override the value of the guile used in some conditions, such as during tests (e.g. the '(set-guile-for-build (default-guile))' calls inside the store monad in tests/pack.scm). It's honored for gexp->derivation, but isn't honored for computed-file, which is supposed to be its declarative counterpart. This problem was only exposed when factoring out 'populate-profile-root' as a computed-file object in 68380db4c40a2ee1156349a87254fd7b1f1a52d5 ("pack: Extract populate-profile-root from self-contained-tarball/builder.") > Overall, I think this change should be reverted but of course, we should > find a solution to the problem you hit in the first place. > > I hope this makes sense to you. See the problem it solves below. If we revert this now, we'd have to mark the 'self-contained-tarball' as an expected fail until we find a a better solution. The problem it solves is this: after reverting the change with: > modified guix/gexp.scm > @@ -601,7 +601,7 @@ (define-gexp-compiler (computed-file-compiler (file <computed-file>) > ;; gexp. > (match file > (($ <computed-file> name gexp guile options) > - (mlet %store-monad ((guile (lower-object (or guile (%guile-for-build) > + (mlet %store-monad ((guile (lower-object (or guile ;(%guile-for-build) > (default-guile)) > system #:target #f))) > (apply gexp->derivation name gexp #:guile-for-build guile Running the pack.scm tests: $ make check TESTS=tests/pack.scm Fails with a timeout, because the %guile-for-build is not honored by a computed-file derivation, and it goes on building the non-bootstrap build-side guile, gcc, etc. in the test store (see: pack.log): --8<---------------cut here---------------start------------->8--- gcc-10.3.0/gcc/targhooks.h gcc-10.3.0/gcc/testsuite/ gcc-10.3.0/gcc/testsuite/.gitattributes gcc-10.3.0/gcc/testsuite/ChangeLog gcc-10.3.0/gcc/testsuite/ChangeLog-1993-2007 gcc-10.3.0/gcc/testsuite/ChangeLog-2008 gcc-10.3.0/gcc/testsuite/ChangeLog-2009 gcc-10.3.0/gcc/testsuite/ChangeLog-2010 gcc-10.3.0/gcc/testsuite/ChangeLog-2011 gcc-10.3.0/gcc/testsuite/ChangeLog-2012 gcc-10.3.0/gcc/testsuite/ChangeLog-2013 gcc-10.3.0/gcc/testsuite/ChangeLog-2014 gcc-10.3.0/gcc/testsuite/ChangeLog-2015 gcc-10.3.0/gcc/testsuite/ChangeLog-2016 gcc-10.3.0/gcc/testsuite/ChangeLog-2017 gcc-10.3.0/gcc/testsuite/ChangeLog-2018 building of `/home/maxim/src/guix/test-tmp/store/hp86j4850ajphhs1hyryis5nj93pv66l-gcc-10.3.0.tar.xz.drv' timed out after 300 seconds @ build-failed /home/maxim/src/guix/test-tmp/store/hp86j4850ajphhs1hyryis5nj93pv66l-gcc-10.3.0.tar.xz.drv - timeout killing process 4149 cannot build derivation `/home/maxim/src/guix/test-tmp/store/82yb9zwxdwhmacz36pjrrzzmgjgakavy-gcc-10.3.0.drv': 1 dependencies couldn't be built @ build-started /home/maxim/src/guix/test-tmp/store/8dfjl4594zgb7wi3icw8s9z3rr3pck6x-gcc-4.9.4.tar.xz.drv - x86_64-linux /home/maxim/src/guix/test-tmp/var/log/guix/drvs/8d//fjl4594zgb7wi3icw8s9z3rr3pck6x-gcc-4.9.4.tar.xz.drv.gz 4611 cannot build derivation `/home/maxim/src/guix/test-tmp/store/hcv6vh1gx5fkw62l3nravi1aqhi8cq60-gcc-cross-boot0-10.3.0.drv': 1 dependencies couldn't be built killing process 4611 cannot build derivation `/home/maxim/src/guix/test-tmp/store/1ihb1yadv4dfbqhfcgn1cyvsl8444yaw-guile-3.0.7.drv': 1 dependencies couldn't be built cannot build derivation `/home/maxim/src/guix/test-tmp/store/6g7fhyr1b84b5qg8nwn46hkrg55i8c2q-profile-directory.drv': 1 dependencies couldn't be built cannot build derivation `/home/maxim/src/guix/test-tmp/store/apm8bjvzs1n707lagw0spzr2m2nc0p4v-pack.tar.gz.drv': 1 dependencies couldn't be built cannot build derivation `/home/maxim/src/guix/test-tmp/store/syiq7lmx3v0pkrjp5wqd5kfapqpxpki3-check-tarball.drv': 1 dependencies couldn't be built test-name: self-contained-tarball location: /home/maxim/src/guix/tests/pack.scm:80 source: + (test-assert + "self-contained-tarball" + (let ((guile (package-derivation %store %bootstrap-guile))) + (run-with-store + %store + (mlet* %store-monad + ((profile + -> + (profile + (content + (packages->manifest (list %bootstrap-guile))) + (hooks '()) + (locales? #f))) + (tarball + (self-contained-tarball + "pack" + profile + #:symlinks + '(("/bin/Guile" -> "bin/guile")) + #:compressor + %gzip-compressor + #:archiver + %tar-bootstrap)) + (check (gexp->derivation + "check-tarball" + (with-imported-modules + '((guix build utils)) + (gexp (begin + (use-modules + (guix build utils) + (srfi srfi-1)) + (define store + (string-append + "." + (%store-directory) + "/")) + (define (canonical? file) + (let ((st (lstat file))) + (or (not (string-prefix? store file)) + (eq? 'symlink (stat:type st)) + (and (= 1 (stat:mtime st)) + (zero? (logand + 146 + (stat:mode st))))))) + (define bin + (string-append + "." + (ungexp profile) + "/bin")) + (setenv + "PATH" + (string-append + (ungexp %tar-bootstrap) + "/bin")) + (system* "tar" "xvf" (ungexp tarball)) + (mkdir (ungexp output)) + (exit (and (file-exists? + (string-append bin "/guile")) + (file-exists? store) + (every canonical? + (find-files + "." + (const #t) + #:directories? + #t)) + (string=? + (string-append + (ungexp %bootstrap-guile) + "/bin") + (readlink bin)) + (string=? + (string-append + ".." + (ungexp profile) + "/bin/guile") + (readlink "bin/Guile")))))))))) + (built-derivations (list check))) + #:guile-for-build + guile))) actual-value: #f actual-error: + (%exception + #<&store-protocol-error message: "build of `/home/maxim/src/guix/test-tmp/store/syiq7lmx3v0pkrjp5wqd5kfapqpxpki3-check-tarball.drv' failed" status: 101>) result: FAIL --8<---------------cut here---------------end--------------->8---
Hi Maxim, Ludovic Courtès <ludo@gnu.org> skribis: > I’m really not sure what the impact of > 68775338a510f84e63657ab09242d79e726fa457 is, nor whether it was the only > solution to the problem. > > One thing that probably happens is that (default-guile) is now never > used for <computed-file>, contrary to what was happening before. The > spirit is that (default-guile) would be used as the default for all the > declarative file-like objects; gexp compilers refer to (default-guile), > not (%guile-for-build). > > Importantly, (%guile-for-build) is a derivation, possibly built for > another system, whereas (default-guile) is a package, which allows > ‘lower-object’ to return the derivation for the right system type. Commit 68775338a510f84e63657ab09242d79e726fa457 turned out to have unintended side effects: https://issues.guix.gnu.org/61841 I fixed it with: a516a0ba93 gexp: computed-file: Do not honor %guile-for-build. fee1d08f0d pack: Make sure tests can run without a world rebuild. Please take a look. We should think about how to improve our processes to avoid such issues in the future. I did raise concerns about this very patch late at night during FOSDEM, 24h after submission, and reaffirmed my viewpoint days later. I understand that delaying a nice patch series like this one is unpleasant, but I think those concerns should have been taken into account. Ludo’.
Hi Ludovic, Ludovic Courtès <ludo@gnu.org> writes: > Hi Maxim, > > Ludovic Courtès <ludo@gnu.org> skribis: > >> I’m really not sure what the impact of >> 68775338a510f84e63657ab09242d79e726fa457 is, nor whether it was the only >> solution to the problem. >> >> One thing that probably happens is that (default-guile) is now never >> used for <computed-file>, contrary to what was happening before. The >> spirit is that (default-guile) would be used as the default for all the >> declarative file-like objects; gexp compilers refer to (default-guile), >> not (%guile-for-build). >> >> Importantly, (%guile-for-build) is a derivation, possibly built for >> another system, whereas (default-guile) is a package, which allows >> ‘lower-object’ to return the derivation for the right system type. > > Commit 68775338a510f84e63657ab09242d79e726fa457 turned out to have > unintended side effects: > > https://issues.guix.gnu.org/61841 Ugh. > I fixed it with: > > a516a0ba93 gexp: computed-file: Do not honor %guile-for-build. > fee1d08f0d pack: Make sure tests can run without a world rebuild. > > Please take a look. Thank you. I still think it'd be nicer if computed-file had a means to honor %guile-for-build rather than having to accommodate it specially as you did in fee1d08f0d, so that it'd be symmetrical to gexp->derivation in that regard. Why can't they? > We should think about how to improve our processes to avoid such issues > in the future. I did raise concerns about this very patch late at night > during FOSDEM, 24h after submission, and reaffirmed my viewpoint days > later. I understand that delaying a nice patch series like this one is > unpleasant, but I think those concerns should have been taken into > account. You are right, I should have delayed this submission passed its 2 weeks, to let some extra time to look at alternatives w.r.t. the %guile-for-build patch. Apologies for being too eager!
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > Ludovic Courtès <ludo@gnu.org> writes: [...] >> Commit 68775338a510f84e63657ab09242d79e726fa457 turned out to have >> unintended side effects: >> >> https://issues.guix.gnu.org/61841 > > Ugh. > >> I fixed it with: >> >> a516a0ba93 gexp: computed-file: Do not honor %guile-for-build. >> fee1d08f0d pack: Make sure tests can run without a world rebuild. >> >> Please take a look. > > Thank you. I still think it'd be nicer if computed-file had a means to > honor %guile-for-build rather than having to accommodate it specially as > you did in fee1d08f0d, so that it'd be symmetrical to gexp->derivation > in that regard. Why can't they? Like I wrote, ‘default-guile’ returns a package whereas ‘%guile-for-build’ returns a derivation. The latter is inherently lower-level: it’s used together with the monadic interface or with plain ‘derivation’, when we know which system we’re targeting. The former is higher-level, system-independent; it must be used for <computed-file> and similar forms, which are system-independent. Ludo’.
Hi Ludo, Ludovic Courtès <ludo@gnu.org> writes: > Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > >> Ludovic Courtès <ludo@gnu.org> writes: > > [...] > >>> Commit 68775338a510f84e63657ab09242d79e726fa457 turned out to have >>> unintended side effects: >>> >>> https://issues.guix.gnu.org/61841 >> >> Ugh. >> >>> I fixed it with: >>> >>> a516a0ba93 gexp: computed-file: Do not honor %guile-for-build. >>> fee1d08f0d pack: Make sure tests can run without a world rebuild. >>> >>> Please take a look. >> >> Thank you. I still think it'd be nicer if computed-file had a means to >> honor %guile-for-build rather than having to accommodate it specially as >> you did in fee1d08f0d, so that it'd be symmetrical to gexp->derivation >> in that regard. Why can't they? > > Like I wrote, ‘default-guile’ returns a package whereas > ‘%guile-for-build’ returns a derivation. > > The latter is inherently lower-level: it’s used together with the > monadic interface or with plain ‘derivation’, when we know which system > we’re targeting. The former is higher-level, system-independent; it > must be used for <computed-file> and similar forms, which are > system-independent. I see, it's starting to make sense. I'll sleep on it :-).
diff --git a/guix/gexp.scm b/guix/gexp.scm index 5f92174a2c..bf75d1f8df 100644 --- a/guix/gexp.scm +++ b/guix/gexp.scm @@ -584,7 +584,8 @@ (define-record-type <computed-file> (options computed-file-options)) ;list of arguments (define* (computed-file name gexp - #:key guile (local-build? #t) (options '())) + #:key (guile (%guile-for-build)) + (local-build? #t) (options '())) "Return an object representing the store item NAME, a file or directory computed by GEXP. When LOCAL-BUILD? is #t (the default), it ensures the corresponding derivation is built locally. OPTIONS may be used to pass