[bug#61255] (%guile-for-build) default in ‘computed-file’

[Maxim Cournoyer]

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

-- 
Thanks,
Maxim