[BUG] Re: Git 2.35.0-rc0

[BUG] Re: Git 2.35.0-rc0

[rsbecker]

Hi All,

Git 2.35.0-rc0 is not portable:

uncompress2(uncompressed + block_header_skip, &dst_len,
                   ^
"/home/ituglib/randall/jenkins/.jenkins/workspace/Git_Pipeline/reftable/bloc
k.c", line 213: error(114):
          identifier "uncompress2" is undefined

This is not a POSIX compliant call.

--Randall

Re: [BUG] Re: Git 2.35.0-rc0

[Taylor Blau]

On Mon, Jan 10, 2022 at 09:20:15PM -0500, rsbecker@nexbridge.com wrote:
> Hi All,
>
> Git 2.35.0-rc0 is not portable:
>
> uncompress2(uncompressed + block_header_skip, &dst_len,
>                    ^
> "/home/ituglib/randall/jenkins/.jenkins/workspace/Git_Pipeline/reftable/bloc
> k.c", line 213: error(114):
>           identifier "uncompress2" is undefined
>
> This is not a POSIX compliant call.

uncompress2() isn't a POSIX thing, it's part of zlib IIUC. The new
caller is in the reftable code, which provides a build knob
NO_UNCOMPRESS2 that you can use if your version of zlib does not have
uncompress2().

uncompress2() was added to zlib back in 37281ac (Add uncompress2()
function, which returns the input size used., 2016-11-15), which first
appeared in version 1.2.9.

If your system doesn't have a modern-ish zlib, you may try building with
that knob, or upgrading your system's copy of zlib. And if NonStop
doesn't have a modern zlib available at all, we should modify the
NonStop section of config.mak.uname.

Thanks,
Taylor

RE: [BUG] Re: Git 2.35.0-rc0

[rsbecker]

On January 10, 2022 9:53 PM, Taylor Blau wrote:
> On Mon, Jan 10, 2022 at 09:20:15PM -0500, rsbecker@nexbridge.com
> wrote:
> > Hi All,
> >
> > Git 2.35.0-rc0 is not portable:
> >
> > uncompress2(uncompressed + block_header_skip, &dst_len,
> >                    ^
> > "/home/ituglib/randall/jenkins/.jenkins/workspace/Git_Pipeline/reftabl
> > e/bloc
> > k.c", line 213: error(114):
> >           identifier "uncompress2" is undefined
> >
> > This is not a POSIX compliant call.
> 
> uncompress2() isn't a POSIX thing, it's part of zlib IIUC. The new caller is in the
> reftable code, which provides a build knob
> NO_UNCOMPRESS2 that you can use if your version of zlib does not have
> uncompress2().
> 
> uncompress2() was added to zlib back in 37281ac (Add uncompress2()
> function, which returns the input size used., 2016-11-15), which first
> appeared in version 1.2.9.
> 
> If your system doesn't have a modern-ish zlib, you may try building with that
> knob, or upgrading your system's copy of zlib. And if NonStop doesn't have a
> modern zlib available at all, we should modify the NonStop section of
> config.mak.uname.

There is no provision in reftable/block.c to avoid using uncompress2, so the knob will not help. Our zlibc is not that recent (as in it does not have uncompress2) and we cannot make the 2.35.0 timeframe to upgrade it. The current zlib seems to require gcc and is very difficult to port at this stage. This is a blocker situation.

--Randall

Re: [BUG] Re: Git 2.35.0-rc0

[Taylor Blau]

On Mon, Jan 10, 2022 at 09:57:57PM -0500, rsbecker@nexbridge.com wrote:
> > If your system doesn't have a modern-ish zlib, you may try building with that
> > knob, or upgrading your system's copy of zlib. And if NonStop doesn't have a
> > modern zlib available at all, we should modify the NonStop section of
> > config.mak.uname.
>
> There is no provision in reftable/block.c to avoid using uncompress2,
> so the knob will not help. Our zlibc is not that recent (as in it does
> not have uncompress2) and we cannot make the 2.35.0 timeframe to
> upgrade it. The current zlib seems to require gcc and is very
> difficult to port at this stage. This is a blocker situation.

NO_UNCOMPRESS2 does not avoid calling uncompress2, but instead compiles
a copy-and-pasted implementation in compat/ so that the function is
available.

Looking through it, I can't imagine that it wouldn't compile just fine
even on NonStop.

Have you tried building with NO_UNCOMPRESS2?

Thanks,
Taylor

RE: [BUG] Re: Git 2.35.0-rc0

[rsbecker]

On January 10, 2022 10:00 PM, Taylor Blau wrote:
> To: rsbecker@nexbridge.com
> On Mon, Jan 10, 2022 at 09:57:57PM -0500, rsbecker@nexbridge.com
> wrote:
> > > If your system doesn't have a modern-ish zlib, you may try building
> > > with that knob, or upgrading your system's copy of zlib. And if
> > > NonStop doesn't have a modern zlib available at all, we should
> > > modify the NonStop section of config.mak.uname.
> >
> > There is no provision in reftable/block.c to avoid using uncompress2,
> > so the knob will not help. Our zlibc is not that recent (as in it does
> > not have uncompress2) and we cannot make the 2.35.0 timeframe to
> > upgrade it. The current zlib seems to require gcc and is very
> > difficult to port at this stage. This is a blocker situation.
> 
> NO_UNCOMPRESS2 does not avoid calling uncompress2, but instead compiles
> a copy-and-pasted implementation in compat/ so that the function is available.
> 
> Looking through it, I can't imagine that it wouldn't compile just fine even on
> NonStop.
> 
> Have you tried building with NO_UNCOMPRESS2?

I'm starting a build with that now. Will report back tomorrow.
--R

RE: [BUG] Re: Git 2.35.0-rc0

[rsbecker]

On January 10, 2022 10:00 PM, Taylor Blau wrote:
> On Mon, Jan 10, 2022 at 09:57:57PM -0500, rsbecker@nexbridge.com
> wrote:
> > > If your system doesn't have a modern-ish zlib, you may try building
> > > with that knob, or upgrading your system's copy of zlib. And if
> > > NonStop doesn't have a modern zlib available at all, we should
> > > modify the NonStop section of config.mak.uname.
> >
> > There is no provision in reftable/block.c to avoid using uncompress2,
> > so the knob will not help. Our zlibc is not that recent (as in it does
> > not have uncompress2) and we cannot make the 2.35.0 timeframe to
> > upgrade it. The current zlib seems to require gcc and is very
> > difficult to port at this stage. This is a blocker situation.
> 
> NO_UNCOMPRESS2 does not avoid calling uncompress2, but instead compiles
> a copy-and-pasted implementation in compat/ so that the function is available.
> 
> Looking through it, I can't imagine that it wouldn't compile just fine even on
> NonStop.
> 
> Have you tried building with NO_UNCOMPRESS2?

The patch for NonStop to make this compile is as follows. Test is running - will be 2 days:

diff --git a/config.mak.uname b/config.mak.uname
index a3a779327f..9b3e9bff5f 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -576,6 +576,7 @@ 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

Could we get that into rc1?

I now have a different issue:

make -C t/ all
make[1]: Entering directory '/home/git/git/t'
rm -f -r 'test-results'
/usr/coreutils/bin/bash: /usr/bin/perl: Argument list too long

Is there anyway to move to xargs? I am not sure why /usr/bin/perl is having issues with the build.

Regards,
Randall

RE: [BUG] Re: Git 2.35.0-rc0

[rsbecker]

On January 10, 2022 10:52 PM, Taylor Blau wrote:
> On January 10, 2022 10:00 PM, Taylor Blau wrote:
> > On Mon, Jan 10, 2022 at 09:57:57PM -0500, rsbecker@nexbridge.com
> > wrote:
> > > > If your system doesn't have a modern-ish zlib, you may try
> > > > building with that knob, or upgrading your system's copy of zlib.
> > > > And if NonStop doesn't have a modern zlib available at all, we
> > > > should modify the NonStop section of config.mak.uname.
> > >
> > > There is no provision in reftable/block.c to avoid using
> > > uncompress2, so the knob will not help. Our zlibc is not that recent
> > > (as in it does not have uncompress2) and we cannot make the 2.35.0
> > > timeframe to upgrade it. The current zlib seems to require gcc and
> > > is very difficult to port at this stage. This is a blocker situation.
> >
> > NO_UNCOMPRESS2 does not avoid calling uncompress2, but instead
> > compiles a copy-and-pasted implementation in compat/ so that the function
> is available.
> >
> > Looking through it, I can't imagine that it wouldn't compile just fine
> > even on NonStop.
> >
> > Have you tried building with NO_UNCOMPRESS2?
> 
> The patch for NonStop to make this compile is as follows. Test is running - will
> be 2 days:
> 
> diff --git a/config.mak.uname b/config.mak.uname index
> a3a779327f..9b3e9bff5f 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -576,6 +576,7 @@ 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
> 
> Could we get that into rc1?

The above is working. We're at t2015 right now. Much relieved.
--Randall

Re: [BUG] Re: Git 2.35.0-rc0

[Taylor Blau]

On Mon, Jan 10, 2022 at 10:51:39PM -0500, rsbecker@nexbridge.com wrote:
> I now have a different issue:
>
> make -C t/ all
> make[1]: Entering directory '/home/git/git/t'
> rm -f -r 'test-results'
> /usr/coreutils/bin/bash: /usr/bin/perl: Argument list too long
>
> Is there anyway to move to xargs? I am not sure why /usr/bin/perl is
> having issues with the build.

Tracing through t/Makefile, I am _pretty_ sure that this Perl invocation
comes from the test-lint-shell-syntax recipe.

I wonder if something like this would do the trick?

--- 8< ---

diff --git a/t/Makefile b/t/Makefile
index 46cd5fc527..d959119133 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -90,7 +90,7 @@ test-lint-executable:
 		echo >&2 "non-executable tests:" $$bad; exit 1; }

 test-lint-shell-syntax:
-	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS) $(TPERF)
+	@echo $(T) $(THELPERS) $(TPERF) | xargs '$(PERL_PATH_SQ)' check-non-portable-shell.pl

 test-lint-filenames:
 	@# We do *not* pass a glob to ls-files but use grep instead, to catch

--- >8 ---

Thanks,
Taylor

RE: [BUG] Re: Git 2.35.0-rc0

[rsbecker]

On January 11, 2022 2:53 PM, Taylor Blau wrote:
> On Mon, Jan 10, 2022 at 10:51:39PM -0500, rsbecker@nexbridge.com
> wrote:
> > I now have a different issue:
> >
> > make -C t/ all
> > make[1]: Entering directory '/home/git/git/t'
> > rm -f -r 'test-results'
> > /usr/coreutils/bin/bash: /usr/bin/perl: Argument list too long
> >
> > Is there anyway to move to xargs? I am not sure why /usr/bin/perl is
> > having issues with the build.
> 
> Tracing through t/Makefile, I am _pretty_ sure that this Perl invocation comes
> from the test-lint-shell-syntax recipe.
> 
> I wonder if something like this would do the trick?
> 
> --- 8< ---
> 
> diff --git a/t/Makefile b/t/Makefile
> index 46cd5fc527..d959119133 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -90,7 +90,7 @@ test-lint-executable:
>  		echo >&2 "non-executable tests:" $$bad; exit 1; }
> 
>  test-lint-shell-syntax:
> -	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
> $(TPERF)
> +	@echo $(T) $(THELPERS) $(TPERF) | xargs '$(PERL_PATH_SQ)'
> +check-non-portable-shell.pl
> 
>  test-lint-filenames:
>  	@# We do *not* pass a glob to ls-files but use grep instead, to catch
> 
> --- >8 ---

