threads / patch / 49031

patchMakefile: enable DEVELOPER by default

Subject: [PATCH] Makefile: enable DEVELOPER by default

## tl;dr

26 messages between Aug 4, 2018 and Sep 1, 2018. Diffs are folded; open one to read it.

replies: 25people: 9as markdown or json

Stefan Beller· Aug 4, 2018, 02:00 UTC · lore

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(+)
Show changes to Makefile +2 −0
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
Stefan Beller· Aug 4, 2018, 02:02 UTC · re: Stefan Beller · lore

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(+)
Show changes to Makefile +2 −0
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
Jonathan Nieder· Aug 4, 2018, 06:09 UTC · re: Stefan Beller · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

Hi,
Stefan Beller wrote:
Show 7 quoted lines
> 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.

Show 14 quoted lines
> 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

Duy Nguyen· Aug 4, 2018, 06:38 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 7 quoted lines
> 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
Junio C Hamano· Aug 4, 2018, 17:19 UTC · re: Duy Nguyen · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

Duy Nguyen <pclouds@gmail.com> writes:
Show 12 quoted lines
> 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.

Ævar Arnfjörð Bjarmason· Aug 6, 2018, 16:40 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

On Sat, Aug 04 2018, Junio C Hamano wrote:
Show 20 quoted lines
> 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.

Jeff King· Aug 6, 2018, 17:02 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

On Mon, Aug 06, 2018 at 06:40:14PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 12 quoted lines
> 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.
Randall S. Becker· Aug 6, 2018, 17:04 UTC · re: Jeff King · lore

RE: [PATCH] Makefile: enable DEVELOPER by default

On August 6, 2018 1:02 PM, Peff wrote:
Show 40 quoted lines
> 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.
Jonathan Nieder· Aug 6, 2018, 17:11 UTC · re: Jeff King · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

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.
[...]
Show 11 quoted lines
> 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

Jeff King· Aug 6, 2018, 18:59 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

On Mon, Aug 06, 2018 at 10:11:19AM -0700, Jonathan Nieder wrote:
Show 21 quoted lines
> > 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
Ævar Arnfjörð Bjarmason· Aug 6, 2018, 17:39 UTC · re: Jeff King · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

On Mon, Aug 06 2018, Jeff King wrote:
Show 19 quoted lines
> 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.

Show 5 quoted lines
> 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.
Stefan Beller· Aug 6, 2018, 18:38 UTC · re: Jeff King · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

Show 10 quoted lines
> 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
Randall S. Becker· Aug 6, 2018, 17:02 UTC · re: Ævar Arnfjörð Bjarmason · lore

RE: [PATCH] Makefile: enable DEVELOPER by default

On August 6, 2018 12:40 PM, Ævar Arnfjörð Bjarmason wrote:
Show 36 quoted lines
> 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.
Ævar Arnfjörð Bjarmason· Aug 6, 2018, 17:41 UTC · re: Randall S. Becker · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

On Mon, Aug 06 2018, Randall S. Becker wrote:
Show 40 quoted lines
> 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.

Randall S. Becker· Aug 6, 2018, 19:20 UTC · re: Ævar Arnfjörð Bjarmason · lore

RE: [PATCH] Makefile: enable DEVELOPER by default

On August 6, 2018 1:42 PM, Ævar Arnfjörð Bjarmason wrote:
Show 53 quoted lines
> 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.
Kaartic Sivaraam· Sep 1, 2018, 21:01 UTC · re: Randall S. Becker · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

On Mon, 2018-08-06 at 13:02 -0400, Randall S. Becker wrote:
Show 10 quoted lines
> 
> 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

Eric Sunshine· Aug 5, 2018, 02:42 UTC · re: Duy Nguyen · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

On Sat, Aug 4, 2018 at 2:38 AM Duy Nguyen <pclouds@gmail.com> wrote:
Show 12 quoted lines
> 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]
Jonathan Nieder· Aug 5, 2018, 03:17 UTC · re: Eric Sunshine · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

Eric Sunshine wrote:
Show 7 quoted lines
> 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

Eric Sunshine· Aug 5, 2018, 03:33 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

On Sat, Aug 4, 2018 at 11:17 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 19 quoted lines
> > 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?

Eric Sunshine· Aug 5, 2018, 04:58 UTC · re: Eric Sunshine · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

On Sat, Aug 4, 2018 at 11:33 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 10 quoted lines
> 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
Jonathan Nieder· Aug 5, 2018, 07:57 UTC · re: Eric Sunshine · lore

Re: [PATCH] Makefile: enable DEVELOPER by default

Hi,
Eric Sunshine wrote:
> On Sat, Aug 4, 2018 at 11:17 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 21 quoted lines
>>> 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):

Show changes to diff +3 −1
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
Eric Sunshine· Aug 31, 2018, 08:33 UTC · re: Jonathan Nieder · lore

[PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning

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(-)
Show changes to config.mak.uname +11 −1
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
Ævar Arnfjörð Bjarmason· Aug 31, 2018, 11:54 UTC · re: Eric Sunshine · lore

Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning

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.

Eric Sunshine· Aug 31, 2018, 18:31 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning

On Fri, Aug 31, 2018 at 7:54 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 11 quoted lines
> 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/
Jonathan Nieder· Aug 31, 2018, 17:00 UTC · re: Eric Sunshine · lore

Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning

Eric Sunshine wrote:
Show 21 quoted lines
> 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>
Eric Sunshine· Aug 31, 2018, 20:59 UTC · re: Jonathan Nieder · lore

Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning

On Fri, Aug 31, 2018 at 1:00 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 10 quoted lines
> > 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).

← back to recent threads