[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 1/2] Import upstream zstd-1.3.6

From: Nick Terrell
Subject: Re: [PATCH v4 1/2] Import upstream zstd-1.3.6
Date: Tue, 6 Nov 2018 19:43:04 +0000

> On Nov 6, 2018, at 6:48 AM, Daniel Kiper <address@hidden> wrote:
> On Wed, Oct 31, 2018 at 10:56:16AM -0700, Nick Terrell wrote:
>> Import zstd-1.3.6 from upstream [1]. Only the files need for decompression
>> are imported. Additionally makes zstd a module by adding module.c which
>> contains the license, and updates Makefile.core.def.
>> I used the latest zstd release, which includes patches [2] to build cleanly
>> in GRUB.
>> Upstream zstd commit hash: 4fa456d7f12f8b27bd3b2f5dfd4f46898cb31c24
>> Upstream zstd commit name: Merge pull request #1354 from facebook/dev
>> I've included the script used to import zstd-1.3.6 below.
>> [1]
>> [2]
>> ```
>> #!/bin/sh -e
>> curl -L -O 
>> curl -L -O 
>> sha256sum --check zstd-1.3.6.tar.gz.sha256
>> tar xzf zstd-1.3.6.tar.gz
>> SRC_LIB="zstd-1.3.6/lib"
>> DST_LIB="grub-core/lib/zstd"
>> rm -rf $DST_LIB
>> mkdir -p $DST_LIB
>> cp $SRC_LIB/zstd.h $DST_LIB/
>> cp $SRC_LIB/common/*.[hc] $DST_LIB/
>> cp $SRC_LIB/decompress/*.[hc] $DST_LIB/
>> rm $DST_LIB/{pool.[hc],threading.[hc]}
>> rm -rf zstd-1.3.6*
>> echo SUCCESS!
>> ```
>> Signed-off-by: Nick Terrell <address@hidden>
> I have a few concerns. First of all I can see that you include a lot of
> standard headers, e.g. stdlib.h, string.h, stddef.h, etc. Where do they
> come from? OS? If yes that is wrong. Additionally, I think that you
> should not use standard types, e.g. size_t, because this may not work if
> you cross compile. You have to use, e.g. grub_size_t. IMO the simplest
> solution would be definition and usage of relevant zstd_* types, e.g.
> zstd_size_t. Then e.g. zstd_size_t should be defined as grub_size_t.

I'm getting the standard headers from grub-core/lib/posix_wrap. I need
to add stddef.h. I'd like to keep using standard types, which are typedefed
to the grub_ equivalent in posix_wrap/sys/types.h.

I agree it would be nice if zstd put all of its dependencies into a 
file that we could replace. I'll look into adding this in a future version. 
since we are using upstream zstd as-is right now, and posix_wrapper is already
present, I think that the benefit of using upstream as-is outweighs the cost of
using the posix_wrapper. Does that sound reasonable to you?

To test that I'm using the right headers, I looked that the .Po files generated,
and saw that stddef.h was using the system, but the rest were using the
posix_wrapper. Is that the right approach? Is there something else I should
be testing?

> [...]


reply via email to

[Prev in Thread] Current Thread [Next in Thread]