[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#34807] [PATCH 1/2] Add (guix lzlib).
From: |
Ludovic Courtès |
Subject: |
[bug#34807] [PATCH 1/2] Add (guix lzlib). |
Date: |
Fri, 22 Mar 2019 22:35:02 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
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’.