Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set

Previous thread: none

Next thread: [PATCH 1/2] War on whitespace: first, a bit of retreat. by Junio C Hamano on Friday, November 2, 2007 - 12:34 am. (9 messages)
From: Blake Ramsdell
Date: Thursday, November 1, 2007 - 7:38 pm

Signed-off-by: Blake Ramsdell <blaker@gmail.com>
---
 Makefile |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 71479a2..5d83756 100644
--- a/Makefile
+++ b/Makefile
@@ -401,7 +401,9 @@ endif
 ifeq ($(uname_S),Darwin)
 	NEEDS_SSL_WITH_CRYPTO = YesPlease
 	NEEDS_LIBICONV = YesPlease
-	OLD_ICONV = UnfortunatelyYes
+	ifneq ($(uname_R),9.0.0)
+		OLD_ICONV = UnfortunatelyYes
+	endif
 	NO_STRLCPY = YesPlease
 	NO_MEMMEM = YesPlease
 endif
-- 
1.5.3.GIT

-

From: Blake Ramsdell
Date: Thursday, November 1, 2007 - 7:38 pm

Signed-off-by: Blake Ramsdell <blaker@gmail.com>
---
 transport.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/transport.c b/transport.c
index 400af71..cac1870 100644
--- a/transport.c
+++ b/transport.c
@@ -107,7 +107,7 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
 		return;
 
 	for (;;) {
-		int cmp, len;
+		int cmp = 0, len;
 
 		if (!fgets(buffer, sizeof(buffer), f)) {
 			fclose(f);
-- 
1.5.3.GIT

-

From: Junio C Hamano
Date: Thursday, November 1, 2007 - 10:37 pm

Yeah, if you follow the logic, it is clear that the variable is
never used while unset, but gcc is not careful enough to see it.

It is customary to use

	int cmp = cmp;

for something like this.  There are already other instances of
such phony initializations in the code elsewhere.

-

From: Junio C Hamano
Date: Friday, November 2, 2007 - 2:03 am

I do not have an access to a Darwin box, but do you mean 10.5
gives 9.0.0 as uname_R?


-

From: Blake Ramsdell
Date: Friday, November 2, 2007 - 2:20 am

Yeah, apparently that's how they roll.

~/Source/OpenSource/git$ uname -r
9.0.0
~/Source/OpenSource/git$ system_profiler SPSoftwareDataType
Software:

    System Software Overview:

      System Version: Mac OS X 10.5 (9A581)
      Kernel Version: Darwin 9.0.0

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

From: Mike Hommey
Date: Friday, November 2, 2007 - 2:30 am

Be it that or not, it looks wrong to me to check the Darwin version to
know what to use. Do you rely on the Linux kernel version to know whether
iconv is present ?

Mike
-

From: Benoit SIGOURE
Date: Friday, November 2, 2007 - 2:39 am

It's very different, on OSX you don't change your own kernel as you  
want, the kernel isn't a standalone component, it comes packaged with  
the entire system of MacOSX.  When you do an update to 10.5 (aka  
Leopard) you will have a new version of iconv so you're guaranteed  
that someone with 10.5 has a system-wide iconv that is not OLD_ICONV.

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory


From: Mike Hommey
Date: Friday, November 2, 2007 - 2:45 am

The fact is you can also use Darwin without OSX...

Mike
-

From: David Symonds
Date: Friday, November 2, 2007 - 3:19 am

Further, that comparison is going to fail as soon as the next revision
of Darwin (9.0.1, etc.) is released.


Dave.
-

From: Junio C Hamano
Date: Friday, November 2, 2007 - 3:33 am

Can we do something intelligent with $(shell iconv --version)
there instead, I wonder, then?

-

From: David Symonds
Date: Friday, November 2, 2007 - 4:23 am

It would probably be most appropriate for the autoconf script. Now
that I look at configure.ac, there's already a test for iconv in
there; is it not used?


Dave.
-

From: Blake Ramsdell
Date: Friday, November 2, 2007 - 1:03 pm

The crux of this problem is that the prototype for iconv in
/usr/include/iconv.h is different between OS X 10.4 and OS X 10.5. So
the "right thing" is definitely to determine what is in the function
prototype, and then act accordingly.

From OS X 10.4.10:

#define _LIBICONV_VERSION 0x0109    /* version number: (major<<8) + minor */
...
extern size_t iconv (iconv_t cd, const char* * inbuf, size_t
*inbytesleft, char* * outbuf, size_t *outbytesleft);

From OS X 10.5:

#define _LIBICONV_VERSION 0x010B    /* version number: (major<<8) + minor */
...
size_t iconv (iconv_t /*cd*/,
        char ** __restrict /*inbuf*/,  size_t * __restrict /*inbytesleft*/,
        char ** __restrict /*outbuf*/, size_t * __restrict /*outbytesleft*/);

So what happened in git is that someone put in OLD_ICONV to
dynamically adjust the const-ness of parameter 2 to the iconv
function, and the way they chose to do that is to identify the OS
(more accurately, the kernel), and then I went and furthered that by
identifying the version.

Now that I look at it further, it seems that yanking OLD_ICONV
altogether is a better approach, and to just check _LIBICONV_VERSION
to make sure it's "new enough". Now, I'm not sure what that comparison
would be, but we know that "later than 0x0109" is a good start. Based
on the version difference between OS X 10.4 and 10.5 I note that there
is only 0x010A intervening.

This strategy presumes that this const parameter was const all the way
up to a particular point, and then stopped being const, which is
probably a reasonable assumption.

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

From: Blake Ramsdell
Date: Friday, November 2, 2007 - 5:00 pm

Signed-off-by: Blake Ramsdell <blaker@gmail.com>
---
 Makefile |    8 --------
 utf8.c   |    2 +-
 2 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index 71479a2..96c8a73 100644
--- a/Makefile
+++ b/Makefile
@@ -95,9 +95,6 @@ all::
 #
 # Define NO_ICONV if your libc does not properly support iconv.
 #
-# Define OLD_ICONV if your library has an old iconv(), where the second
-# (input buffer pointer) parameter is declared with type (const char **).
-#
 # Define NO_R_TO_GCC_LINKER if your gcc does not like "-R/path/lib"
 # that tells runtime paths to dynamic libraries;
 # "-Wl,-rpath=/path/lib" is used instead.
@@ -401,7 +398,6 @@ endif
 ifeq ($(uname_S),Darwin)
 	NEEDS_SSL_WITH_CRYPTO = YesPlease
 	NEEDS_LIBICONV = YesPlease
-	OLD_ICONV = UnfortunatelyYes
 	NO_STRLCPY = YesPlease
 	NO_MEMMEM = YesPlease
 endif
@@ -657,10 +653,6 @@ ifdef NO_ICONV
 	BASIC_CFLAGS += -DNO_ICONV
 endif
 
-ifdef OLD_ICONV
-	BASIC_CFLAGS += -DOLD_ICONV
-endif
-
 ifdef PPC_SHA1
 	SHA1_HEADER = "ppc/sha1.h"
 	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
diff --git a/utf8.c b/utf8.c
index 4efef6f..a7feb4f 100644
--- a/utf8.c
+++ b/utf8.c
@@ -300,7 +300,7 @@ int is_encoding_utf8(const char *name)
  * with iconv.  If the conversion fails, returns NULL.
  */
 #ifndef NO_ICONV
-#ifdef OLD_ICONV
+#if _LIBICONV_VERSION <= 0x0109
 	typedef const char * iconv_ibp;
 #else
 	typedef char * iconv_ibp;
-- 
1.5.3.GIT

-

From: Junio C Hamano
Date: Friday, November 2, 2007 - 5:07 pm

Does everybody's iconv use the same _LIBICONV_VERSION scheme?

Compiling this:

        #include <iconv.h>
        int i = _LIBICONV_VERSION;

on a Linux box with GNU C library gives:

	i.c:3: error: '_LIBICONV_VERSION' undeclared here (not in a function)

here.
-

From: Blake Ramsdell
Date: Friday, November 2, 2007 - 5:21 pm

[Empty message]
Previous thread: none

Next thread: [PATCH 1/2] War on whitespace: first, a bit of retreat. by Junio C Hamano on Friday, November 2, 2007 - 12:34 am. (9 messages)