Re: v2.35.0 DEVELOPER=1 regression

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Jan 18 2022, Johannes Schindelin wrote:

> Hi Junio,
>
> On Fri, 14 Jan 2022, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> >> I think we had this discussion about FreeBSD before and that's why I
>> >> specifically dropped that option from the main makefile.  We can either
>> >> drop that patch, or we can set it to -std=gnu11 and tell folks setting
>> >> DEVELOPER to use a system released in the last five years.  I think we
>> >> can be a little stricter with what we require in the case of DEVELOPER
>> >> than we might be otherwise.
>> >
>> > But that is not being stricter, but looser, no?  I thought that the
>> > point of -std=gnu99 was to allow us to use C99 features while catching
>> > use of language features newer than that, and use of -std=gnu11 will
>> > defeat half the point, wouldn't it?
>>
>> If FreeBSD (or any other platform) cannot do "reject features beyond
>> C99", I am perfectly OK to drop -std=gnu99 on such a place.
>>
>> DEVELOPER=YesPlease ought to be a collection of settings that helps
>> the developers the most.  So on platforms that *can* do "reject
>> features beyond C99", I prefer to have it enabled when
>> DEVELOPER=YesPlease is given.
>>
>> It seems that -std=gnu99 is only added conditionally even in today's
>> config.mak.dev, so it is fine if we dropped -std=gnu99 from there.
>> Which means that developers on FreeBSD cannot participate in vetting
>> use of features beyond C99, but there are developers on other
>> platforms who will, so it's not too bad, I would say.
>
> Plus, we have CI runs that do help us, thanks to setting `DEVELOPER=1`
> (see https://github.com/git/git/blob/v2.35.0-rc1/ci/lib.sh#L154).
>
>> As config.mak.dev is included after config.mak.uname, something like
>> this may be sufficient, perhaps?
>>
>>  config.mak.dev | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git i/config.mak.dev w/config.mak.dev
>> index d4afac6b51..3deb076d5e 100644
>> --- i/config.mak.dev
>> +++ w/config.mak.dev
>> @@ -20,9 +20,14 @@ endif
>>  endif
>>  endif
>>
>> +ifneq ($(uname_S),FreeBSD)
>>  ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang7,$(COMPILER_FEATURES))),)
>>  DEVELOPER_CFLAGS += -std=gnu99
>>  endif
>> +else
>> +# FreeBSD cannot limit to C99 because its system headers unconditionally
>> +# rely on C11 features.
>> +endif
>>
>>  DEVELOPER_CFLAGS += -Wdeclaration-after-statement
>>  DEVELOPER_CFLAGS += -Wformat-security
>>
>
> I applied this patch on top of the current tip of `seen`, opened a PR at
> https://github.com/gitgitgadget/git/pull/1116. The corresponding FreeBSD
> run is here: https://cirrus-ci.com/task/5867558132776960, and it
> succeeded.
>
> In addition, I concur that this is the best we can do, and I really would
> like to have that patch as soon as possible because it confuses new
> contributors when their PR builds fail out of no mistake of their own.

I just submitted a more narrow fix in the side-thread[1]. I've tested it
on the FreeBSD 13.0 box I have access to, but perhaps you'd like to test
it too?

> Please count this as a vote for including this patch in -rc2.

I'd like to have it too, but for context needing to add NO_UNCOMPRESS2=Y
(which Junio's punted on for the final[2]) is a much more widespread
issue of needing new post-install build tweaking than this issue that
only affects developer builds on FreeBSD.

What makes that FreeBSD run a part of the GGG CI? Another and more
narrow workaround for that in particular is to have whatever's driving
it add -Wno-c11-extensions to the CFLAGS.

1. https://lore.kernel.org/git/patch-1.1-06cc12be94d-20220118T151234Z-avarab@gmail.com/
2. https://lore.kernel.org/git/patch-1.1-9cea01a1395-20220117T170457Z-avarab@gmail.com/