| Message ID | xmqqh79t7sj4.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] compat: auto-detect if zlib has uncompress2() | expand |
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?
diff --git c/Makefile w/Makefile index 5580859afd..a32d278700 100644 --- c/Makefile +++ w/Makefile @@ -256,8 +256,6 @@ all:: # # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound. # -# Define NO_UNCOMPRESS2 if your zlib does not have uncompress2. -# # Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback, # as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299) # @@ -862,6 +860,7 @@ LIB_OBJS += commit-reach.o LIB_OBJS += commit.o LIB_OBJS += compat/obstack.o LIB_OBJS += compat/terminal.o +LIB_OBJS += compat/zlib-uncompress2.o LIB_OBJS += config.o LIB_OBJS += connect.o LIB_OBJS += connected.o @@ -1726,11 +1725,6 @@ ifdef NO_DEFLATE_BOUND BASIC_CFLAGS += -DNO_DEFLATE_BOUND endif -ifdef NO_UNCOMPRESS2 - BASIC_CFLAGS += -DNO_UNCOMPRESS2 - REFTABLE_OBJS += compat/zlib-uncompress2.o -endif - ifdef NO_POSIX_GOODIES BASIC_CFLAGS += -DNO_POSIX_GOODIES endif diff --git c/cache.h w/cache.h index 281f00ab1b..3a0142aa56 100644 --- c/cache.h +++ w/cache.h @@ -18,7 +18,6 @@ #include "repository.h" #include "mem-pool.h" -#include <zlib.h> typedef struct git_zstream { z_stream z; unsigned long avail_in; diff --git c/ci/lib.sh w/ci/lib.sh index 9d28ab50fb..cbc2f8f1ca 100755 --- c/ci/lib.sh +++ w/ci/lib.sh @@ -197,7 +197,6 @@ esac case "$jobname" in linux32) CC=gcc - MAKEFLAGS="$MAKEFLAGS NO_UNCOMPRESS2=1" ;; linux-musl) CC=gcc diff --git c/compat/zlib-uncompress2.c w/compat/zlib-uncompress2.c index 722610b971..77a1b08048 100644 --- c/compat/zlib-uncompress2.c +++ w/compat/zlib-uncompress2.c @@ -1,3 +1,6 @@ +#include "git-compat-util.h" + +#if ZLIB_VERNUM < 0x1290 /* taken from zlib's uncompr.c commit cacf7f1d4e3d44d871b605da3b647f07d718623f @@ -8,16 +11,11 @@ */ -#include "../reftable/system.h" -#define z_const - /* * Copyright (C) 1995-2003, 2010, 2014, 2016 Jean-loup Gailly, Mark Adler * For conditions of distribution and use, see copyright notice in zlib.h */ -#include <zlib.h> - /* clang-format off */ /* =========================================================================== @@ -93,3 +91,6 @@ int ZEXPORT uncompress2 ( err == Z_BUF_ERROR && left + stream.avail_out ? Z_DATA_ERROR : err; } +#else +static void *dummy_variable = &dummy_variable; +#endif diff --git c/config.mak.uname w/config.mak.uname index c48db45106..92ea00c219 100644 --- c/config.mak.uname +++ w/config.mak.uname @@ -66,7 +66,6 @@ ifeq ($(uname_S),Linux) # centos7/rhel7 provides gcc 4.8.5 and zlib 1.2.7. ifneq ($(findstring .el7.,$(uname_R)),) BASIC_CFLAGS += -std=c99 - NO_UNCOMPRESS2 = YesPlease endif endif ifeq ($(uname_S),GNU/kFreeBSD) @@ -266,10 +265,6 @@ ifeq ($(uname_S),FreeBSD) FILENO_IS_A_MACRO = UnfortunatelyYes endif ifeq ($(uname_S),OpenBSD) - # Versions < 7.0 need compatibility layer - ifeq ($(shell expr "$(uname_R)" : "[1-6]\."),2) - NO_UNCOMPRESS2 = UnfortunatelyYes - endif NO_STRCASESTR = YesPlease NO_MEMMEM = YesPlease USE_ST_TIMESPEC = YesPlease @@ -525,7 +520,6 @@ ifeq ($(uname_S),Interix) endif endif ifeq ($(uname_S),Minix) - NO_UNCOMPRESS2 = YesPlease NO_IPV6 = YesPlease NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease NO_NSEC = YesPlease @@ -581,7 +575,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL) NO_SETENV = YesPlease NO_UNSETENV = YesPlease NO_MKDTEMP = YesPlease - NO_UNCOMPRESS2 = YesPlease # Currently libiconv-1.9.1. OLD_ICONV = UnfortunatelyYes NO_REGEX = NeedsStartEnd diff --git c/configure.ac w/configure.ac index d60d494ee4..5ee25ec95c 100644 --- c/configure.ac +++ w/configure.ac @@ -664,22 +664,9 @@ AC_LINK_IFELSE([ZLIBTEST_SRC], NO_DEFLATE_BOUND=yes]) LIBS="$old_LIBS" -AC_DEFUN([ZLIBTEST_UNCOMPRESS2_SRC], [ -AC_LANG_PROGRAM([#include <zlib.h>], - [uncompress2(NULL,NULL,NULL,NULL);])]) -AC_MSG_CHECKING([for uncompress2 in -lz]) -old_LIBS="$LIBS" -LIBS="$LIBS -lz" -AC_LINK_IFELSE([ZLIBTEST_UNCOMPRESS2_SRC], - [AC_MSG_RESULT([yes])], - [AC_MSG_RESULT([no]) - NO_UNCOMPRESS2=yes]) -LIBS="$old_LIBS" - GIT_UNSTASH_FLAGS($ZLIB_PATH) GIT_CONF_SUBST([NO_DEFLATE_BOUND]) -GIT_CONF_SUBST([NO_UNCOMPRESS2]) # # Define NEEDS_SOCKET if linking with libc is not enough (SunOS, diff --git c/git-compat-util.h w/git-compat-util.h index 1229c8296b..ea111a7b48 100644 --- c/git-compat-util.h +++ w/git-compat-util.h @@ -1386,6 +1386,18 @@ void unleak_memory(const void *ptr, size_t len); #define UNLEAK(var) do {} while (0) #endif +#define z_const +#include <zlib.h> + +#if ZLIB_VERNUM < 0x1290 +/* + * This is uncompress2, which is only available in zlib >= 1.2.9 + * (released as of early 2017). See compat/zlib-uncompress2.c. + */ +int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source, + uLong *sourceLen); +#endif + /* * This include must come after system headers, since it introduces macros that * replace system names. diff --git c/reftable/system.h w/reftable/system.h index 4907306c0c..18f9207dfe 100644 --- c/reftable/system.h +++ w/reftable/system.h @@ -16,17 +16,6 @@ license that can be found in the LICENSE file or at #include "hash.h" /* hash ID, sizes.*/ #include "dir.h" /* remove_dir_recursively, for tests.*/ -#include <zlib.h> - -#ifdef NO_UNCOMPRESS2 -/* - * This is uncompress2, which is only available in zlib >= 1.2.9 - * (released as of early 2017) - */ -int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source, - uLong *sourceLen); -#endif - int hash_size(uint32_t id); #endif