GNU bug report logs - #34807
[PATCH 1/2] Add (guix lzlib).

Previous Next

Full log

View this message in rfc822 format

From: Ludovic Courtès <ludo <at> gnu.org>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: 34807 <at> debbugs.gnu.org
Subject: [bug#34807] [PATCH 1/2] Add (guix lzlib).
Date: Fri, 22 Mar 2019 22:35:02 +0100

Hello,

Pierre Neidhardt <mail <at> ambrevar.xyz> 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’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.