On Tue, Jan 18 2022, Johannes Schindelin wrote: > Hi Junio, > > On Fri, 14 Jan 2022, Junio C Hamano wrote: > >> Junio C Hamano <gitster@xxxxxxxxx> 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@xxxxxxxxx/ 2. https://lore.kernel.org/git/patch-1.1-9cea01a1395-20220117T170457Z-avarab@xxxxxxxxx/