I think we don't need this yet. My CI/CD system is a bit more robust and does not get stuck here.
--Randall

Re: [BUG] Re: Git 2.35.0-rc0

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> diff --git a/config.mak.uname b/config.mak.uname
> index a3a779327f..9b3e9bff5f 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -576,6 +576,7 @@ 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
>
> Could we get that into rc1?

Sure, with an appliable patch that is properly signed off.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Date: Mon, 10 Jan 2022 22:51:39 -0500
From: "Randall S. Becker" <rsbecker@nexbridge.com>
Subject: [PATCH] build: NonStop ships with an older zlib

Notably, it lacks uncompress2(); use the fallback we ship in our
tree instead.

not-Signed-off-yet-by: Randall S. Becker <rsbecker@nexbridge.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.mak.uname | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.uname b/config.mak.uname
index a3a779327f..9b3e9bff5f 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -576,6 +576,7 @@ 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

Re: [BUG] Re: Git 2.35.0-rc0

[Taylor Blau]

On Wed, Jan 12, 2022 at 11:18:20AM -0800, Junio C Hamano wrote:
> <rsbecker@nexbridge.com> writes:
>
> > diff --git a/config.mak.uname b/config.mak.uname
> > index a3a779327f..9b3e9bff5f 100644
> > --- a/config.mak.uname
> > +++ b/config.mak.uname
> > @@ -576,6 +576,7 @@ 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
> >
> > Could we get that into rc1?
>
> Sure, with an appliable patch that is properly signed off.

