[PATCH] Silence iconv warnings on Leopard

[PATCH] Silence iconv warnings on Leopard

[Wincent Colaiuta]

Apple ships a newer version of iconv with Leopard (Mac OS X 10.5/Darwin
9). Ensure that OLD_ICONV is not set on any version of Darwin in the
9.x series; this should be good for at least a couple of years, when
Darwin 10 comes out and we can invert the sense of the test to
specifically check for Darwin 7 or 8.

A more sophisticated and robust check is possible for those who use
autoconf, but not everybody does that.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 Makefile |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 999391e..4dda340 100644
--- a/Makefile
+++ b/Makefile
@@ -406,7 +406,9 @@ endif
 ifeq ($(uname_S),Darwin)
 	NEEDS_SSL_WITH_CRYPTO = YesPlease
 	NEEDS_LIBICONV = YesPlease
-	OLD_ICONV = UnfortunatelyYes
+	ifneq ($(shell expr "$(uname_R)" : '9\.'),2)
+		OLD_ICONV = UnfortunatelyYes
+	endif
 	NO_STRLCPY = YesPlease
 	NO_MEMMEM = YesPlease
 endif
-- 
1.5.3.7.1067.gaa51

Re: [PATCH] Silence iconv warnings on Leopard

[Blake Ramsdell]

On Dec 6, 2007 11:07 AM, Wincent Colaiuta <win@wincent.com> wrote:
> Apple ships a newer version of iconv with Leopard (Mac OS X 10.5/Darwin
> 9). Ensure that OLD_ICONV is not set on any version of Darwin in the
> 9.x series; this should be good for at least a couple of years, when
> Darwin 10 comes out and we can invert the sense of the test to
> specifically check for Darwin 7 or 8.

This approach seems fine to me, there was some concern about matching
the OS type / version in the past, but I haven't really seen a better
answer.

> A more sophisticated and robust check is possible for those who use
> autoconf, but not everybody does that.

I did make a patch for configure.ac that does this. If it's
interesting, I'll send it along.

Blake
-- 
Blake Ramsdell | http://www.blakeramsdell.com

Re: [PATCH] Silence iconv warnings on Leopard

[Jakub Narebski]

On Fri, 7 Dec 2007, Blake Ramsdell wrote:
> On Dec 6, 2007 11:07 AM, Wincent Colaiuta <win@wincent.com> wrote:

>> Apple ships a newer version of iconv with Leopard (Mac OS X 10.5/Darwin
>> 9). Ensure that OLD_ICONV is not set on any version of Darwin in the
>> 9.x series; this should be good for at least a couple of years, when
>> Darwin 10 comes out and we can invert the sense of the test to
>> specifically check for Darwin 7 or 8.

>> A more sophisticated and robust check is possible for those who use
>> autoconf, but not everybody does that.
> 
> I did make a patch for configure.ac that does this. If it's
> interesting, I'll send it along.

I would be interested, as I tried to make a patch to configure.ac which
does that. The problem is that it should be check that tests for compile
time _warnings_; my solution was to use '-Werror' flag to make warning
into error, and AC_COMPILE_IFELSE, but this might be gcc only solution.

  Message-ID: <1196895948-25115-1-git-send-email-jnareb@gmail.com>
  http://permalink.gmane.org/gmane.comp.version-control.git/67209

So please send it.

Thanks in advance
-- 
Jakub Narebski
Poland

Re: [PATCH] Silence iconv warnings on Leopard

[Blake Ramsdell]

On Dec 6, 2007 4:11 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> I would be interested, as I tried to make a patch to configure.ac which
> does that. The problem is that it should be check that tests for compile
> time _warnings_; my solution was to use '-Werror' flag to make warning
> into error, and AC_COMPILE_IFELSE, but this might be gcc only solution.

That's precisely what I did. If this isn't suitable, then bummer --
impossible ;).

Blake
-- 
Blake Ramsdell | http://www.blakeramsdell.com

Re: [PATCH] Silence iconv warnings on Leopard

[Linus Torvalds]

