Re: [PATCH] cache.h: auto-detect if zlib has uncompress2()

["René Scharfe" <l.s.r@web.de>]

Am 17.01.22 um 18:13 schrieb Ævar Arnfjörð Bjarmason:
> Change the NO_UNCOMPRESS2=Y setting to auto-detect those older zlib
> versions that don't have uncompress2().
>
> This makes the compilation of git less annoying on older systems,
> since the inclusion of a322920d0bb (Provide zlib's uncompress2 from
> compat/zlib-compat.c, 2021-10-07) in v2.35.0-rc0 our default
> dependency on a zlib 1.2.9 or newer unless NO_UNCOMPRESS2=Y is
> specified has resulted in errors when git is compiled.
>
> To get around those errors we've needed to bundle config.mak.uname
> changes such as such as 68d1da41c4e (build: NonStop ships with an
> older zlib, 2022-01-10) and the in-flight
> https://lore.kernel.org/git/20220116020520.26895-1-davvid@gmail.com/.
>
> Let's instead rely on ZLIB_VERNUM. Now only those systems where zlib
> is so broken that it can't be rely on (such a system probably doesn't
> exist) need to provide a NO_UNCOMPRESS2=Y.
>
> See 9da3acfb194 ([PATCH] compat: support pre-1.2 zlib, 2005-04-30) and
> 609a2289d76 (Improve accuracy of check for presence of deflateBound.,
> 2007-11-07) for in-tree prior art using ZLIB_VERNUM.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> I think this should be strongly considered for inclusion before the
> final v2.35.0 release.
>
> Aside from the ones already (and in-flight) in config.mak.uname, I've
> run into numerous other cases where NO_UNCOMPRESS2=y is needed (so far
> gcc{10,14,45,111,119,135,210} on the GCC farm). Adding
> config.mak.uname detections to those would be tedious, we'd need to
> start detecting various other OS versions.
>
> Or, we can just ask zlib.h abuot its ZLIB_VERSION instead, and include
> compat/zlib-uncompress2.c in our own zlib.c wrapper.
>
> This has an interaction with da/rhel7-lacks-uncompress2-and-c99 (the
> merge should preferably delete the NO_UNCOMPRESS2=Y it adds), it's in
> "next", but I didn't base this on that topic as "nex" clearly won't be
> merged down before v2.35.0.
>
>  Makefile                  |  6 ++++--
>  cache.h                   |  5 +++++
>  compat/zlib-uncompress2.c |  5 +----
>  config.mak.uname          |  6 ------
>  reftable/block.c          |  2 +-
>  reftable/system.h         | 12 +-----------
>  zlib.c                    |  3 +++
>  7 files changed, 15 insertions(+), 24 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 5580859afdb..3e90820bbfd 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -256,7 +256,10 @@ all::
>  #
>  # Define NO_DEFLATE_BOUND if your zlib does not have deflateBound.
>  #
> -# Define NO_UNCOMPRESS2 if your zlib does not have uncompress2.
> +# Define NO_UNCOMPRESS2 if your zlib is older than v1.2.9 and does not
> +# have uncompress2. You should not need to define this unless your
> +# zlib's ZLIB_VERNUM is broken. We'll auto-detect this on the basis of
> +# that macro.
>  #
>  # 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)
> @@ -1728,7 +1731,6 @@ endif
>
>  ifdef NO_UNCOMPRESS2
>  	BASIC_CFLAGS += -DNO_UNCOMPRESS2
> -	REFTABLE_OBJS += compat/zlib-uncompress2.o
>  endif
>
>  ifdef NO_POSIX_GOODIES
> diff --git a/cache.h b/cache.h
> index 281f00ab1b1..02b355fcf08 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -29,6 +29,11 @@ typedef struct git_zstream {
>  	unsigned char *next_out;
>  } git_zstream;
>
> +#if defined(NO_UNCOMPRESS2) || ZLIB_VERNUM < 0x1290
> +#define GIT_NO_UNCOMPRESS2 1
> +int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source,
> +		uLong *sourceLen);
> +#endif
>  void git_inflate_init(git_zstream *);
>  void git_inflate_init_gzip_only(git_zstream *);
>  void git_inflate_end(git_zstream *);
> diff --git a/compat/zlib-uncompress2.c b/compat/zlib-uncompress2.c
> index 722610b9718..915796e85ac 100644
> --- a/compat/zlib-uncompress2.c
> +++ b/compat/zlib-uncompress2.c
> @@ -8,15 +8,12 @@
>
>  */
>
> -#include "../reftable/system.h"
> -#define z_const

Why is it safe to remove this definition?  Because it's defined in
zconf.h, included by zlib.h.  But why did we need it in the first place?
Not caused by this patch, of course, but still strange.

> -
>  /*
>   * 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>
> +/* No "#include <zlib.h>", done in cache.h */

Well, it's rather something like "No #include, period.  Because this
file is not meant to be compiled on its own, but is included itself.",
isn't it?

>
>  /* clang-format off */
>
> diff --git a/config.mak.uname b/config.mak.uname
> index 9b3e9bff5f5..d0701f9beb0 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -261,10 +261,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 longer relying on OS version trivia like this is very nice.

>  	NO_STRCASESTR = YesPlease
>  	NO_MEMMEM = YesPlease
>  	USE_ST_TIMESPEC = YesPlease
> @@ -520,7 +516,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
> @@ -576,7 +571,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 a/reftable/block.c b/reftable/block.c
> index 855e3f5c947..946edd0f34e 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -13,7 +13,7 @@ license that can be found in the LICENSE file or at
>  #include "record.h"
>  #include "reftable-error.h"
>  #include "system.h"
> -#include <zlib.h>
> +#include "zlib.h"

Why?  We don't have a local zlib.h and this patch doesn't add one.

And don't you need to rather include cache.h, to get the definition
of uncompress2 on systems that don't have it in their zlib.h?

>
>  int header_size(int version)
>  {
> diff --git a/reftable/system.h b/reftable/system.h
> index 4907306c0c5..2cebbc94d4d 100644
> --- a/reftable/system.h
> +++ b/reftable/system.h
> @@ -15,17 +15,7 @@ license that can be found in the LICENSE file or at
>  #include "strbuf.h"
>  #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
> +#include "zlib.h"

Same here.  Or rather: Just here.  The include in reftable/block.c
can be removed safely, because it includes this reftable/system.h
here anyway.

>
>  int hash_size(uint32_t id);
>
> diff --git a/zlib.c b/zlib.c
> index d594cba3fc9..d9440dfb784 100644
> --- a/zlib.c
> +++ b/zlib.c
> @@ -3,6 +3,9 @@
>   * at init time.
>   */
>  #include "cache.h"
> +#ifdef GIT_NO_UNCOMPRESS2
> +#include "compat/zlib-uncompress2.c"
> +#endif

Keeping the function definition in its own file in compat/ with its
own license note is a good choice for maintainability, I think.

>
>  static const char *zerr_to_string(int status)
>  {