I'm happy to attach my s-o-b to the below as well (and in general, that
it's safe to assume my sign-off is provided to everything I post to the
list, unless I explicitly say otherwise), since I originally suggested
the diff.

Thanks,
Taylor

Re: [BUG] Re: Git 2.35.0-rc0

[Ævar Arnfjörð Bjarmason]

On Wed, Jan 12 2022, Junio C Hamano wrote:

> <rsbecker@nexbridge.com> writes:
>
>> diff --git a/config.mak.uname b/config.mak.uname
>> index a3a779327f..9b3e9bff5f 100644
>> --- a/config.mak.uname
>> +++ b/config.mak.uname
>> @@ -576,6 +576,7 @@ 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
>>
>> Could we get that into rc1?
>
> Sure, with an appliable patch that is properly signed off.
>
> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
> Date: Mon, 10 Jan 2022 22:51:39 -0500
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
> Subject: [PATCH] build: NonStop ships with an older zlib
>
> Notably, it lacks uncompress2(); use the fallback we ship in our
> tree instead.
>
> not-Signed-off-yet-by: Randall S. Becker <rsbecker@nexbridge.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  config.mak.uname | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index a3a779327f..9b3e9bff5f 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -576,6 +576,7 @@ 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

I've run into some other cases on testing on platforms hwere the
NO_UNCOMPRESS2 is now needed.

