[bug#61255] [PATCH 0/5] Add support for the RPM format to "guix pack"

[Ludovic Courtès]

Hey!

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> * guix/rpm.scm: New file.
> * guix/scripts/pack.scm (rpm-archive): New procedure.
> (%formats): Register it.
> (show-formats): Add it.
> (guix-pack): Register supported extra-options for the rpm format.
> * tests/pack.scm (rpm-for-tests): New variable.
> ("rpm archive can be installed/uninstalled"): New test.
> * tests/rpm.scm: New test.
> * doc/guix.texi (Invoking guix pack): Document it.

(‘Makefile.am’ changes are missing here.)

Woow, there’s a lot of fun stuff in here!  :-)  Nice work!

Perhaps we’ll soon see Guix-generated RPMs for, say, Jami?  :-)

Overall it looks great to me.

Perhaps you should submit an ‘etc/news.scm’ entry here so that
translators can work on it before it’s eventually pushed (I think that’s
the workflow Julien proposed).

Some comments follow:

> +@cindex Debian, build a .deb package with guix pack

@file{.deb} and @command{guix pack}

> +The RPM format supports relocatable packages via the @option{--prefix}
> +option of the @command{rpm} command, which can be handy to install an
> +RPM package to a specific prefix, making installing multiple
> +Guix-produced RPM packages side by side possible.
> +
> +@example
> +guix pack -f rpm -R -C xz -S /usr/bin/hello=bin/hello hello
> +sudo rpm --install --prefix=/opt /gnu/store/...-hello.rpm
> +@end example

Perhaps use two different @example boxes to distinguish between the Guix
machine that produces the RPM, and the RPM-based system that installs
it?

> +@quotation Note
> +Similarly to Debian packages, two RPM packages with conflicting files
> +cannot be installed simultaneously.  Contrary to Debian packages, RPM
> +supports relocatable packages, so file conflicts can be avoided by
> +installing the RPM packages under different installation prefixes, as
> +shown in the above example.

So for relocatable packages, one really needs ‘guix pack -R’ IIUC.
Interesting.

> +;;; Commentary:
> +;;;
> +;;; This module provides the building blocks required to construct RPM
> +;;; archives.  It is intended to be importable on the build side, so 
> shouldn't
> +;;; depend on (guix diagnostics) or other host-side-only modules.
> +
> +(define-module (guix rpm)

The commentary should be followed by “Code:” and it should come after
the ‘define-module’ form.  That way, (ice-9 documentation) can find it.

> +(define (make-header-index+data entries)
> +  "Return the index and data sections as u8 number lists, via multiple 
> values.
> +An index is composed of four u32 (16 bytes total) quantities, in order: tag,
> +type, offset and count."
> +  (match (fold (match-lambda*
> +                 ((entry (offset . (index . data)))
> +                  (let* ((tag (header-entry-tag entry))
> +                         (tag-number (rpm-tag-number tag))
> +                         (tag-type (rpm-tag-type tag))
> +                         (count (header-entry-count entry))
> +                         (data* (header-entry->data entry))
> +                         (alignment (entry-type->alignement tag-type))
> +                         (aligned-offset (next-aligned-offset offset 
> alignment))
> +                         (padding (make-list (- aligned-offset offset) 0)))
> +                    (cons (+ aligned-offset (length data*))
> +                          (cons (append index
> +                                        (u32-number->u8-list tag-number)
> +                                        (u32-number->u8-list tag-type)
> +                                        (u32-number->u8-list aligned-offset)
> +                                        (u32-number->u8-list count))
> +                                (append data padding data*))))))

I think it would be possible (throughout the code) to avoid building
lists of bytes and instead directly produce bytevectors or, better,
produce procedures that write bytes directly to an output port (with
macros along the lines of ‘define-operation’ in (guix store) or
‘define-pack’ in (guix cpio)).

I don’t think it should be a blocker though, it’s okay to keep it this
way.

> +(define (files->md5-checksums files)
> +  "Return the MD5 checksums (formatted as hexadecimal strings) for FILES."

Does it have to be MD5?  If RPM supports SHA1 or SHA2*, it would be best
to pick one of these; MD5 is okay to detect unintended modifications,
but it’s useless if we care about malicious tampering.

> +            (define name (or (and=> single-entry manifest-entry-name)
> +                             (manifest->friendly-name manifest)))
> +
> +            (define version (or (and=> single-entry manifest-entry-version)
> +                                "0.0.0"))
> +
> +            (define lead (generate-lead (string-append name "-" version)
> +                                        #:target (or #$target %host-type)))
> +
> +            (define payload-digest (bytevector->hex-string
> +                                    (file-sha256 #$payload)))

Nitpick: the convention usually followed is to write the value, when
it’s long enough as is the case here, on the next line, as in:

  (define something
    value-thats-a-little-bit-long)

> +  (unless store (test-skip 1))
> +  (test-assertm "rpm archive can be installed/uninstalled" store

Really cool to have a full-blown test like this.

> +(define-module (test-rpm)
> +  #:use-module (guix rpm)

That too!

Thanks,
Ludo’.