Makefile: enable DEVELOPER by default
PATCH 1/1 Makefile: enable DEVELOPER by default
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)
--- Makefile | 2 ++ 1 file changed, 2 insertions(+)
Makefile +2 -0
@@ -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
Signature · 1 line
2.18.0.597.ga71716f1ad-goog
PATCH 1/1 Makefile: enable DEVELOPER by default
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)
---
Resending with <[email protected]> cc'd.
Makefile | 2 ++ 1 file changed, 2 insertions(+)
Makefile +2 -0
@@ -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
Signature · 1 line
2.18.0.597.ga71716f1ad-goog
Hi,
Quoted 7 lines · 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)
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.
Quoted 10 lines
Signed-off-by Stefan Beller <[email protected]>--- Makefile | 2 ++ 1 file changed, 2 insertions(+)Makefile +0
-0
Makefile ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) · line 500
Quoted 4 lines
Makefile +1
-0@@ -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
Quoted 7 lines · Jonathan Nieder
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.
Signature · 1 line
Duy
Quoted 9 lines · Duy Nguyen
Quoted 7 lines · Jonathan Nieder
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.
Quoted 3 lines
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.
Quoted 15 lines · Junio C Hamano
Quoted 9 lines · Duy Nguyen
Quoted 7 lines · Jonathan Nieder
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.Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 8 lines · Ævar Arnfjörð Bjarmason
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.
Quoted 4 lines
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.
Quoted 34 lines · Peff
To: Ævar Arnfjörð Bjarmason <[email protected]>
Cc: Junio C Hamano <[email protected]>; Duy Nguyen <[email protected]>; Jonathan Nieder <[email protected]>; Stefan Beller <[email protected]>; Git Mailing List <[email protected]>; git- [email protected]; Han-Wen Nienhuys <[email protected]>
Subject: Re: [PATCH]Makefile: enable DEVELOPER by defaultQuoted 8 lines · Ævar Arnfjörð Bjarmason
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.Quoted 4 lines
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.
Quoted 6 lines
-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.
Hi,
Quoted 3 lines · Jeff King
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.
[...]
Quoted 11 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
Quoted 17 lines · Jonathan Nieder
Quoted 11 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.
Quoted 4 lines
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
Quoted 15 lines · Jeff King
Quoted 8 lines · Ævar Arnfjörð Bjarmason
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.Quoted 4 lines
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.
Quoted 4 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 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.
Quoted 5 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.
Quoted 10 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
Quoted 28 lines · Ævar Arnfjörð Bjarmason
Quoted 3 lines · Junio C Hamano
Quoted 1 line · Duy Nguyen
On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder <[email protected]>
Quoted 11 lines · wrote:
Quoted 10 lines
Quoted 6 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.
Quoted 5 lines
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?
Quoted 8 lines
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.
Quoted 34 lines · Randall S. Becker
Quoted 33 lines · Ævar Arnfjörð Bjarmason
Quoted 3 lines · Junio C Hamano
Quoted 1 line · Duy Nguyen
On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder <[email protected]>
Quoted 11 lines · wrote:
Quoted 10 lines
Quoted 6 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.
Quoted 5 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 46 lines · Ævar Arnfjörð Bjarmason
Quoted 7 lines · Randall S. Becker
Quoted 6 lines · Ævar Arnfjörð Bjarmason
Quoted 4 lines · Junio C Hamano
Quoted 2 lines · Duy Nguyen
On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder
<[email protected]>Quoted 7 lines
Quoted 7 lines
Quoted 6 lines · wrote:
Quoted 6 lines
Quoted 6 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.
Quoted 16 lines
Quoted 16 lines
Quoted 10 lines
Quoted 4 lines
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?
Quoted 8 lines
Quoted 8 lines
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.
Quoted 3 lines
I am the platform maintainer for HPE NonStop and need to make sure I'm not packaging DEV builds to anyone
Quoted 7 lines
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.
Quoted 10 lines · Randall S. Becker
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
Quoted 9 lines · Duy Nguyen
Quoted 7 lines · Jonathan Nieder
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.
Quoted 3 lines
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]
Quoted 7 lines · Eric Sunshine
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
Quoted 12 lines · Jonathan Nieder
Quoted 3 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_ticonv(iconv_t cd, char **restrict inbuf,
size_t*restrict inbytesleft, char **restrict outbuf,size_t*restrict outbytesleft);config.mak.uname contains
Quoted 7 lines
ifeq ($(uname_S),FreeBSD)
NEEDS_LIBICONV= YesPlease
OLD_ICONV= YesPleaseSo 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?
Quoted 10 lines · Eric Sunshine
Quoted 3 lines · Jonathan Nieder
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
Hi,
Quoted 18 lines · Eric Sunshine
Quoted 16 lines · Jonathan Nieder
Quoted 3 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_ticonv(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
Quoted 5 lines
Quoted 3 lines
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):
config.mak.uname +3 -1
@@ -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.
[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 1/1 config.mak.uname: resolve FreeBSD iconv-related compilation warning
From: Jonathan Nieder <[email protected]>
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]
---
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/[email protected]/
config.mak.uname | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
config.mak.uname +11 -1
@@ -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
Signature · 1 line
2.19.0.rc1.352.gb1634b371d
Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning
Quoted 4 lines · Eric Sunshine
OLD_ICONVhas 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 definingOLD_ICONVonly 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
Quoted 11 lines · Ævar Arnfjörð Bjarmason
Quoted 4 lines · Eric Sunshine
OLD_ICONVhas 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 definingOLD_ICONVonly 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_ICONVthere.
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
In reply to Eric SunshineQuoted 14 lines · Eric Sunshine
From: Jonathan Nieder <[email protected]>
OLD_ICONVhas 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 definingOLD_ICONVonly 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
Quoted 7 lines
[es: commit message; tweak version check to distinguish 10.x versions]
Signed-off-by Jonathan Nieder <[email protected]>Signed-off-by Eric Sunshine <[email protected]>--- 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,
Re: [PATCH] config.mak.uname: resolve FreeBSD iconv-related compilation warning
Quoted 10 lines · Jonathan Nieder
Quoted 5 lines
From: Jonathan Nieder <[email protected]>
OLD_ICONVhas 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 definingOLD_ICONVonly 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).