Package: guix-patches;
Reported by: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Date: Sat, 4 Mar 2023 03:16:02 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
Message #11 received at 61949 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ludovic Courtès <ludo <at> gnu.org> Cc: 61949 <at> debbugs.gnu.org Subject: Re: [bug#61949] [PATCH] pack: Move common build code to (guix build pack). Date: Mon, 06 Mar 2023 14:13:11 -0500
Hi Ludovic,
Ludovic Courtès <ludo <at> gnu.org> writes:
> Hi,
>
> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> The rationale is to reduce the number of derivations built per pack to ideally
>> one, to minimize storage requirements. The number of derivations had gone up
>> with 68380db4 ("pack: Extract populate-profile-root from
>> self-contained-tarball/builder.") as a side effect to improving code reuse.
>>
>> * guix/scripts/pack.scm (guix): Add commentary comment.
>> (populate-profile-root, self-contained-tarball/builder): Extract to...
>> * guix/build/pack.scm (populate-profile-root!): ... this, and...
>> (build-self-contained-tarball): ... that, adjusting for use on the build side.
>> (assert-utf8-locale): New procedure.
>> (self-contained-tarball, debian-archive, rpm-archive): Adjust accordingly.
>
> Thanks for working on it!
> [...]
>
>> +;;; Copyright © 2021, 2023 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
>
> This may be inaccurate given that some of the code here predates this
> file.
>
>> ;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>.
>>
>> +;;; Commentary:
>> +
>> +;;; This module contains build-side common procedures used by the host-side
>> +;;; (guix scripts pack) module, mostly to allow for code reuse. Due to making
>> +;;; use of the (guix build store-copy) module, it transitively requires the
>> +;;; sqlite and gcrypt extensions to be available.
>> +
>> +;;; Code:
>> +
>> (define-module (guix build pack)
>
> Commentary/code should come after ‘define-module’.
Eh. I remembered getting it wrong last time, and tried finding the
information in the Guile Reference manual; I ended up looking at the
"module/scripts/display-commentary.scm" source of the Guile tree, which
has:
--8<---------------cut here---------------start------------->8---
[...]
;;; Commentary:
;; Usage: display-commentary REF1 REF2 ...
;;
;; Display Commentary section from REF1, REF2 and so on.
;; Each REF may be a filename or module name (list of symbols).
;; In the latter case, a filename is computed by searching `%load-path'.
;;; Code:
(define-module (scripts display-commentary)
:use-module (ice-9 documentation)
:export (display-commentary))
--8<---------------cut here---------------end--------------->8---
Is this wrong? It seems the module implementing the functionality
should have gotten that right, ha! Fixed.
>> +(define (assert-utf8-locale)
>> + "Verify the current process is using the en_US.utf8 locale."
>> + (unless (string=? "unset for tests" (getenv "GUIX_LOCPATH"))
>> + (unless (false-if-exception (setlocale LC_ALL "en_US.utf8"))
>> + (error "environment not configured for en_US.utf8 locale"))))
>> +
>> +(define* (populate-profile-root! profile
>> + #:key (profile-name "guix-profile")
>> + localstatedir?
>> + store-database
>> + deduplicate?
>> + (symlinks '()))
>
> Please leave out the bang from the name. The convention in Scheme is to
> suffix a name with bang when it modifies the object(s) it’s given;
> that’s not the case here (see also ‘mkdir’, ‘open-output-file’, etc.).
I see. I wasn't sure, thanks. Fixed.
>> + "Populate the root profile directory with SYMLINKS and a Guix database, when
>> +LOCALSTATEDIR? is set, and a pre-computed STORE-DATABASE is provided. The
>> +directory is created as \"root\" in the current working directory. When
>> +DEDUPLICATE? is true, deduplicate the store items, which relies on hard
>> +links. It needs to run in an environment where "
>> + (when localstatedir?
>> + (unless store-database
>> + (error "missing STORE-DATABASE argument")))
>> +
>> + (define symlink->directives
>
> Please move the ‘when’ expression after all defines so that this code
> can be interpreted by Guile 2.0, which in turn will allow us to run
> tests on ‘guile-bootstrap’.
Done, but there were more complications to get the correct Guile running
(because of the new gcrypt extension dependency introduced with the move
of 'populate-profile-root' to inside the build module).
>> +(define* (build-self-contained-tarball profile
>> + tarball-file-name
>> + #:key (profile-name "guix-profile")
>> + target
>> + localstatedir?
>> + store-database
>> + deduplicate?
>> + symlinks
>> + compressor-command
>> + archiver)
>> + "Create a self-contained tarball TARBALL-FILE-NAME from PROFILE, optionally
>> +compressing it with COMPRESSOR-COMMAND, the complete command-line string to
>> +use for the compressor."
>> + (assert-utf8-locale)
>> +
>> + (populate-profile-root! profile
>> + #:profile-name profile-name
>> + #:localstatedir? localstatedir?
>> + #:store-database store-database
>> + #:deduplicate? deduplicate?
>> + #:symlinks symlinks)
>> +
>> + (define tar (string-append archiver "/bin/tar"))
>
> Likewise, move defines before statements.
Done.
> Also, I would just assume “tar” is in $PATH. That’s the assumption
> generally made for things that need to shell out to various commands,
> such as (gnu build file-systems), (guix docker), etc.
Done. I also dropped the extraneous #:target argument of the
'build-self-contained-tarball' procedure.
>> ;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>.
>>
>> +;;; Commentary:
>> +
>> +;;; This module implements the 'guix pack' command and the various supported
>> +;;; formats. Where feasible, the builders of the packs should be implemented
>> +;;; as single derivations to minimize storage requirements.
>> +
>> +;;; Code:
>
> Likewise needs to be moved down. :-)
Done.
>> -(test-assertm "self-contained-tarball" %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))
>
> [...]
>
>> ;; The following test needs guile-sqlite3, libgcrypt, etc. as a consequence of
>> ;; commit c45477d2a1a651485feede20fe0f3d15aec48b39 and related changes. Thus,
>> ;; run it on the user's store, if it's available, on the grounds that these
>> ;; dependencies may be already there, or we can get substitutes or build them
>> ;; quite inexpensively; see <https://bugs.gnu.org/32184>.
>> -
>> (with-external-store store
>> + (unless store (test-skip 1))
>> + (test-assertm "self-contained-tarball" store
>
> We should avoid moving this tests here. The goal is to keep as many
> tests as possible under the “normal mode” (outside
> ‘with-external-store’) because they are exercised more frequently.
I tried avoiding it, but I think it's because of the new gcrypt
'with-extensions' requirement that is now needed for the
populate-profile-root that runs on the build side, as explained above.
It would attempt to build guile-default and others, like the earlier
problem we've had.
> I went to great lengths to make it possible here, so we should strive to
> preserve that property.
I also appreciate the value of being able to run things without a true
store/daemon.
> (Note that I haven’t tried running the code and tests yet.)
>
> Could you send a v2?
It will follow shortly.
By the way, any clue why this happens?
--8<---------------cut here---------------start------------->8---
$ make check TESTS=tests/pack.sh
[...]
PASS: tests/pack.scm
--8<---------------cut here---------------end--------------->8---
I'd have expected PASS: tests/pack.sh
Thanks!
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.