From: "Ludovic Courtès" <ludo@gnu.org>
To: Pierre Neidhardt <mail@ambrevar.xyz>
Cc: 34807@debbugs.gnu.org
Subject: [bug#34807] [PATCH 1/2] Add (guix lzlib).
Date: Fri, 22 Mar 2019 22:35:02 +0100 [thread overview]
Message-ID: <8736ne3855.fsf@gnu.org> (raw)
In-Reply-To: <20190310180209.11578-1-mail@ambrevar.xyz> (Pierre Neidhardt's message of "Sun, 10 Mar 2019 19:02:09 +0100")
Hello,
Pierre Neidhardt <mail@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’.
next prev parent reply other threads:[~2019-03-22 21:41 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-10 18:02 [bug#34807] [PATCH 1/2] Add (guix lzlib) Pierre Neidhardt
2019-03-10 18:09 ` [bug#34807] [PATCH 2/2] dir-locals.el: Add 'call-with-lzip-input-port' and 'call-with-lzip-output-port' keywords Pierre Neidhardt
2019-03-22 21:35 ` Ludovic Courtès [this message]
2019-05-01 16:46 ` [bug#34807] [PATCH 1/2] Add (guix lzlib) Pierre Neidhardt
2019-05-02 9:16 ` Pierre Neidhardt
2019-05-04 9:11 ` Ludovic Courtès
2019-05-04 10:23 ` Pierre Neidhardt
2019-05-04 21:09 ` Ludovic Courtès
2019-05-04 21:39 ` Pierre Neidhardt
2019-05-06 21:18 ` Ludovic Courtès
2019-05-06 23:28 ` Tobias Geerinckx-Rice
2019-05-07 7:02 ` Pierre Neidhardt
2019-05-07 15:44 ` bug#34807: " Ludovic Courtès
2019-05-07 15:51 ` [bug#34807] " Pierre Neidhardt
2019-05-07 8:19 ` Ludovic Courtès
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8736ne3855.fsf@gnu.org \
--to=ludo@gnu.org \
--cc=34807@debbugs.gnu.org \
--cc=mail@ambrevar.xyz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.