Re: [PATCH v5] compat: auto-detect if zlib has uncompress2()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 24, 2022, 19:07 UTC
Message-ID: <220124.86r18xgcv4.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqr18x3s5s.fsf@gitster.g>

On Mon, Jan 24 2022, Junio C Hamano wrote:

Show 12 quoted lines

> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> [...]
> Attempt to instead ask the system header <zlib.h> to decide if we
> need the compatibility implementation.  This is a deviation from the
> way we have been handling the "compatiblity" features so far, and if
> it can be done cleanly enough, it could work as a model for features
> that need compatibility definition we discover in the future.  With
> that goal in mind, avoid expedient but ugly hacks, like shoving the
> code that is conditionally compiled into an unrelated .c file, which
> may not work in future cases---instead, take an approach that uses a
> file that is independently compiled and stands on its own.
> [...]

I think so, we do auto-probes on version (e.g. for libcurl and libpcre), but I don't think we've outright defined some missing functions on that basis, we have defined some missing macros though.

FWIW I did some experiments the other day with handling this via the Makefile.

I.e. similar to how we generate the .depend/* dirs to optionally inject the building of various "configure" programs into the build step, so that e.g. git.c would depend on trying to compile a probe/uncompress2.c or whatever. We'd then write NO_UNCOMPRESS2=$(FOUND_IT) to a file we'd "include".

I think it's a generally nice thing to move towards, is relatively light, and would eventually allow us to get rid of the optional configure.ac (it would become redundant). We could also use it for e.g. a more accurate "detect-compiler" shellscript (instead of trying to guess from clang/gcc versions).

Show 5 quoted lines

>  - Beat found a trick used by OpenSSL to avoid making the
>    conditionally-compiled object truly empty (apparently because
>    they had to deal with compilers that do not want to see an
>    effectively empty input file).  Our compat/zlib-uncompress2.c
>    file borrows the same trick for portabilty.

Aside: I have not yet found such a compiler, does anyone know of one
that breaks? In any case doing this for good measure seems fine, just
wondering if we're cargo-culting a needless workaround or not.

Show 11 quoted lines

>  * With a single-liner update to the Makefile with an updated log
>    message that explains the change.  I am not sure if this version
>    can become the model of future "compat" support, or we should
>    just discard the new approach and use the Makefile macro approach
>    that has worked well for all of our existing compat support
>    already.
> [...]
>      -GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB)
>     ++# xdiff and reftable libs may in turn depend on what is in libgit.a
>      +GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
>       EXTLIBS =

As noted I'm rather "who cares? :)" on the question of how we get the uncompress2() compatibility function over to its eventual location (directly or via *.o linker indirection), but this approch in v5 works & addresses the issue I noted in v4. So this looks good to me!

FWIW I've tested this with this ad-hoc patch to simulate not having
uncompress2, and on systems that genuinely don't have it:
    
    diff --git a/compat/zlib-uncompress2.c b/compat/zlib-uncompress2.c
    index 77a1b080484..539c14c2b54 100644
    --- a/compat/zlib-uncompress2.c
    +++ b/compat/zlib-uncompress2.c
    @@ -3 +3 @@
    -#if ZLIB_VERNUM < 0x1290
    +#if 1
    @@ -37 +37 @@
    -int ZEXPORT uncompress2 (
    +int ZEXPORT uncompress3 (
    diff --git a/git-compat-util.h b/git-compat-util.h
    index ea111a7b481..a6df48d76d5 100644
    --- a/git-compat-util.h
    +++ b/git-compat-util.h
    @@ -1392 +1392 @@ void unleak_memory(const void *ptr, size_t len);
    -#if ZLIB_VERNUM < 0x1290
    +#if 1
    @@ -1397 +1397 @@ void unleak_memory(const void *ptr, size_t len);
    -int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source,
    +int uncompress3(Bytef *dest, uLongf *destLen, const Bytef *source,
    diff --git a/reftable/block.c b/reftable/block.c
    index 855e3f5c947..ee341eb65fe 100644
    --- a/reftable/block.c
    +++ b/reftable/block.c
    @@ -213 +213 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
    -		    uncompress2(uncompressed + block_header_skip, &dst_len,
    +		    uncompress3(uncompressed + block_header_skip, &dst_len,