On Fri, 7 Dec 2007, Jakub Narebski wrote:
>
> The problem is that it should be check that tests for compile time 
> _warnings_; my solution was to use '-Werror' flag to make warning into 
> error, and AC_COMPILE_IFELSE, but this might be gcc only solution.
> 
>   Message-ID: <1196895948-25115-1-git-send-email-jnareb@gmail.com>
>   http://permalink.gmane.org/gmane.comp.version-control.git/67209
> 
> So please send it.

Umm. Why not just make the test be whether the following compiles cleanly?

	#include <iconv.h>
	
	extern size_t iconv(iconv_t cd,
	  char **inbuf, size_t *inbytesleft,
	  char **outbuf, size_t *outbytesleft);

because if the compiler has seen a "const char **inbuf", then it  should 
error out with a "conflicting types for ‘iconv’" style message..

Just do

	 $CC -c test-iconv.c

or something.

Totally untested. I don't do autoconf.

		Linus

Re: [PATCH] Silence iconv warnings on Leopard

[Blake Ramsdell]

On Dec 6, 2007 4:30 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> Umm. Why not just make the test be whether the following compiles cleanly?
>
>         #include <iconv.h>
>
>         extern size_t iconv(iconv_t cd,
>           char **inbuf, size_t *inbytesleft,
>           char **outbuf, size_t *outbytesleft);
>
> because if the compiler has seen a "const char **inbuf", then it  should
> error out with a "conflicting types for 'iconv'" style message..

Yeah, this is what I did:

diff --git a/configure.ac b/configure.ac
index 5f8a15b..675d3e0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -182,6 +182,29 @@ AC_SUBST(NEEDS_LIBICONV)
 AC_SUBST(NO_ICONV)
 test -n "$NEEDS_LIBICONV" && LIBS="$LIBS -liconv"
 #
