git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jan 24, 2022, 09:27 UTC
Message-ID
<220124.86pmohii9n.gmgdl@evledraar.gmail.com>
In-Reply-To
<xmqqh79t7sj4.fsf_-_@gitster.g>
On Sun, Jan 23 2022, Junio C Hamano wrote:
Show 48 quoted lines
> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> We have a copy of uncompress2() implementation in compat/ so that we
> can build with an older version of zlib that lack the function, and
> the build procedure selects if it is used via the NO_UNCOMPRESS2
> $(MAKE) variable.  This is yet another "annoying" knob the porters
> need to tweak on platforms that are not common enough to have the
> default set in the config.mak.uname file.
>
> Ævar came up with an idea to instead ask the system header <zlib.h>
> to decide if we need the compatibility implementation.  We can
> compile and link compat/zlib-uncompress2.c file unconditionally, but
> conditionally hide the implementation behind #if/#endif when zlib
> version is 1.2.9 or newer, and unconditionally archive the resulting
> object file in the libgit.a to be picked up by the linker.
>
> There are a few things to note in the shape of the code base after
> this change:
>
>  - We no longer use NO_UNCOMPRESS2 knob; if the system header
>    <zlib.h> claims a version that is more cent than the library
>    actually is, this would break, but it is easy to add it back when
>    we find such a system.
>
>  - The object file compat/zlib-uncompress2.o is always compiled and
>    archived in libgit.a, just like a few other compat/ object files
>    already are.
>
>  - The inclusion of <zlib.h> is done in <git-compat-util.h>; we used
>    to do so from <cache.h> which includes <git-compat-util.h> as the
>    first thing it does, so from the *.c codes, there is no practical
>    change.
>
>  - 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.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Helped-by: Beat Bolli <dev+git@drbeat.li>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * So, here is an updated one, still retaining the authorship but
>    adjusting for the "no empty source" trick.  v3 seems to fail
>    "win+VS build" due to the use of an extra $(ZLIB_COMPAT_OBJS)
>    macro, [...]

I hadn't noticed that "win+VS build" issue, but it's another issue with the CMake.txt scraping of the Makefile. I.e. it scrapes $(LIB_OBJS) with a regex, and as soon as another variable is used to append to it it fails. It has special-casing for the $(COMPAT_OBJS) seen in the v3 diff context.

All of which b.t.w. would be avoided with the approach I suggested in in https://lore.kernel.org/git/patch-v2-3.3-cd62d8f92d1-20211101T191231Z-avarab@gmail.com/ of having it ask "make" for these values rather than scraping them.

>    and this iteration, which just uses LIB_OBJS as everybody
>    else does, should be sufficient to avoid introducing such an
>    issue.

The change you have here doesn't work because in relying on $(LIB_OBJS) you broke the linking of libreftable.a, which doesn't link using $(LIB_OBJS), but requires uncompress2().

I think you didn't test this on a system that doesn't have uncompress3(). Testing it can be emulated by just naming it uncompress3() or something, and making the "#if" macro check an "#if 1".

That linking issue can also be fixed, but overall I really don't see the point of this complexity of insisting that this fallback function must arrive via a compat object.

The approach I had in the v2 of just including these sources in the top-level zlib.c just works, without any of the added build system complexity.

So just taking the v2 would also work (perhaps with the config.mak.uname changes), or this v4 with compat/zlib-uncompress2.o hardcoded in both LIB_OBJS and REFTABLE_OBJS, so that the scraping in CMake.txt can understand it.

Show 11 quoted lines
>    One thing that I found a bit iffy is the use of "force z_const to
>    an empty string before including <zlib.h>".  This trick to work
>    around too old a version of zlib (according to Carlo) was used
>    only when compat/zlib-uncompress2.c included <zlib.h> via
>    <reftable/system.h>, but never done when <cache.h> included
>    <zlib.h>, which means that the two parts of the code could have
>    been using incompatible definitions of the same structs (many
>    struct definitions zlib uses have const members).  I opted to be
>    "conservative" and choose to cast away z_const before
>    <git-compat-util.> includes <zlib.h>, but we may want to drop it
>    to see if anybody screams.

Weren't any issues with this avoided because we didn't include the reftable/* sources, but compiled them and then linked libreftable.a?

Previous: Junio C HamanoNext: Junio C Hamano
Message 15 of 22 in “cache.h: auto-detect if zlib has uncompress2()”
  1. cache.h: auto-detect if zlib has uncompress2()Ævar Arnfjörð Bjarmason, Jan 17, 2022
  2. Junio C HamanoJan 17, 2022
  3. rsbecker@nexbridge.comJan 17, 2022
  4. René ScharfeJan 17, 2022
  5. Carlo ArenasJan 20, 2022
  6. cache.h: auto-detect if zlib has uncompress2()Ævar Arnfjörð Bjarmason, Jan 19, 2022
  7. Junio C HamanoJan 19, 2022
  8. cache.h: auto-detect if zlib has uncompress2()Ævar Arnfjörð Bjarmason, Jan 20, 2022
  9. Junio C HamanoJan 21, 2022
  10. rsbecker@nexbridge.comJan 21, 2022
  11. Junio C HamanoJan 22, 2022
  12. Beat BolliJan 23, 2022
  13. Junio C HamanoJan 23, 2022
  14. compat: auto-detect if zlib has uncompress2()Junio C Hamano, Jan 24, 2022
  15. Ævar Arnfjörð BjarmasonJan 24, 2022
  16. compat: auto-detect if zlib has uncompress2()Junio C Hamano, Jan 24, 2022
  17. Ævar Arnfjörð BjarmasonJan 24, 2022
  18. Junio C HamanoJan 24, 2022
  19. Carlo ArenasJan 25, 2022
  20. Junio C HamanoJan 25, 2022
  21. Ævar Arnfjörð BjarmasonJan 25, 2022
  22. Junio C HamanoJan 26, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.