[bug#61949] [PATCH] pack: Move common build code to (guix build pack).

[Ludovic Courtès]

Hi,

Maxim Cournoyer <maxim.cournoyer@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@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’.