+# Define OLD_ICONV if the iconv function prototype uses const** (Darwin and
+# some FreeBSD installations).
+AC_DEFUN([OLDICONVTEST_SRC], [
+#include <iconv.h>
+
+int main(void)
+{
+    char* value = "test";
+
+    (void) iconv (NULL, &value, NULL, NULL, NULL);
+}
+])
+AC_MSG_CHECKING([for old iconv])
+old_CFLAGS="$CFLAGS"
+CFLAGS="$CFLAGS -Werror"
+AC_COMPILE_IFELSE(OLDICONVTEST_SRC,
+	[AC_MSG_RESULT([no])
+	OLD_ICONV=],
+	[AC_MSG_RESULT([yes])
+	OLD_ICONV=UnfortunatelyYes])
+CFLAGS="$old_CFLAGS"
+AC_SUBST(OLD_ICONV)
+#
 # Define NO_DEFLATE_BOUND if deflateBound is missing from zlib.
 AC_DEFUN([ZLIBTEST_SRC], [
 #include <zlib.h>

The problem is that AC_COMPILE_IFELSE doesn't barf on warnings, so I
had to put in the CFLAGS hack to do -Werror (this is what Jakub did
also, I think).

So if this isn't rude to use -Werror (which is probably gcc-specific
in one or more ways), then fine. If it is rude to use -Werror, then
yeah, there needs to be some check for the warning, which I confess in
my five minutes of learning autoconf I don't understand well enough to
say if it's possible.

Blake
-- 
Blake Ramsdell | http://www.blakeramsdell.com

Re: [PATCH] Silence iconv warnings on Leopard

[Blake Ramsdell]

On Dec 6, 2007 4:41 PM, Blake Ramsdell <blaker@gmail.com> wrote:
> On Dec 6, 2007 4:30 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > Umm. Why not just make the test be whether the following compiles cleanly?
> >
> >         #include <iconv.h>
> >
> >         extern size_t iconv(iconv_t cd,
> >           char **inbuf, size_t *inbytesleft,
> >           char **outbuf, size_t *outbytesleft);
> >
> > because if the compiler has seen a "const char **inbuf", then it  should
> > error out with a "conflicting types for 'iconv'" style message..
>
> Yeah, this is what I did:

My apologies. Your suggestion is completely different, and should work
without -Werror. Let me try that.

Blake
-- 
Blake Ramsdell | http://www.blakeramsdell.com

[PATCH/RFC (take 3)] autoconf: Add test for OLD_ICONV (squelching compiler warning)

[Jakub Narebski]

Update configure.ac (and config.mak.in) to keep up with git
development by adding [compile] test whether your library has an old
iconv(), where the second (input buffer pointer) parameter is declared
with type (const char **) (OLD_ICONV).

Test-proposed-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
CC-ed to all parties (in this and previous thread).

I should probably used AC_LANG_PROGRAM like for NO_C99_FORMAT instead
of generating whole programlet^W test program by hand (I hope that for
example I haven't missed some header file which needs to be included);
I have followed example for NO_ICONV / NEEDS_LIBICONV and
NO_DEFLATE_BOUND test.  I'm also not sure if I have put this test in
the correct autoconf section, but that is probably matter of taste.

On Fri, 7 Dec 2007, Linus Torvalds wrote:
> 
> On Fri, 7 Dec 2007, Jakub Narebski wrote:
>>
>> The problem is that it should be check that tests for compile time 
>> _warnings_; my solution was to use '-Werror' flag to make warning into 
>> error, and AC_COMPILE_IFELSE, but this might be gcc only solution.
>> 
>>   Message-ID: <1196895948-25115-1-git-send-email-jnareb@gmail.com>
>>   http://permalink.gmane.org/gmane.comp.version-control.git/67209
>> 
>> So please send it.
> 
> Umm. Why not just make the test be whether the following compiles cleanly?
> 
> 	#include <iconv.h>
> 	
> 	extern size_t iconv(iconv_t cd,
> 	  char **inbuf, size_t *inbytesleft,
> 	  char **outbuf, size_t *outbytesleft);
> 
> because if the compiler has seen a "const char **inbuf", then it  should 
> error out with a "conflicting types for ?iconv?" style message..

[...]

> Totally untested. I don't do autoconf.

Thanks for a suggestion. Implemented in the patch below. It should
work correctly, but could those who have old iconv(), or used to have
old iconv() please check if works correctly for them?

By the way, perhaps the previous idea would work using AC_LANG_WERROR
instead of passing adding [temporarily] GCC `-Werror' option to
`CFLAGS', or use parts of its implementation.

 - Macro: AC_LANG_WERROR
     Normally Autoconf ignores warnings generated by the compiler,
     linker, and preprocessor.  If this macro is used, warnings will be
     treated as fatal errors instead for the current language.  This
     macro is useful when the results of configuration will be used
     where warnings are unacceptable; for instance, if parts of a
     program are built with the GCC `-Werror' option.  If the whole
     program will be built using `-Werror' it is often simpler to put
     `-Werror' in the compiler flags (`CFLAGS' etc.).


On Fri, 7 Dec 2007, Blake Ramsdell wrote:
> On Dec 6, 2007 4:41 PM, Blake Ramsdell <blaker@gmail.com> wrote:
>> On Dec 6, 2007 4:30 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>> Umm. Why not just make the test be whether the following compiles cleanly?
>>>
>>>         #include <iconv.h>
>>>
>>>         extern size_t iconv(iconv_t cd,
>>>           char **inbuf, size_t *inbytesleft,
>>>           char **outbuf, size_t *outbytesleft);
>>>
>>> because if the compiler has seen a "const char **inbuf", then it  should
>>> error out with a "conflicting types for 'iconv'" style message..
>>
>> Yeah, this is what I did:
> 
> My apologies. Your suggestion is completely different, and should work
> without -Werror. Let me try that.

Is something like the patch below what you wanted to try?

 config.mak.in |    1 +
 configure.ac  |   22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/config.mak.in b/config.mak.in
index 11d256e..7d5df9b 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -41,4 +41,5 @@ NO_STRTOUMAX=@NO_STRTOUMAX@
 NO_SETENV=@NO_SETENV@
 NO_MKDTEMP=@NO_MKDTEMP@
 NO_ICONV=@NO_ICONV@
+OLD_ICONV=@OLD_ICONV@
 NO_DEFLATE_BOUND=@NO_DEFLATE_BOUND@
diff --git a/configure.ac b/configure.ac
index 5f8a15b..86be19a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -212,6 +212,28 @@ test -n "$NEEDS_SOCKET" && LIBS="$LIBS -lsocket"
 
 
 ## Checks for header files.
+AC_MSG_NOTICE([CHECKS for header files])
+#
+# Define OLD_ICONV if your library has an old iconv(), where the second
+# (input buffer pointer) parameter is declared with type (const char **).
+AC_DEFUN([OLDICONVTEST_SRC], [[
+#include <iconv.h>
+
+extern size_t iconv(iconv_t cd,
+                    char **inbuf, size_t *inbytesleft,
+                    char **outbuf, size_t *outbytesleft);
+
+int main(void)
+{
+	return 0;
+}
+]])
+AC_MSG_CHECKING([for old iconv()])
+AC_COMPILE_IFELSE(OLDICONVTEST_SRC,
+	[AC_MSG_RESULT([no])],
+	[AC_MSG_RESULT([yes])
+	OLD_ICONV=UnfortunatelyYes])
+AC_SUBST(OLD_ICONV)
 
 
 ## Checks for typedefs, structures, and compiler characteristics.
-- 
1.5.3.7

Re: [PATCH/RFC (take 3)] autoconf: Add test for OLD_ICONV (squelching compiler warning)

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> On Fri, 7 Dec 2007, Blake Ramsdell wrote:
>> On Dec 6, 2007 4:41 PM, Blake Ramsdell <blaker@gmail.com> wrote:
>>> On Dec 6, 2007 4:30 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>>> Umm. Why not just make the test be whether the following compiles cleanly?
>>>>
>>>>         #include <iconv.h>
>>>>
>>>>         extern size_t iconv(iconv_t cd,
>>>>           char **inbuf, size_t *inbytesleft,
>>>>           char **outbuf, size_t *outbytesleft);
>>>>
>>>> because if the compiler has seen a "const char **inbuf", then it  should
>>>> error out with a "conflicting types for 'iconv'" style message..
>>>
>>> Yeah, this is what I did:
>> 
>> My apologies. Your suggestion is completely different, and should work
>> without -Werror. Let me try that.
>
> Is something like the patch below what you wanted to try?

This looks sensible.  Will apply.

Re: [PATCH/RFC (take 3)] autoconf: Add test for OLD_ICONV (squelching compiler warning)

[Wincent Colaiuta]

El 7/12/2007, a las 8:26, Junio C Hamano escribió:

> Jakub Narebski <jnareb@gmail.com> writes:
>
>> Is something like the patch below what you wanted to try?
>
> This looks sensible.  Will apply.


This works for those using autoconf. Junio, did you get a chance to  
look at the patch I posted for people who don't use autoconf?

Cheers,
Wincent

Re: [PATCH/RFC (take 3)] autoconf: Add test for OLD_ICONV (squelching compiler warning)

[Wincent Colaiuta]

El 7/12/2007, a las 2:27, Jakub Narebski escribió:

> Update configure.ac (and config.mak.in) to keep up with git
> development by adding [compile] test whether your library has an old
> iconv(), where the second (input buffer pointer) parameter is declared
> with type (const char **) (OLD_ICONV).
>
> Test-proposed-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>

Tested and working here on Mac OS X 10.5.1/Darwin 9.1.0 (where we no  
longer have an old iconv):

configure: CHECKS for header files
checking for old iconv()... no

Cheers,
Wincent

Re: [PATCH/RFC (take 3)] autoconf: Add test for OLD_ICONV (squelching compiler warning)

[Blake Ramsdell]

On Dec 6, 2007 5:27 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Is something like the patch below what you wanted to try?

Yes. Not sure if main() is required, so it might be shorter if that's
removed, but not really relevant as long as it works.

And CHECKS for header files might be considered chatty.

Blake
-- 
Blake Ramsdell | http://www.blakeramsdell.com