Re: 01/01: guix: Simplify and robustify lzread!.

Re: 01/01: guix: Simplify and robustify lzread!.

[Ludovic Courtès]

Hi Pierre!

guix-commits@gnu.org skribis:

> commit ecfc54403e2a1934b4f6e84ddad429b7970091fa
> Author: Pierre Neidhardt <mail@ambrevar.xyz>
> Date:   Tue May 7 18:40:40 2019 +0200
>
>     guix: Simplify and robustify lzread!.
      ^
Should be lzlib.

>     * guix/lzlib.scm (lzread!): Do it.
>     
>     Previously lzread! would fail if COUNT was bigger
>     than (lz-decompress-write-size).  This is possible if a previous call to
>     lzread! didn't empty the LZ_decompress input buffer (e.g. BV was too small to
>     fit all the data).

Could you add a test for this corner case?  It’s corner cases like that
that can spoil the whole experience, so it feels safer to add tests as
soon as we encounter them.  :-)

Thanks!

Ludo’.

Re: 01/01: guix: Simplify and robustify lzread!.

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

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

> Hi Pierre!
>
> guix-commits@gnu.org skribis:
>
>> commit ecfc54403e2a1934b4f6e84ddad429b7970091fa
>> Author: Pierre Neidhardt <mail@ambrevar.xyz>
>> Date:   Tue May 7 18:40:40 2019 +0200
>>
>>     guix: Simplify and robustify lzread!.
>       ^
> Should be lzlib.

Oops!
I must confess that I never really understood the whole logic behind
GNU-style commit message :p

>>     * guix/lzlib.scm (lzread!): Do it.
>>     
>>     Previously lzread! would fail if COUNT was bigger
>>     than (lz-decompress-write-size).  This is possible if a previous call to
>>     lzread! didn't empty the LZ_decompress input buffer (e.g. BV was too small to
>>     fit all the data).
>
> Could you add a test for this corner case?  It’s corner cases like that
> that can spoil the whole experience, so it feels safer to add tests as
> soon as we encounter them.  :-)

I would have, but I don't know how.  The problem is that I don't control
the chunk size passed to the read! callback of the custom port.  That's
all up to Guile's implementation of custom ports.

In practice, it seems that the BV is always big enough so the potential
issue can never arise.  But the new code is better on all aspects.

Cheers!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: 01/01: guix: Simplify and robustify lzread!.

[Ludovic Courtès]

Hi Pierre,

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Hi Pierre!
>>
>> guix-commits@gnu.org skribis:
>>
>>> commit ecfc54403e2a1934b4f6e84ddad429b7970091fa
>>> Author: Pierre Neidhardt <mail@ambrevar.xyz>
>>> Date:   Tue May 7 18:40:40 2019 +0200
>>>
>>>     guix: Simplify and robustify lzread!.
>>       ^
>> Should be lzlib.
>
> Oops!
> I must confess that I never really understood the whole logic behind
> GNU-style commit message :p

It’s really Guix-specific here, but the idea is that the word before
colon indicates the part of the code being modified, to provide a bit of
context.

>> Could you add a test for this corner case?  It’s corner cases like that
>> that can spoil the whole experience, so it feels safer to add tests as
>> soon as we encounter them.  :-)
>
> I would have, but I don't know how.  The problem is that I don't control
> the chunk size passed to the read! callback of the custom port.  That's
> all up to Guile's implementation of custom ports.
>
> In practice, it seems that the BV is always big enough so the potential
> issue can never arise.  But the new code is better on all aspects.

Didn’t you encounter the bug on an actual use case, though?  If you did,
you could just write that use case in the test file.  If not, that’s
okay.

(I’m super cautious here but that’s because I spent countless hours
debugging code that used zlib and libbz2 and lzo in the past.  :-))

Thanks,
Ludo’.

Re: 01/01: guix: Simplify and robustify lzread!.

[Pierre Neidhardt]

[-- Attachment #1: Type: text/plain, Size: 2052 bytes --]


>>>>     guix: Simplify and robustify lzread!.
>>>       ^
>>> Should be lzlib.
>>
>> Oops!
>> I must confess that I never really understood the whole logic behind
>> GNU-style commit message :p
>
> It’s really Guix-specific here, but the idea is that the word before
> colon indicates the part of the code being modified, to provide a bit of
> context.

That makes sense.  Where it makes less sense to me is when we prefix
with "gnu:" for packages.  Why GNU?  Why not "packages:"?

>>> Could you add a test for this corner case?  It’s corner cases like that
>>> that can spoil the whole experience, so it feels safer to add tests as
>>> soon as we encounter them.  :-)
>>
>> I would have, but I don't know how.  The problem is that I don't control
>> the chunk size passed to the read! callback of the custom port.  That's
>> all up to Guile's implementation of custom ports.
>>
>> In practice, it seems that the BV is always big enough so the potential
>> issue can never arise.  But the new code is better on all aspects.
>
> Didn’t you encounter the bug on an actual use case, though?  If you did,
> you could just write that use case in the test file.  If not, that’s
> okay.

No bug on an actual use case.  I kept discussing with Antonio and he
pointed out the potential issue.  The solution occurred to me on a fresh
sunny morning :D

> (I’m super cautious here but that’s because I spent countless hours
> debugging code that used zlib and libbz2 and lzo in the past.  :-))

Yup, tell me about it...  Took me weeks just to figure out basic
(de)compression ;)

But now that I have a better understanding of the library, I'm much more
confident about the robustness and debugability of the bindings.  Once
things start adding up in your head, it's much more straightforward to
see what goes wrong and how to fix it.  (Stating the obvious here, but
that's only true with good libraries, and lzlib is a good one.)

Well, time will tell :)

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: 01/01: guix: Simplify and robustify lzread!.

[Ludovic Courtès]

Heya,

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

>>>>>     guix: Simplify and robustify lzread!.
>>>>       ^
>>>> Should be lzlib.
>>>
>>> Oops!
>>> I must confess that I never really understood the whole logic behind
>>> GNU-style commit message :p
>>
>> It’s really Guix-specific here, but the idea is that the word before
>> colon indicates the part of the code being modified, to provide a bit of
>> context.
>
> That makes sense.  Where it makes less sense to me is when we prefix
> with "gnu:" for packages.  Why GNU?  Why not "packages:"?

Historical reasons.  :-)  Once upon a time that was “distro:”…

> No bug on an actual use case.  I kept discussing with Antonio and he
> pointed out the potential issue.  The solution occurred to me on a fresh
> sunny morning :D

Good.  :-)

>> (I’m super cautious here but that’s because I spent countless hours
>> debugging code that used zlib and libbz2 and lzo in the past.  :-))
>
> Yup, tell me about it...  Took me weeks just to figure out basic
> (de)compression ;)
>
> But now that I have a better understanding of the library, I'm much more
> confident about the robustness and debugability of the bindings.  Once
> things start adding up in your head, it's much more straightforward to
> see what goes wrong and how to fix it.  (Stating the obvious here, but
> that's only true with good libraries, and lzlib is a good one.)

Alright, sounds good!

Thank you,
Ludo’.