On Sun, Jan 23 2022, Junio C Hamano wrote: > From: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> > > 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@xxxxxxxxx> > Helped-by: Beat Bolli <dev+git@xxxxxxxxx> > Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> > --- > > * 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@xxxxxxxxx/ 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?
