On Tue, Jan 18 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> 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. > > I hate it when people misrepresent what I said, even in an attempt > to spite me. Let's assume some mutual good faith here. I'm just trying to help debug last-minute issues in this release to help get it out the door. If I've misrepresented your views that's clearly true if you're the one claiming it, but it's not intentional. I was trying to read the tea leaves here in terms of helping patch triage along. My assessment of that NO_UNCOMPRESS2=Y issue from the boxes I've tested on is that it's a much more widespread problem new in this release (this is purely from testing on the GCC farm, however representative that is). It'll impact multiple OS's, linux distros & versions. Whereas the C11 warning is "just" recent FreeBSD && DEVELOPER=1. So I assumed if you weren't interested in the former before the final you probably wouldn't be in the latter, but wanted to provide a more narrow fix in case you were. > For the "check ZLIB_VERSION_NUM" topic, I gave you > specific things that needs to be different from the version posted > with reasons, fully expecting that a better version of the patch to > materialize soon (knowing how proliferate you can be when you want > to) to give us enough time to assess the potential damage. I can re-roll it sooner than later if you'd like, but figured per your "I like the basic idea of the patch, but I am afraid that it is way too late in the cycle"[1] that the resulting on-list distraction wouldn't be worth it at this point, if it wasn't being considered for being applied in the release window. Aside from "I can re-roll it". I also think that the point of making that change mostly (but not entirely) disappears if it isn't included in v2.35.0. I.e. the point of doing it is to avoid the one-time pain of anyone building new releases of git on $OLD_OS/$OLD_DISTRO not having to run into the compilation error that's fixed with NO_UNCOMPRESS2=Y. So if we put out a release without it anyone who's compiling git releases will need to adjust their build system for that anyway (or they're using autoconf, where it'll Just Work). If we then get this into v2.36.0 there'll be someone somewhere that benefits, but I'd think the ship has sailed for most of those who'd avoid the needless flag twiddling (git-packagers@ et al). So I don't know if it's even worth it to pursue that patch in that case... > I wouldn't call that "punted". I don't think I've used or heard that word outside of this ML. FWIW I'm using it in the sense of "kicking the can down the road" or "deferred it". Maybe that's incorrect, and I certainly stand corrected if I implied something that wasn't true about our views by using that word. > For this one, config.mak.dev patch WOULD only affect developer > builds, which is a much better solution than overriding what their > system headers want to do and force compiling with C99 mode. With > the status quo with today's code, with or without the patch Dscho > wants in this thread, means ANYBODY will be stopped when they > attempt to build with -std=gnu99 on FreeBSD, which is a GOOD thing. > > The reason why it is a much better solution to PUNT on using C99 > mode on FreeBSD is because this episode makes it very clear that > nobody tested building anything that use basename(), dirname(), > etc. with C99 mode on the platform. I do not trust such a build, > even if we could work around the system header breakage. Aside from anything else I think you're assuming a lot about warning hygiene in a typical C project. Git's use of even optional -Werror is fairly unusual. If you Google search that error you'll find numerous past reports of it, it just hasn't been solved. I also think we discussed around the -std=gnu99 change that the "C99 mode" in compilers we tested isn't impacting generated code (although of course it will if it's conditional on __STDC_VERSION___). Otherwise the generated objects are the same, it's just what warnings/errors you get that changes. In this case the fallback case already existed without my __generic hack, so forcing that codepath for libgen.h isn't new. > This time we got lucky and wereq stopped by a compilation error, but > I have a strong suspicion that C99-only mode of compiler on this > platform is not as well battle tested as their standard mode of > compilation that allows C11. I simply do not think we want to waste > developer's time with C99-only mode on this platform which may end > up debugging the platform and not our program. Next bug that will > hit us may not be so friendly to clearly break compilation. > Instead, the symptom may be a subtle runtime breakage that wastes > people's time. I'd share that opinion if this was aCC on HP/UX or xlc on AIX or whatever. But in this case it's just vanilla clang, not some scary platform-specific compiler. Its "C99-only mode" isn't anything different on FreeBSD than Linux. We're just talking about one specific and avoidable warning in /usr/include/libgen.h on that OS, once we're past that its "C99-mode" works just fine. > After developers who work on FreeBSD (not Git developers who uses > FreeBSD) ships an updated system headers so that more programs, not > just us, are built and testsd with C99-only mode, perhaps it becomes > usable as a platform to catch use of language features beyond C99, > like everybody else. But with such a clearly broken system headers, > I do not think today's FreeBSD is ready for that. I do trust their > default settings that allows C11, a lot more than C99-only > compilation with the "their libgen is broken, so here is a user side > workaround" patch. Fair enough. I'm happy for us to get past this one way or the other, whether it's with your patch or mine. I took your previous reservations about my suggestions to just do this C99 checking in CI-only to mean that you'd like to keep -std=gnu99 on DEVELOPER=1 if at all possible, which my approach does. 1. https://lore.kernel.org/git/xmqq7dayw732.fsf@gitster.g/
