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

[Maxim Cournoyer]

Hi again!

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

> 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?  :-)

Thanks!  Yes, Guix-baked RPMs to the packaging pipeline of Jami was the
motivator; in theory maintaining just "One Way" of packaging things
(Guix) should now allow covering all the systems that Jami currently
targets (and more).

> Overall it looks great to me.

Great!

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

Done, although I'm weary of forgetting to update the commit (I guess
make check-news would catch this though).

> Some comments follow:
>
>> +@cindex Debian, build a .deb package with guix pack
>
> @file{.deb} and @command{guix pack}

I thought cindex text shouldn't be decorated, no?

>> +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?

Technically, the above can run on your Guix System if you 'mkdir
/var/lib/rpm && chown $USER /var/lib/rpm' :-).  That's what I used while
developing.  But I've separated the box, as the common and recommended
use case is to install these on non-Guix systems.

>> +@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.

Indeed.  If you try to use rpm's --relocate without having passed -R,
it'll fail and tell you the package is not relocatable.

>> +;;; 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.

Thanks.  I didn't know that, or the reason it was this way.

>> +(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.

OK.  I pondered about the API, but in the end it seems more malleable to
keep everything in an list "intermediate representation", as I could
stitch it together at a later point and more easily inspect things in
tests.

>> +(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.

We can choose the algorithm, but MD5 is still the default in the latest
RPM version.  These are intended to detect simple data corruption.

>> +            (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:

Oh, OK!  I hadn't noticed, adjusted.

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

Thanks for taking the time to review this!  The changes implemented will
appear in v2.

--
Maxim