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

[Maxim Cournoyer]

Hi Ludovic,

Ludovic Courtès <ludo@gnu.org> writes:

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

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