* [PATCH] Makefile: enable DEVELOPER by default @ 2018-08-04 2:00 Stefan Beller 2018-08-04 2:02 ` Stefan Beller 0 siblings, 1 reply; 26+ messages in thread From: Stefan Beller @ 2018-08-04 2:00 UTC (permalink / raw) To: git; +Cc: avarab, Stefan Beller Reviewer bandwidth is limited, so let's have the machine of the (potentially new) contributor warn about issues with the code by default. As setting DEVELOPER, the compiler is stricter and we may run into problems on some architectures. But packagers of said platforms are knowledgeable enough to turn off this flag. (Also they are fewer than the number of new contributors) Signed-off-by: Stefan Beller <sbeller@google.com> --- Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Makefile b/Makefile index 41b93689add..95aa3ff3185 100644 --- a/Makefile +++ b/Makefile @@ -497,6 +497,8 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) ALL_LDFLAGS = $(LDFLAGS) STRIP ?= strip +DEVELOPER=1 + # Create as necessary, replace existing, make ranlib unneeded. ARFLAGS = rcs -- 2.18.0.597.ga71716f1ad-goog ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH] Makefile: enable DEVELOPER by default 2018-08-04 2:00 [PATCH] Makefile: enable DEVELOPER by default Stefan Beller @ 2018-08-04 2:02 ` Stefan Beller 2018-08-04 6:09 ` Jonathan Nieder 0 siblings, 1 reply; 26+ messages in thread From: Stefan Beller @ 2018-08-04 2:02 UTC (permalink / raw) To: sbeller; +Cc: avarab, git, git-packagers Reviewer bandwidth is limited, so let's have the machine of the (potentially new) contributor warn about issues with the code by default. As setting DEVELOPER, the compiler is stricter and we may run into problems on some architectures. But packagers of said platforms are knowledgeable enough to turn off this flag. (Also they are fewer than the number of new contributors) Signed-off-by: Stefan Beller <sbeller@google.com> --- Resending with <git-packagers@googlegroups.com> cc'd. Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Makefile b/Makefile index 41b93689add..95aa3ff3185 100644 --- a/Makefile +++ b/Makefile @@ -497,6 +497,8 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) ALL_LDFLAGS = $(LDFLAGS) STRIP ?= strip +DEVELOPER=1 + # Create as necessary, replace existing, make ranlib unneeded. ARFLAGS = rcs -- 2.18.0.597.ga71716f1ad-goog ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-04 2:02 ` Stefan Beller @ 2018-08-04 6:09 ` Jonathan Nieder 2018-08-04 6:38 ` Duy Nguyen 0 siblings, 1 reply; 26+ messages in thread From: Jonathan Nieder @ 2018-08-04 6:09 UTC (permalink / raw) To: Stefan Beller; +Cc: avarab, git, git-packagers, hanwen Hi, Stefan Beller wrote: > Reviewer bandwidth is limited, so let's have the machine of the > (potentially new) contributor warn about issues with the code by default. > > As setting DEVELOPER, the compiler is stricter and we may run into problems > on some architectures. But packagers of said platforms are knowledgeable > enough to turn off this flag. (Also they are fewer than the number of new > contributors) Which architectures would we run into problems on? Aren't those bugs that should themselves be fixed? I think you are right that the packagers will cope with whatever setting we choose. My main concern is not about them but about other people building from source in order to run (instead of to develop) Git, and by extension, the people they go to for help when it doesn't work. I have lots of bitter experience of -Werror being a support headache and leading to bad workarounds when someone upgrades their compiler and the build starts failing due to a new warning it has introduced. > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > Makefile | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Makefile b/Makefile > index 41b93689add..95aa3ff3185 100644 > --- a/Makefile > +++ b/Makefile > @@ -497,6 +497,8 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) > ALL_LDFLAGS = $(LDFLAGS) > STRIP ?= strip > > +DEVELOPER=1 I like the idea of making this more discoverable to new contributors. It seems that Documentation/SubmittingPatches doesn't mention this setting. Should it? Should a non-DEVELOPER build print a note encouraging enabling this setting in case you're developing patches meant for submission to the project? Should we have a CONTRIBUTING.md file suggesting this setting? Other ideas for ensuring it's enabled for those who need it? Thanks and hope that helps, Jonathan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-04 6:09 ` Jonathan Nieder @ 2018-08-04 6:38 ` Duy Nguyen 2018-08-04 17:19 ` Junio C Hamano 2018-08-05 2:42 ` Eric Sunshine 0 siblings, 2 replies; 26+ messages in thread From: Duy Nguyen @ 2018-08-04 6:38 UTC (permalink / raw) To: Jonathan Nieder Cc: Stefan Beller, Ævar Arnfjörð Bjarmason, Git Mailing List, git-packagers, Han-Wen Nienhuys On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder <jrnieder@gmail.com> wrote: > My main concern is not about them but about other > people building from source in order to run (instead of to develop) > Git, and by extension, the people they go to for help when it doesn't > work. I have lots of bitter experience of -Werror being a support > headache and leading to bad workarounds when someone upgrades their > compiler and the build starts failing due to a new warning it has > introduced. Even old compilers can also throw some silly, false positive warnings (which now turn into errors) because they are not as smart as new ones. -- Duy ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-04 6:38 ` Duy Nguyen @ 2018-08-04 17:19 ` Junio C Hamano 2018-08-06 16:40 ` Ævar Arnfjörð Bjarmason 2018-08-05 2:42 ` Eric Sunshine 1 sibling, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2018-08-04 17:19 UTC (permalink / raw) To: Duy Nguyen Cc: Jonathan Nieder, Stefan Beller, Ævar Arnfjörð Bjarmason, Git Mailing List, git-packagers, Han-Wen Nienhuys Duy Nguyen <pclouds@gmail.com> writes: > On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder <jrnieder@gmail.com> wrote: >> My main concern is not about them but about other >> people building from source in order to run (instead of to develop) >> Git, and by extension, the people they go to for help when it doesn't >> work. I have lots of bitter experience of -Werror being a support >> headache and leading to bad workarounds when someone upgrades their >> compiler and the build starts failing due to a new warning it has >> introduced. > > Even old compilers can also throw some silly, false positive warnings > (which now turn into errors) because they are not as smart as new > ones. I agree with both of the above. I do not think the pros-and-cons are in favor of forcing the developer bit to everybody, even though I am sympathetic to the desire to see people throw fewer bad changes that waste review bandwidth by not compiling or passing its own tests at us. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-04 17:19 ` Junio C Hamano @ 2018-08-06 16:40 ` Ævar Arnfjörð Bjarmason 2018-08-06 17:02 ` Jeff King 2018-08-06 17:02 ` Randall S. Becker 0 siblings, 2 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-08-06 16:40 UTC (permalink / raw) To: Junio C Hamano Cc: Duy Nguyen, Jonathan Nieder, Stefan Beller, Git Mailing List, git-packagers, Han-Wen Nienhuys On Sat, Aug 04 2018, Junio C Hamano wrote: > Duy Nguyen <pclouds@gmail.com> writes: > >> On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder <jrnieder@gmail.com> wrote: >>> My main concern is not about them but about other >>> people building from source in order to run (instead of to develop) >>> Git, and by extension, the people they go to for help when it doesn't >>> work. I have lots of bitter experience of -Werror being a support >>> headache and leading to bad workarounds when someone upgrades their >>> compiler and the build starts failing due to a new warning it has >>> introduced. >> >> Even old compilers can also throw some silly, false positive warnings >> (which now turn into errors) because they are not as smart as new >> ones. > > I agree with both of the above. I do not think the pros-and-cons > are in favor of forcing the developer bit to everybody, even though > I am sympathetic to the desire to see people throw fewer bad changes > that waste review bandwidth by not compiling or passing its own > tests at us. I agree. Responding to the thread in general, perhaps people would like this more if we turned DEVELOPER=1 DEVOPTS=no-error on by default? That's basically why I added it in 99f763baf5 ("Makefile: add a DEVOPTS to suppress -Werror under DEVELOPER", 2018-04-14), because I wanted the abilty to have verbose informative output without the build dying on some older systems / compilers. It's fine and understandable if you're someone who's just building a package on some older system if you get a bunch of compiler warnings, but more annoying if you have to dig into how to disable a default -Werror. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-06 16:40 ` Ævar Arnfjörð Bjarmason @ 2018-08-06 17:02 ` Jeff King 2018-08-06 17:04 ` Randall S. Becker ` (3 more replies) 2018-08-06 17:02 ` Randall S. Becker 1 sibling, 4 replies; 26+ messages in thread From: Jeff King @ 2018-08-06 17:02 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Junio C Hamano, Duy Nguyen, Jonathan Nieder, Stefan Beller, Git Mailing List, git-packagers, Han-Wen Nienhuys On Mon, Aug 06, 2018 at 06:40:14PM +0200, Ævar Arnfjörð Bjarmason wrote: > Responding to the thread in general, perhaps people would like this more > if we turned DEVELOPER=1 DEVOPTS=no-error on by default? > > That's basically why I added it in 99f763baf5 ("Makefile: add a DEVOPTS > to suppress -Werror under DEVELOPER", 2018-04-14), because I wanted the > abilty to have verbose informative output without the build dying on > some older systems / compilers. > > It's fine and understandable if you're someone who's just building a > package on some older system if you get a bunch of compiler warnings, > but more annoying if you have to dig into how to disable a default > -Werror. I had the impression that DEVELOPER=1 was allowed to set flags that old versions might not even know about. Hence they might actually barf, even without -Werror. Maybe that's better since the introduction of the detect-compiler script, though. I do think we may have a skewed view of the population on this list. We're developers ourselves, and we interact with new developers that we want to help. But there are masses of people[1] building Git who are _not_ developers, and want the default to be as robust as possible. They're probably not going to show up in this thread. -Peff [1] I actually wonder how large that mass is. Clearly there are many orders of magnitude more users than there are developers. But I have no idea what percentage of them build from source versus using somebody else's binary package. ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH] Makefile: enable DEVELOPER by default 2018-08-06 17:02 ` Jeff King @ 2018-08-06 17:04 ` Randall S. Becker 2018-08-06 17:11 ` Jonathan Nieder ` (2 subsequent siblings) 3 siblings, 0 replies; 26+ messages in thread From: Randall S. Becker @ 2018-08-06 17:04 UTC (permalink / raw) To: 'Jeff King', 'Ævar Arnfjörð Bjarmason' Cc: 'Junio C Hamano', 'Duy Nguyen', 'Jonathan Nieder', 'Stefan Beller', 'Git Mailing List', git-packagers, 'Han-Wen Nienhuys' On August 6, 2018 1:02 PM, Peff wrote: > To: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > Cc: Junio C Hamano <gitster@pobox.com>; Duy Nguyen > <pclouds@gmail.com>; Jonathan Nieder <jrnieder@gmail.com>; Stefan > Beller <sbeller@google.com>; Git Mailing List <git@vger.kernel.org>; git- > packagers@googlegroups.com; Han-Wen Nienhuys <hanwen@google.com> > Subject: Re: [PATCH] Makefile: enable DEVELOPER by default > > On Mon, Aug 06, 2018 at 06:40:14PM +0200, Ævar Arnfjörð Bjarmason > wrote: > > > Responding to the thread in general, perhaps people would like this > > more if we turned DEVELOPER=1 DEVOPTS=no-error on by default? > > > > That's basically why I added it in 99f763baf5 ("Makefile: add a > > DEVOPTS to suppress -Werror under DEVELOPER", 2018-04-14), because I > > wanted the abilty to have verbose informative output without the build > > dying on some older systems / compilers. > > > > It's fine and understandable if you're someone who's just building a > > package on some older system if you get a bunch of compiler warnings, > > but more annoying if you have to dig into how to disable a default > > -Werror. > > I had the impression that DEVELOPER=1 was allowed to set flags that old > versions might not even know about. Hence they might actually barf, even > without -Werror. Maybe that's better since the introduction of the detect- > compiler script, though. > > I do think we may have a skewed view of the population on this list. > We're developers ourselves, and we interact with new developers that we > want to help. But there are masses of people[1] building Git who are _not_ > developers, and want the default to be as robust as possible. > They're probably not going to show up in this thread. > > -Peff > > [1] I actually wonder how large that mass is. Clearly there are many > orders of magnitude more users than there are developers. But I have > no idea what percentage of them build from source versus using > somebody else's binary package. One? 😉 Jokingly, Randall -- Brief whoami: NonStop developer since approximately 211288444200000000 UNIX developer since approximately 421664400 -- In my real life, I talk too much. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-06 17:02 ` Jeff King 2018-08-06 17:04 ` Randall S. Becker @ 2018-08-06 17:11 ` Jonathan Nieder 2018-08-06 18:59 ` Jeff King 2018-08-06 17:39 ` Ævar Arnfjörð Bjarmason 2018-08-06 18:38 ` Stefan Beller 3 siblings, 1 reply; 26+ messages in thread From: Jonathan Nieder @ 2018-08-06 17:11 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano, Duy Nguyen, Stefan Beller, Git Mailing List, git-packagers, Han-Wen Nienhuys Hi, Jeff King wrote: > I had the impression that DEVELOPER=1 was allowed to set flags that old > versions might not even know about. Hence they might actually barf, even > without -Werror. Yes. [...] > We're developers ourselves, and we interact with new developers that we > want to help. But there are masses of people[1] building Git who are > _not_ developers, and want the default to be as robust as possible. > They're probably not going to show up in this thread. > > -Peff > > [1] I actually wonder how large that mass is. Clearly there are many > orders of magnitude more users than there are developers. But I have > no idea what percentage of them build from source versus using > somebody else's binary package. Relatedly, we need to think about the incentives these defaults create. Personally, I want *more* naive users to be building from source, because then they are better able to test recent versions, bisect, test my patches, etc. As I hinted in my earlier reply, I think it would be best to try some basic things to make DEVELOPER more visible first. If that fails, then we can revisit how to make this more drastic change in a way that minimizes the harm (and I am not sure yet that that is possible). Thanks, Jonathan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-06 17:11 ` Jonathan Nieder @ 2018-08-06 18:59 ` Jeff King 0 siblings, 0 replies; 26+ messages in thread From: Jeff King @ 2018-08-06 18:59 UTC (permalink / raw) To: Jonathan Nieder Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano, Duy Nguyen, Stefan Beller, Git Mailing List, git-packagers, Han-Wen Nienhuys On Mon, Aug 06, 2018 at 10:11:19AM -0700, Jonathan Nieder wrote: > > We're developers ourselves, and we interact with new developers that we > > want to help. But there are masses of people[1] building Git who are > > _not_ developers, and want the default to be as robust as possible. > > They're probably not going to show up in this thread. > > > > -Peff > > > > [1] I actually wonder how large that mass is. Clearly there are many > > orders of magnitude more users than there are developers. But I have > > no idea what percentage of them build from source versus using > > somebody else's binary package. > > Relatedly, we need to think about the incentives these defaults > create. Personally, I want *more* naive users to be building from > source, because then they are better able to test recent versions, > bisect, test my patches, etc. > > As I hinted in my earlier reply, I think it would be best to try some > basic things to make DEVELOPER more visible first. If that fails, > then we can revisit how to make this more drastic change in a way that > minimizes the harm (and I am not sure yet that that is possible). Yes, I agree very much with both of those paragraphs. -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-06 17:02 ` Jeff King 2018-08-06 17:04 ` Randall S. Becker 2018-08-06 17:11 ` Jonathan Nieder @ 2018-08-06 17:39 ` Ævar Arnfjörð Bjarmason 2018-08-06 18:38 ` Stefan Beller 3 siblings, 0 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-08-06 17:39 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Duy Nguyen, Jonathan Nieder, Stefan Beller, Git Mailing List, git-packagers, Han-Wen Nienhuys On Mon, Aug 06 2018, Jeff King wrote: > On Mon, Aug 06, 2018 at 06:40:14PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Responding to the thread in general, perhaps people would like this more >> if we turned DEVELOPER=1 DEVOPTS=no-error on by default? >> >> That's basically why I added it in 99f763baf5 ("Makefile: add a DEVOPTS >> to suppress -Werror under DEVELOPER", 2018-04-14), because I wanted the >> abilty to have verbose informative output without the build dying on >> some older systems / compilers. >> >> It's fine and understandable if you're someone who's just building a >> package on some older system if you get a bunch of compiler warnings, >> but more annoying if you have to dig into how to disable a default >> -Werror. > > I had the impression that DEVELOPER=1 was allowed to set flags that old > versions might not even know about. Hence they might actually barf, even > without -Werror. Maybe that's better since the introduction of the > detect-compiler script, though. I misrecalled that -Wimadethis-up wouldn't error on e.g. GCC today in the interest of forward-compatibility, but it does. So that changes things. Although we could today pick some set of flags greater than what we use today if there's interest, and enough compiler compatibility. > I do think we may have a skewed view of the population on this list. > We're developers ourselves, and we interact with new developers that we > want to help. But there are masses of people[1] building Git who are > _not_ developers, and want the default to be as robust as possible. > They're probably not going to show up in this thread. Indeed. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-06 17:02 ` Jeff King ` (2 preceding siblings ...) 2018-08-06 17:39 ` Ævar Arnfjörð Bjarmason @ 2018-08-06 18:38 ` Stefan Beller 3 siblings, 0 replies; 26+ messages in thread From: Stefan Beller @ 2018-08-06 18:38 UTC (permalink / raw) To: Jeff King Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano, Duy Nguyen, Jonathan Nieder, git, git-packagers, Han-Wen Nienhuys > I had the impression that DEVELOPER=1 was allowed to set flags that old > versions might not even know about. Hence they might actually barf, even > without -Werror. Maybe that's better since the introduction of the > detect-compiler script, though. > > I do think we may have a skewed view of the population on this list. > We're developers ourselves, and we interact with new developers that we > want to help. But there are masses of people[1] building Git who are > _not_ developers, and want the default to be as robust as possible. > They're probably not going to show up in this thread. Good point about the skewed perception of people who compile Git themselves. At first I thought this could be mitigated by detecting if someone is a developer by being more clever, for example if they have a commit (at HEAD) that is not contained in any remote, it is a pretty good indicator that fresh code was written. But this could also be the case for some non mainstream platform, that needs fixing. But then I entertained the thought that we actually do not care about people committing non conforming code to their copy of git.git, but actually only care about the code when we see it (i.e. upon review). So I wonder if we want to "fork" git-send-email and provide a tool to send emails with a pre-send compile&lint check specific to our code base. But then all these sound a lot more complicated than a simple knob that we turn on and off. So I don't look any further into it. Stefan ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH] Makefile: enable DEVELOPER by default 2018-08-06 16:40 ` Ævar Arnfjörð Bjarmason 2018-08-06 17:02 ` Jeff King @ 2018-08-06 17:02 ` Randall S. Becker 2018-08-06 17:41 ` Ævar Arnfjörð Bjarmason 2018-09-01 21:01 ` Kaartic Sivaraam 1 sibling, 2 replies; 26+ messages in thread From: Randall S. Becker @ 2018-08-06 17:02 UTC (permalink / raw) To: 'Ævar Arnfjörð Bjarmason', 'Junio C Hamano' Cc: 'Duy Nguyen', 'Jonathan Nieder', 'Stefan Beller', 'Git Mailing List', git-packagers, 'Han-Wen Nienhuys' On August 6, 2018 12:40 PM, Ævar Arnfjörð Bjarmason wrote: > On Sat, Aug 04 2018, Junio C Hamano wrote: > > > Duy Nguyen <pclouds@gmail.com> writes: > > > >> On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder <jrnieder@gmail.com> > wrote: > >>> My main concern is not about them but about other people building > >>> from source in order to run (instead of to develop) Git, and by > >>> extension, the people they go to for help when it doesn't work. I > >>> have lots of bitter experience of -Werror being a support headache > >>> and leading to bad workarounds when someone upgrades their compiler > >>> and the build starts failing due to a new warning it has introduced. > >> > >> Even old compilers can also throw some silly, false positive warnings > >> (which now turn into errors) because they are not as smart as new > >> ones. > > > > I agree with both of the above. I do not think the pros-and-cons are > > in favor of forcing the developer bit to everybody, even though I am > > sympathetic to the desire to see people throw fewer bad changes that > > waste review bandwidth by not compiling or passing its own tests at > > us. > > I agree. > > Responding to the thread in general, perhaps people would like this more if > we turned DEVELOPER=1 DEVOPTS=no-error on by default? > > That's basically why I added it in 99f763baf5 ("Makefile: add a DEVOPTS to > suppress -Werror under DEVELOPER", 2018-04-14), because I wanted the > abilty to have verbose informative output without the build dying on some > older systems / compilers. > > It's fine and understandable if you're someone who's just building a package > on some older system if you get a bunch of compiler warnings, but more > annoying if you have to dig into how to disable a default -Werror. Any idea when this is going to be in an official release, and exactly what the settings will be for "Not Developer". I assume DEVELOPER=0 and DEVOPTS=error, which is the current behaviour, correct? I am the platform maintainer for HPE NonStop and need to make sure I'm not packaging DEV builds to anyone, since I'm the only one doing this for the platform. It's another hoop, but hopefully not a bad one. The question is the best place to set this, assuming we are using Jenkins for our builds, and I'd rather keep the existing config.mak.uname the same, since at least it seems stable. We currently just run "make" for our build. So make arguments? And is making the change now non-destructive in preparation so that I don't forget when the time comes? Cheers, Randall -- Brief whoami: NonStop developer since approximately 211288444200000000 UNIX developer since approximately 421664400 -- In my real life, I talk too much. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-06 17:02 ` Randall S. Becker @ 2018-08-06 17:41 ` Ævar Arnfjörð Bjarmason 2018-08-06 19:20 ` Randall S. Becker 2018-09-01 21:01 ` Kaartic Sivaraam 1 sibling, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-08-06 17:41 UTC (permalink / raw) To: Randall S. Becker Cc: 'Junio C Hamano', 'Duy Nguyen', 'Jonathan Nieder', 'Stefan Beller', 'Git Mailing List', git-packagers, 'Han-Wen Nienhuys' On Mon, Aug 06 2018, Randall S. Becker wrote: > On August 6, 2018 12:40 PM, Ævar Arnfjörð Bjarmason wrote: >> On Sat, Aug 04 2018, Junio C Hamano wrote: >> >> > Duy Nguyen <pclouds@gmail.com> writes: >> > >> >> On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder <jrnieder@gmail.com> >> wrote: >> >>> My main concern is not about them but about other people building >> >>> from source in order to run (instead of to develop) Git, and by >> >>> extension, the people they go to for help when it doesn't work. I >> >>> have lots of bitter experience of -Werror being a support headache >> >>> and leading to bad workarounds when someone upgrades their compiler >> >>> and the build starts failing due to a new warning it has introduced. >> >> >> >> Even old compilers can also throw some silly, false positive warnings >> >> (which now turn into errors) because they are not as smart as new >> >> ones. >> > >> > I agree with both of the above. I do not think the pros-and-cons are >> > in favor of forcing the developer bit to everybody, even though I am >> > sympathetic to the desire to see people throw fewer bad changes that >> > waste review bandwidth by not compiling or passing its own tests at >> > us. >> >> I agree. >> >> Responding to the thread in general, perhaps people would like this more if >> we turned DEVELOPER=1 DEVOPTS=no-error on by default? >> >> That's basically why I added it in 99f763baf5 ("Makefile: add a DEVOPTS to >> suppress -Werror under DEVELOPER", 2018-04-14), because I wanted the >> abilty to have verbose informative output without the build dying on some >> older systems / compilers. >> >> It's fine and understandable if you're someone who's just building a package >> on some older system if you get a bunch of compiler warnings, but more >> annoying if you have to dig into how to disable a default -Werror. > > I am the platform maintainer for HPE NonStop and need to make sure I'm > not packaging DEV builds to anyone Perhaps confusingly, the DEVELOPER=1 flag in git is not like the developer flag in some other projects. It's purely there to turn on extra compiler warnings (by default, fatal), it doesn't e.g. turn on extra asserts, tracing, or suppress stripping of the binaries. So if we enabled some variant of it by default it would be fine to ship the result of that to your users, e.g. I ship DEVELOPER=1 builds to users. ^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH] Makefile: enable DEVELOPER by default 2018-08-06 17:41 ` Ævar Arnfjörð Bjarmason @ 2018-08-06 19:20 ` Randall S. Becker 0 siblings, 0 replies; 26+ messages in thread From: Randall S. Becker @ 2018-08-06 19:20 UTC (permalink / raw) To: 'Ævar Arnfjörð Bjarmason' Cc: 'Junio C Hamano', 'Duy Nguyen', 'Jonathan Nieder', 'Stefan Beller', 'Git Mailing List', git-packagers, 'Han-Wen Nienhuys' On August 6, 2018 1:42 PM, Ævar Arnfjörð Bjarmason wrote: > On Mon, Aug 06 2018, Randall S. Becker wrote: > > > On August 6, 2018 12:40 PM, Ævar Arnfjörð Bjarmason wrote: > >> On Sat, Aug 04 2018, Junio C Hamano wrote: > >> > >> > Duy Nguyen <pclouds@gmail.com> writes: > >> > > >> >> On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder > >> >> <jrnieder@gmail.com> > >> wrote: > >> >>> My main concern is not about them but about other people building > >> >>> from source in order to run (instead of to develop) Git, and by > >> >>> extension, the people they go to for help when it doesn't work. > >> >>> I have lots of bitter experience of -Werror being a support > >> >>> headache and leading to bad workarounds when someone upgrades > >> >>> their compiler and the build starts failing due to a new warning it has > introduced. > >> >> > >> >> Even old compilers can also throw some silly, false positive > >> >> warnings (which now turn into errors) because they are not as > >> >> smart as new ones. > >> > > >> > I agree with both of the above. I do not think the pros-and-cons > >> > are in favor of forcing the developer bit to everybody, even though > >> > I am sympathetic to the desire to see people throw fewer bad > >> > changes that waste review bandwidth by not compiling or passing its > >> > own tests at us. > >> > >> I agree. > >> > >> Responding to the thread in general, perhaps people would like this > >> more if we turned DEVELOPER=1 DEVOPTS=no-error on by default? > >> > >> That's basically why I added it in 99f763baf5 ("Makefile: add a > >> DEVOPTS to suppress -Werror under DEVELOPER", 2018-04-14), because I > >> wanted the abilty to have verbose informative output without the > >> build dying on some older systems / compilers. > >> > >> It's fine and understandable if you're someone who's just building a > >> package on some older system if you get a bunch of compiler warnings, > >> but more annoying if you have to dig into how to disable a default - > Werror. > > > > I am the platform maintainer for HPE NonStop and need to make sure I'm > > not packaging DEV builds to anyone > > Perhaps confusingly, the DEVELOPER=1 flag in git is not like the developer > flag in some other projects. It's purely there to turn on extra compiler > warnings (by default, fatal), it doesn't e.g. turn on extra asserts, tracing, or > suppress stripping of the binaries. > > So if we enabled some variant of it by default it would be fine to ship the > result of that to your users, e.g. I ship DEVELOPER=1 builds to users. That works for me. I generally consider warnings to be errors in my builds anyway, by policy (except when I have no choice in the matter and the warning is explainable). Cheers, Randall -- Brief whoami: NonStop developer since approximately 211288444200000000 UNIX developer since approximately 421664400 -- In my real life, I talk too much. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-06 17:02 ` Randall S. Becker 2018-08-06 17:41 ` Ævar Arnfjörð Bjarmason @ 2018-09-01 21:01 ` Kaartic Sivaraam 1 sibling, 0 replies; 26+ messages in thread From: Kaartic Sivaraam @ 2018-09-01 21:01 UTC (permalink / raw) To: Randall S. Becker, 'Ævar Arnfjörð Bjarmason', 'Junio C Hamano' Cc: 'Duy Nguyen', 'Jonathan Nieder', 'Stefan Beller', 'Git Mailing List', git-packagers, 'Han-Wen Nienhuys' On Mon, 2018-08-06 at 13:02 -0400, Randall S. Becker wrote: > > Any idea when this is going to be in an official release, and exactly > what the settings will be for "Not Developer". I assume DEVELOPER=0 > and DEVOPTS=error, which is the current behaviour, correct? I am the > platform maintainer for HPE NonStop and need to make sure I'm not > packaging DEV builds to anyone, since I'm the only one doing this for > the platform. It's another hoop, but hopefully not a bad one. The > question is the best place to set this, assuming we are using Jenkins > for our builds, and I'd rather keep the existing config.mak.uname the > same, since at least it seems stable. Just a FYI and in case you aren't aware, you could create a "config.mak" to store your custom configurations. You can be sure it's used due to the following Makefile part: ... include config.mak.uname -include config.mak.autogen -include config.mak ... It's just not a hard dependency. Hope that helps, Sivaraam ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-04 6:38 ` Duy Nguyen 2018-08-04 17:19 ` Junio C Hamano @ 2018-08-05 2:42 ` Eric Sunshine 2018-08-05 3:17 ` Jonathan Nieder 1 sibling, 1 reply; 26+ messages in thread From: Eric Sunshine @ 2018-08-05 2:42 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: Jonathan Nieder, Stefan Beller, Ævar Arnfjörð Bjarmason, Git List, git-packagers, Han-Wen Nienhuys On Sat, Aug 4, 2018 at 2:38 AM Duy Nguyen <pclouds@gmail.com> wrote: > On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder <jrnieder@gmail.com> wrote: > > My main concern is not about them but about other > > people building from source in order to run (instead of to develop) > > Git, and by extension, the people they go to for help when it doesn't > > work. I have lots of bitter experience of -Werror being a support > > headache and leading to bad workarounds when someone upgrades their > > compiler and the build starts failing due to a new warning it has > > introduced. > > Even old compilers can also throw some silly, false positive warnings > (which now turn into errors) because they are not as smart as new > ones. And, compilation warnings are not limited to old compilers. Even my fully up-to-date FreeBSD 11.2 installation is not warning-free[1]. [1]: For instance: utf8.c:486:28: warning: passing 'iconv_ibp *' (aka 'const char **') to parameter of type 'char **' discards qualifiers in nested pointer types [-Wincompatible-pointer-types-discards-qualifiers] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-05 2:42 ` Eric Sunshine @ 2018-08-05 3:17 ` Jonathan Nieder 2018-08-05 3:33 ` Eric Sunshine 0 siblings, 1 reply; 26+ messages in thread From: Jonathan Nieder @ 2018-08-05 3:17 UTC (permalink / raw) To: Eric Sunshine Cc: Nguyễn Thái Ngọc Duy, Stefan Beller, Ævar Arnfjörð Bjarmason, Git List, git-packagers, Han-Wen Nienhuys Eric Sunshine wrote: > And, compilation warnings are not limited to old compilers. Even my > fully up-to-date FreeBSD 11.2 installation is not warning-free[1]. > > [1]: For instance: > utf8.c:486:28: warning: passing 'iconv_ibp *' (aka 'const char **') to parameter > of type 'char **' discards qualifiers in nested pointer types > [-Wincompatible-pointer-types-discards-qualifiers] Oh, good catch! POSIX documents iconv has having signature size_t iconv(iconv_t cd, char **restrict inbuf, size_t *restrict inbytesleft, char **restrict outbuf, size_t *restrict outbytesleft); The Makefile explains # Define OLD_ICONV if your library has an old iconv(), where the second # (input buffer pointer) parameter is declared with type (const char **). which is implemented as #if defined(OLD_ICONV) || (defined(__sun__) && !defined(_XPG6)) typedef const char * iconv_ibp; #else typedef char * iconv_ibp; #endif config.mak.uname contains ifeq ($(uname_S),FreeBSD) NEEDS_LIBICONV = YesPlease OLD_ICONV = YesPlease So it looks like FreeBSD has modernized and we need to make that conditional in config.mak.uname on $(uname_R). Do you know which version of FreeBSD changed the signature? Care to write a patch? Thanks, Jonathan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-05 3:17 ` Jonathan Nieder @ 2018-08-05 3:33 ` Eric Sunshine 2018-08-05 4:58 ` Eric Sunshine 2018-08-05 7:57 ` Jonathan Nieder 0 siblings, 2 replies; 26+ messages in thread From: Eric Sunshine @ 2018-08-05 3:33 UTC (permalink / raw) To: Jonathan Nieder Cc: Nguyễn Thái Ngọc Duy, Stefan Beller, Ævar Arnfjörð Bjarmason, Git List, git-packagers, Han-Wen Nienhuys On Sat, Aug 4, 2018 at 11:17 PM Jonathan Nieder <jrnieder@gmail.com> wrote: > > utf8.c:486:28: warning: passing 'iconv_ibp *' (aka 'const char **') to parameter > > of type 'char **' discards qualifiers in nested pointer types > > [-Wincompatible-pointer-types-discards-qualifiers] > > Oh, good catch! POSIX documents iconv has having signature > > size_t iconv(iconv_t cd, char **restrict inbuf, > size_t *restrict inbytesleft, char **restrict outbuf, > size_t *restrict outbytesleft); > > config.mak.uname contains > > ifeq ($(uname_S),FreeBSD) > NEEDS_LIBICONV = YesPlease > OLD_ICONV = YesPlease > > So it looks like FreeBSD has modernized and we need to make that > conditional in config.mak.uname on $(uname_R). Do you know which > version of FreeBSD changed the signature? Care to write a patch? Unfortunately, I don't know in which version of FreeBSD that changed. I rarely fire up that virtual machine (only in rare cases when I want to verify some change to Git also builds/runs/whatever on FreeBSD), so I haven't really been paying attention to it. I know that this warning was present in 11.1 (and I'm guessing all of 11.x), but I don't recall if it manifested in 10.x. I guess it shouldn't be too hard to install various versions of FreeBSD to determine this, but it would be quite time-consuming. I'm not very familiar with FreeBSD-land, but I would hope there would be an easier way to determine when it changed than by installing old versions. Does FreeBSD have historic package repositories (containing headers, for instance) which one could consult instead? ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-05 3:33 ` Eric Sunshine @ 2018-08-05 4:58 ` Eric Sunshine 2018-08-05 7:57 ` Jonathan Nieder 1 sibling, 0 replies; 26+ messages in thread From: Eric Sunshine @ 2018-08-05 4:58 UTC (permalink / raw) To: Jonathan Nieder Cc: Nguyễn Thái Ngọc Duy, Stefan Beller, Ævar Arnfjörð Bjarmason, Git List, git-packagers, Han-Wen Nienhuys On Sat, Aug 4, 2018 at 11:33 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > On Sat, Aug 4, 2018 at 11:17 PM Jonathan Nieder <jrnieder@gmail.com> wrote: > > So it looks like FreeBSD has modernized and we need to make that > > conditional in config.mak.uname on $(uname_R). Do you know which > > version of FreeBSD changed the signature? Care to write a patch? > > I'm not very familiar with FreeBSD-land, but I would > hope there would be an easier way to determine when it changed than by > installing old versions. Does FreeBSD have historic package > repositories (containing headers, for instance) which one could > consult instead? I thought perhaps we could figure out the timeframe by looking at the Git package[1] in the FreeBSD ports tree to see when they added, removed, or changed a patch file for config.mak.uname, but that didn't pan out since they invoke the Git 'configure' script which determines OLD_ICONV automatically. [1]: https://github.com/freebsd/freebsd-ports/tree/master/devel/git ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] Makefile: enable DEVELOPER by default 2018-08-05 3:33 ` Eric Sunshine 2018-08-05 4:58 ` Eric Sunshine @ 2018-08-05 7:57 ` Jonathan Nieder 2018-08-31 8:33 ` [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning Eric Sunshine 1 sibling, 1 reply; 26+ messages in thread From: Jonathan Nieder @ 2018-08-05 7:57 UTC (permalink / raw) To: Eric Sunshine Cc: Nguyễn Thái Ngọc Duy, Stefan Beller, Ævar Arnfjörð Bjarmason, Git List, git-packagers, Han-Wen Nienhuys Hi, Eric Sunshine wrote: > On Sat, Aug 4, 2018 at 11:17 PM Jonathan Nieder <jrnieder@gmail.com> wrote: >>> utf8.c:486:28: warning: passing 'iconv_ibp *' (aka 'const char **') to parameter >>> of type 'char **' discards qualifiers in nested pointer types >>> [-Wincompatible-pointer-types-discards-qualifiers] >> >> Oh, good catch! POSIX documents iconv has having signature >> >> size_t iconv(iconv_t cd, char **restrict inbuf, >> size_t *restrict inbytesleft, char **restrict outbuf, >> size_t *restrict outbytesleft); >> >> config.mak.uname contains >> >> ifeq ($(uname_S),FreeBSD) >> NEEDS_LIBICONV = YesPlease >> OLD_ICONV = YesPlease >> >> So it looks like FreeBSD has modernized and we need to make that >> conditional in config.mak.uname on $(uname_R). Do you know which >> version of FreeBSD changed the signature? Care to write a patch? > > Unfortunately, I don't know in which version of FreeBSD that changed. "git blame" tells me it's from r281550[1] (Remove the const qualifier from iconv(3) to comply with POSIX, 2015-04-15), which was part of FreeBSD 11. r282275[2] (2015-04-30) backported the same change to FreeBSD 10 and is part of 10.2. FreeBSD 9 has #define iconv(cd, in, insize, out, outsize) libiconv(cd, __DECONST(char **, in), insize, out, outsize) from r219019[3] (2011-02-25). I don't know what to make of it. The underlying libiconv function it calls takes char ** (the modern thing) but the macro does the __DECONST thing for compatibility with GNU libiconv. Older versions, going back at least as far as FreeBSD 3, behave the same way as FreeBSD 9. So that must be what was tested with OLD_ICONV=YesPlease. FreeBSD 10.1.0 and 10.0.0 have size_t iconv(iconv_t, const char ** __restrict, size_t * __restrict, char ** __restrict, size_t * __restrict); which also needs OLD_ICONV. If I assume everyone on 10.x is using 10.2 or newer, then the patch could be something like this (completely untested): diff --git i/config.mak.uname w/config.mak.uname index 684fc5bf026..8078c099313 100644 --- i/config.mak.uname +++ w/config.mak.uname @@ -192,7 +192,9 @@ ifeq ($(uname_O),Cygwin) endif ifeq ($(uname_S),FreeBSD) NEEDS_LIBICONV = YesPlease - OLD_ICONV = YesPlease + ifeq ($(shell expr "$(uname_R)" : '[1-9]\.'),2) + OLD_ICONV = YesPlease + endif NO_MEMMEM = YesPlease BASIC_CFLAGS += -I/usr/local/include BASIC_LDFLAGS += -L/usr/local/lib Deciding whether to fix the pattern matching to also include 10.0 and 10.1 (and doing so if warranted) is left as an exercise to the reader. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> [1] https://github.com/freebsd/freebsd/commit/b0813ee288f64f677a2cebf7815754b027a8215b [2] https://github.com/freebsd/freebsd/commit/b709ec868adb5170d09bc5a66b18d0e0d5987ab6 [3] https://github.com/freebsd/freebsd/commit/c91ab1769b1237e3663d59888cebe31ceee47570 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning 2018-08-05 7:57 ` Jonathan Nieder @ 2018-08-31 8:33 ` Eric Sunshine 2018-08-31 11:54 ` Ævar Arnfjörð Bjarmason 2018-08-31 17:00 ` Jonathan Nieder 0 siblings, 2 replies; 26+ messages in thread From: Eric Sunshine @ 2018-08-31 8:33 UTC (permalink / raw) To: git; +Cc: Jonathan Nieder, Eric Sunshine From: Jonathan Nieder <jrnieder@gmail.com> OLD_ICONV has long been needed by FreeBSD so config.mak.uname defines it unconditionally. However, recent versions do not need it, and its presence results in compilation warnings. Resolve this issue by defining OLD_ICONV only for older FreeBSD versions. Specifically, revision r281550[1], which is part of FreeBSD 11, removed the need for OLD_ICONV, and r282275[2] back-ported that change to 10.2. Versions prior to 10.2 do need it. [1] https://github.com/freebsd/freebsd/commit/b0813ee288f64f677a2cebf7815754b027a8215b [2] https://github.com/freebsd/freebsd/commit/b709ec868adb5170d09bc5a66b18d0e0d5987ab6 [es: commit message; tweak version check to distinguish 10.x versions] Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- This is a follow-up to [1] which encapsulates Jonathan's proposed change as a proper patch. I made Jonathan as the author since he did all the hard research and formulated the core of the change (whereas I only reported the issue and extended the version check to correctly handle FreeBSD 10.0 and 10.1). Jonathan's sign-off comes from [1]. [1]: https://public-inbox.org/git/20180805075736.GF44140@aiede.svl.corp.google.com/ config.mak.uname | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/config.mak.uname b/config.mak.uname index 2be2f19811..e47af72e01 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -192,7 +192,17 @@ ifeq ($(uname_O),Cygwin) endif ifeq ($(uname_S),FreeBSD) NEEDS_LIBICONV = YesPlease - OLD_ICONV = YesPlease + # Versions up to 10.1 require OLD_ICONV; 10.2 and beyond don't. + # A typical version string looks like "10.2-RELEASE". + ifeq ($(shell expr "$(uname_R)" : '[1-9]\.'),2) + OLD_ICONV = YesPlease + endif + ifeq ($(firstword $(subst -, ,$(uname_R))),10.0) + OLD_ICONV = YesPlease + endif + ifeq ($(firstword $(subst -, ,$(uname_R))),10.1) + OLD_ICONV = YesPlease + endif NO_MEMMEM = YesPlease BASIC_CFLAGS += -I/usr/local/include BASIC_LDFLAGS += -L/usr/local/lib -- 2.19.0.rc1.352.gb1634b371d ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning 2018-08-31 8:33 ` [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning Eric Sunshine @ 2018-08-31 11:54 ` Ævar Arnfjörð Bjarmason 2018-08-31 18:31 ` Eric Sunshine 2018-08-31 17:00 ` Jonathan Nieder 1 sibling, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-08-31 11:54 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git Mailing List, Jonathan Nieder, Renato Botelho On Fri, Aug 31, 2018 at 11:52 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > OLD_ICONV has long been needed by FreeBSD so config.mak.uname defines > it unconditionally. However, recent versions do not need it, and its > presence results in compilation warnings. Resolve this issue by defining > OLD_ICONV only for older FreeBSD versions. This seems sane, but just for context is FreeBSD ports itself just compiling without iconv entirely? [CC FreeBSD devel/git maintainer] $ uname -r; grep -ri iconv /usr/ports/devel/git 11.2-RELEASE-p2 /usr/ports/devel/git/Makefile:OPTIONS_DEFINE= GUI SVN GITWEB CONTRIB P4 CVS HTMLDOCS PERL ICONV CURL \ /usr/ports/devel/git/Makefile:OPTIONS_DEFAULT= CONTRIB P4 CVS PERL GITWEB ICONV CURL SEND_EMAIL PCRE \ /usr/ports/devel/git/Makefile:ICONV_USES= iconv /usr/ports/devel/git/Makefile:ICONV_MAKE_ARGS_OFF= NO_ICONV=1 I have little clue about how ports works, but just noticed that they're not monkeypatching in OLD_ICONV there. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning 2018-08-31 11:54 ` Ævar Arnfjörð Bjarmason @ 2018-08-31 18:31 ` Eric Sunshine 0 siblings, 0 replies; 26+ messages in thread From: Eric Sunshine @ 2018-08-31 18:31 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Git List, Jonathan Nieder, Renato Botelho On Fri, Aug 31, 2018 at 7:54 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Fri, Aug 31, 2018 at 11:52 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > OLD_ICONV has long been needed by FreeBSD so config.mak.uname defines > > it unconditionally. However, recent versions do not need it, and its > > presence results in compilation warnings. Resolve this issue by defining > > OLD_ICONV only for older FreeBSD versions. > > This seems sane, but just for context is FreeBSD ports itself just > compiling without iconv entirely? > > I have little clue about how ports works, but just noticed that > they're not monkeypatching in OLD_ICONV there. My experience with FreeBSD ports is pretty limited too, but, as I discovered in [1], they run Git's configure script, so OLD_ICONV is determined dynamically, as far as I can tell. [1]: https://public-inbox.org/git/CAPig+cTEGtsmUyoYsKEx+erMsXKm5=c6TJuNAgeky2pcgw18JQ@mail.gmail.com/ ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning 2018-08-31 8:33 ` [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning Eric Sunshine 2018-08-31 11:54 ` Ævar Arnfjörð Bjarmason @ 2018-08-31 17:00 ` Jonathan Nieder 2018-08-31 20:59 ` Eric Sunshine 1 sibling, 1 reply; 26+ messages in thread From: Jonathan Nieder @ 2018-08-31 17:00 UTC (permalink / raw) To: Eric Sunshine; +Cc: git Eric Sunshine wrote: > From: Jonathan Nieder <jrnieder@gmail.com> > > OLD_ICONV has long been needed by FreeBSD so config.mak.uname defines > it unconditionally. However, recent versions do not need it, and its > presence results in compilation warnings. Resolve this issue by defining > OLD_ICONV only for older FreeBSD versions. > > Specifically, revision r281550[1], which is part of FreeBSD 11, removed > the need for OLD_ICONV, and r282275[2] back-ported that change to 10.2. > Versions prior to 10.2 do need it. > > [1] https://github.com/freebsd/freebsd/commit/b0813ee288f64f677a2cebf7815754b027a8215b > [2] https://github.com/freebsd/freebsd/commit/b709ec868adb5170d09bc5a66b18d0e0d5987ab6 > > [es: commit message; tweak version check to distinguish 10.x versions] > > Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> > --- > config.mak.uname | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) I think it makes sense for you to take credit for this one. You noticed the original problem, tested on FreeBSD, wrote the explanation, and figured out the firstword hackery. All I did was to say "somebody should fix this" and run "git log -S" a few times. In any event, Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning 2018-08-31 17:00 ` Jonathan Nieder @ 2018-08-31 20:59 ` Eric Sunshine 0 siblings, 0 replies; 26+ messages in thread From: Eric Sunshine @ 2018-08-31 20:59 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Git List On Fri, Aug 31, 2018 at 1:00 PM Jonathan Nieder <jrnieder@gmail.com> wrote: > > From: Jonathan Nieder <jrnieder@gmail.com> > > OLD_ICONV has long been needed by FreeBSD so config.mak.uname defines > > it unconditionally. However, recent versions do not need it, and its > > presence results in compilation warnings. Resolve this issue by defining > > OLD_ICONV only for older FreeBSD versions. > > I think it makes sense for you to take credit for this one. You > noticed the original problem, tested on FreeBSD, wrote the > explanation, and figured out the firstword hackery. All I did was to > say "somebody should fix this" and run "git log -S" a few times. [...] I'm fine going either way with authorship, and I can re-roll with that minor change (if Junio isn't interested in tweaking it while queueing). ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2018-09-01 21:06 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-04 2:00 [PATCH] Makefile: enable DEVELOPER by default Stefan Beller 2018-08-04 2:02 ` Stefan Beller 2018-08-04 6:09 ` Jonathan Nieder 2018-08-04 6:38 ` Duy Nguyen 2018-08-04 17:19 ` Junio C Hamano 2018-08-06 16:40 ` Ævar Arnfjörð Bjarmason 2018-08-06 17:02 ` Jeff King 2018-08-06 17:04 ` Randall S. Becker 2018-08-06 17:11 ` Jonathan Nieder 2018-08-06 18:59 ` Jeff King 2018-08-06 17:39 ` Ævar Arnfjörð Bjarmason 2018-08-06 18:38 ` Stefan Beller 2018-08-06 17:02 ` Randall S. Becker 2018-08-06 17:41 ` Ævar Arnfjörð Bjarmason 2018-08-06 19:20 ` Randall S. Becker 2018-09-01 21:01 ` Kaartic Sivaraam 2018-08-05 2:42 ` Eric Sunshine 2018-08-05 3:17 ` Jonathan Nieder 2018-08-05 3:33 ` Eric Sunshine 2018-08-05 4:58 ` Eric Sunshine 2018-08-05 7:57 ` Jonathan Nieder 2018-08-31 8:33 ` [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning Eric Sunshine 2018-08-31 11:54 ` Ævar Arnfjörð Bjarmason 2018-08-31 18:31 ` Eric Sunshine 2018-08-31 17:00 ` Jonathan Nieder 2018-08-31 20:59 ` Eric Sunshine
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).