[PATCH] Makefile: enable DEVELOPER by default

[PATCH] Makefile: enable DEVELOPER by default

[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

[PATCH] Makefile: enable DEVELOPER by default

[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>
---

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

Re: [PATCH] Makefile: enable DEVELOPER by default

[Jonathan Nieder]

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

Re: [PATCH] Makefile: enable DEVELOPER by default

[Duy Nguyen]

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

Re: [PATCH] Makefile: enable DEVELOPER by default

[Junio C Hamano]

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.

Re: [PATCH] Makefile: enable DEVELOPER by default

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH] Makefile: enable DEVELOPER by default

[Jeff King]

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.

RE: [PATCH] Makefile: enable DEVELOPER by default

[Randall S. Becker]

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.

Re: [PATCH] Makefile: enable DEVELOPER by default

[Jonathan Nieder]

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

Re: [PATCH] Makefile: enable DEVELOPER by default

[Jeff King]

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

Re: [PATCH] Makefile: enable DEVELOPER by default

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH] Makefile: enable DEVELOPER by default

[Stefan Beller]

> 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

RE: [PATCH] Makefile: enable DEVELOPER by default

[Randall S. Becker]

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.

Re: [PATCH] Makefile: enable DEVELOPER by default

[Ævar Arnfjörð Bjarmason]

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.

RE: [PATCH] Makefile: enable DEVELOPER by default

[Randall S. Becker]

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.

Re: [PATCH] Makefile: enable DEVELOPER by default

[Kaartic Sivaraam]

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

Re: [PATCH] Makefile: enable DEVELOPER by default

[Eric Sunshine]

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]

Re: [PATCH] Makefile: enable DEVELOPER by default

[Jonathan Nieder]

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

Re: [PATCH] Makefile: enable DEVELOPER by default

[Eric Sunshine]

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?

Re: [PATCH] Makefile: enable DEVELOPER by default

[Eric Sunshine]

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

Re: [PATCH] Makefile: enable DEVELOPER by default

[Jonathan Nieder]

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

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

[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

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

[Ævar Arnfjörð Bjarmason]

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.

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

[Eric Sunshine]

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/

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

[Jonathan Nieder]

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>

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

[Eric Sunshine]

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).