It's a bit of an annoyance, as git would previously compile cleanly on
almost any system with NO_CURL=Y NO_OPENSSL=Y, most of those with a C
compiler have a libz, even if it's ancient.

As I noted in
https://lore.kernel.org/git/87wnn62nhp.fsf@evledraar.gmail.com/ this use
of uncompress2() is just saving a few lines of boilerplate instead of
using the underlying zlib functions, which every other in-tree user
uses.

So I think it would be most welcome if someone wanted to work on moving
to that API use, and IMO such a change could even happen before the
final release. After all we don't use reftable for anything except the
tests it runs, so as long as those pass it IMO outweighs the new
annyonce in compiling git on those platforms.

Re: [BUG] Re: Git 2.35.0-rc0

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> https://lore.kernel.org/git/87wnn62nhp.fsf@evledraar.gmail.com/ this use
> of uncompress2() is just saving a few lines of boilerplate instead of
> using the underlying zlib functions, which every other in-tree user
> uses.

I looked at the source of uncompress2(), and I tend to agree that,
we do NOT HAVE TO add dependency on it.  You should be able to
rewrite the new caller without using it.

But this is a 5-year old API function, that first appeared how many
years ago in their public release?  In a few years, I would say that
we would be laughed at if we avoid it with "it is not available
everywhere".

And in the meantime, we ship a copy lifted from upstream, so those
with older zlib would be OK with it, too.

That is how I view what we have today.  To me, the logical
conclusion of the observation is that, we do not have good reason
to avoid it.

When we are adding a new piece of code that drives inflate(), we
should remember that we now have uncompress2() available in the
codebase, and see if we can simplify it by using uncompress2().  The
same is true for a case where we refactor existing code that can
become simpler with uncompress2().

Thanks.

RE: [BUG] Re: Git 2.35.0-rc0

[rsbecker]

Died a quick death. Someone added a call to uncompress2, which is not
POSIX-compliant.

> -----Original Message-----
> From: rsbecker@nexbridge.com <rsbecker@nexbridge.com>
> Sent: January 10, 2022 9:20 PM
> To: 'git@vger.kernel.org' <git@vger.kernel.org>
> Subject: [BUG] Re: Git 2.35.0-rc0
> 
> Hi All,
> 
> Git 2.35.0-rc0 is not portable:
> 
> uncompress2(uncompressed + block_header_skip, &dst_len,
>                    ^
>
"/home/ituglib/randall/jenkins/.jenkins/workspace/Git_Pipeline/reftable/bloc
> k.c", line 213: error(114):
>           identifier "uncompress2" is undefined
> 
> This is not a POSIX compliant call.
> 
> --Randall