[bug#34807] [PATCH 1/2] Add (guix lzlib).

[Ludovic Courtès]

Hello,

Pierre Neidhardt <address@hidden> skribis:

> * guix/lzlib.scm, tests/lzlib.scm: New files.
> * Makefile.am (MODULES): Add guix/lzlib.scm.
> (SCM_TESTS): Add tests/lzlib.scm.
> * m4/guix.m4 (GUIX_LIBLZ_LIBDIR): New macro.
> * configure.ac (LIBLZ_LIBDIR): Use it.  Define and substitute
> 'LIBLZ'.
> * guix/config.scm.in (%liblz): New variable.

This looks really nice!

Please update ‘make-config.scm’ in (guix self) so that it defines
‘%liblz’ as well (setting it to #f for now).

> +(define %liblz
> +  ;; TODO: Set this dynamically.
> +  "/gnu/store/8db7vivi8p9mpkbphb8xy8gh2bkwc4iz-lzlib-1.11/lib/liblz")

You can already put "@LIBLZ@" here.

> +(define %lzlib
> +  ;; File name of lzlib's shared library.  When updating via 'guix pull',
> +  ;; '%liblz' might be undefined so protect against it.

Updating ‘make-config.scm’ will fix it.

> +(define %error-number-ok
> +  ;; TODO: How do we get the values of a C enum?

See the thread on guix-devel.

> +(define lz-compress-open
> +  (let ((proc (lzlib-procedure '* "LZ_compress_open" (list int int uint64))))
> +    ;; TODO: member-size default is INT64_MAX.  Is there a better way to do 
> this with Guile?
> +    (lambda* (dictionary-size match-length-limit #:optional (member-size 
> #x7FFFFFFFFFFFFFFF))

You could write (- (expt 2 63) 1) I guess for clarity, but what you wrote is OK.

Is it also the case on 32-bit platforms?

> +(define lz-compress-finish
> +  (let ((proc (lzlib-procedure int "LZ_compress_finish" '(*))))
> +    (lambda (encoder)
> +      "Use this function to tell that all the data for this member have
> +already been written (with the `lz-compress-write' function).  It is safe to
> +call `lz-compress-finish' as many times as needed.  After all the produced
> +compressed data have been read with `lz-compress-read' and
> +`lz-compress-member-finished?' returns #t, a new member can be started with
> +'lz-compress-restart-member'."

For docstrings, the convention in GNU and Guix is to use the imperative
tense and to explicitly refer to the arguments there, like:

  "Tell ENCODER that all the data for this member have alrady been
  written. …"

(Same for the other docstrings that start with “Use this function.”)

> +(define* (lzread! decoder file-port bv
> +                 #:optional (start 0) (count (bytevector-length bv)))
> +  "Read up to COUNT bytes from FILE-PORT into BV at offset START.  Return the
> +number of uncompressed bytes actually read; it is zero if COUNT is zero or if
> +the end-of-stream has been reached."
> +  (let* ((written 0)
> +         (read 0)
> +         (chunk (* 64 1024))
> +         (file-bv (get-bytevector-n file-port count)))
> +    (if (eof-object? file-bv)
> +        0
> +        (begin
> +          (while (and (< 0 (lz-decompress-write-size decoder))
> +                      (< written (bytevector-length file-bv)))
> +            (set! written (lz-decompress-write decoder file-bv written (- 
> (bytevector-length file-bv) written))))
> +          ;; TODO: When should we call `lz-decompress-finish'?
> +          ;; (lz-decompress-finish decoder)
> +          ;; TODO: Loop?
> +          (set! read (lz-decompress-read decoder bv start
> +                                         (- (bytevector-length bv) start)))

It’s worth figuring out.  :-)
Are there examples or the code of the actual ‘lzip’ command that could help?

> +dnl GUIX_LIBLZ_LIBDIR VAR
> +dnl
> +dnl Attempt to determine liblz's LIBDIR; store the result in VAR.
> +AC_DEFUN([GUIX_LIBLZ_LIBDIR], [
> +  AC_REQUIRE([PKG_PROG_PKG_CONFIG])
> +  AC_CACHE_CHECK([lzlib's library directory],
> +    [guix_cv_liblz_libdir],
> +    dnl TODO: This fails because lzlib has no pkg-config.
> +    [guix_cv_liblz_libdir="`$PKG_CONFIG lzlib --variable=libdir 2> 
> /dev/null`"])
> +  $1="$guix_cv_liblz_libdir"
> +])

I think you could do something like this in the body of ‘AC_CACHE_CHECK’
(untested):

  old_LIBS="$LIBS"
  LIBS="-llz"
  AC_LINK_IFELSE([LZ_decompress_open();],
    [guix_cv_libz_libdir="`ldd conftest$EXEEXT | grep liblz | sed '-es/.*=> 
\(.*\) .*$/\1/g'`"])
  LIBS="$old_LIBS"

Regarding testing, it’s easy to get this sort of binding subtly wrong
IME.  :-)  I’d suggest looking at things like:

  1. Passing short input bytevectors, large input bytevectors, and input
     that’s equal to liblz’s internal buffer size or off by one.

  2. File descriptors: strace guile while doing
     ‘call-with-lzip-input-port’ and ‘call-with-lzip-output-port’ and
     make sure that file descriptors are not left open and are not
     closed prematurely either.  (This is particularly important for
     long-running processes like ‘guix publish’.)

But overall, modulo the small issues above, it looks pretty much ready
to me.

Thank you!

Ludo’.