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 #8 received at 61949 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> 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 16:47:28 +0100
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’.
> +(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.).
> + "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’.
> +(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.
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.
> ;;; 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. :-)
> -(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 went to great lengths to make it possible here, so we should strive to
preserve that property.
(Note that I haven’t tried running the code and tests yet.)
Could you send a v2?
Thanks,
Ludo’.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.