Re: v2.35.0 DEVELOPER=1 regression

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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.

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

Thank you,
Dscho