From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Beat Bolli" <dev+git@drbeat.li>,
"David Aguilar" <davvid@gmail.com>,
"Randall S . Becker" <randall.becker@nexbridge.ca>,
"Taylor Blau" <me@ttaylorr.com>,
"Carlo Marcelo Arenas Belón" <carenas@gmail.com>,
"René Scharfe" <l.s.r@web.de>
Subject: Re: [PATCH v4] compat: auto-detect if zlib has uncompress2()
Date: Mon, 24 Jan 2022 10:27:02 +0100 [thread overview]
Message-ID: <220124.86pmohii9n.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqh79t7sj4.fsf_-_@gitster.g>
On Sun, Jan 23 2022, Junio C Hamano wrote:
> 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.
> 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?
next prev parent reply other threads:[~2022-01-24 9:40 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-17 17:13 [PATCH] cache.h: auto-detect if zlib has uncompress2() Ævar Arnfjörð Bjarmason
2022-01-17 18:27 ` Junio C Hamano
2022-01-17 18:48 ` rsbecker
2022-01-17 19:49 ` René Scharfe
2022-01-20 2:43 ` Carlo Arenas
2022-01-19 9:45 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2022-01-19 19:17 ` Junio C Hamano
2022-01-20 1:16 ` [PATCH v3] " Ævar Arnfjörð Bjarmason
2022-01-21 23:23 ` Junio C Hamano
2022-01-21 23:44 ` rsbecker
2022-01-22 0:55 ` Junio C Hamano
2022-01-23 0:19 ` Beat Bolli
2022-01-23 19:18 ` Junio C Hamano
2022-01-24 2:54 ` [PATCH v4] compat: " Junio C Hamano
2022-01-24 9:27 ` Ævar Arnfjörð Bjarmason [this message]
2022-01-24 18:27 ` [PATCH v5] " Junio C Hamano
2022-01-24 19:07 ` Ævar Arnfjörð Bjarmason
2022-01-24 20:21 ` Junio C Hamano
2022-01-25 10:11 ` Carlo Arenas
2022-01-25 18:11 ` Junio C Hamano
2022-01-25 19:12 ` Ævar Arnfjörð Bjarmason
2022-01-26 7:23 ` Junio C Hamano
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=220124.86pmohii9n.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=carenas@gmail.com \
--cc=davvid@gmail.com \
--cc=dev+git@drbeat.li \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
--cc=me@ttaylorr.com \
--cc=randall.becker@nexbridge.ca \
/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 public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).