threads / patch / 10583

patchMac OS X 10.5 does not require the OLD_ICONV flag set

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

## tl;dr

15 messages between Nov 2, 2007 and Nov 3, 2007. Diffs are folded; open one to read it.

replies: 14people: 5as markdown or json

Blake Ramsdell· Nov 2, 2007, 02:38 UTC · lore
Signed-off-by: Blake Ramsdell <blaker@gmail.com>
---
 Makefile |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Show changes to Makefile +3 −1
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
Blake Ramsdell· Nov 2, 2007, 02:38 UTC · re: Blake Ramsdell · lore

[PATCH] Fixed a gcc 4.0.1 complaint about an uninitialized variable

Signed-off-by: Blake Ramsdell <blaker@gmail.com>
---
 transport.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to transport.c +1 −1
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
Junio C Hamano· Nov 2, 2007, 05:37 UTC · re: Blake Ramsdell · lore

Re: [PATCH] Fixed a gcc 4.0.1 complaint about an uninitialized variable

Blake Ramsdell <blaker@gmail.com> writes:
Show 10 quoted lines
> 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;

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.

Junio C Hamano· Nov 2, 2007, 09:03 UTC · re: Blake Ramsdell · lore

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

Blake Ramsdell <blaker@gmail.com> writes:
Show 20 quoted lines
> 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

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

Blake Ramsdell· Nov 2, 2007, 09:20 UTC · re: Junio C Hamano · lore

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

On 11/2/07, Junio C Hamano <gitster@pobox.com> wrote:
> I do not have an access to a Darwin box, but do you mean 10.5
> gives 9.0.0 as uname_R?
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
Mike Hommey· Nov 2, 2007, 09:30 UTC · re: Junio C Hamano · lore

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

On Fri, Nov 02, 2007 at 02:03:52AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> Blake Ramsdell <blaker@gmail.com> writes:
> 
> > 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
> 
> I do not have an access to a Darwin box, but do you mean 10.5
> gives 9.0.0 as uname_R?

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
Benoit SIGOURE· Nov 2, 2007, 09:39 UTC · re: Mike Hommey · lore

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

On Nov 2, 2007, at 10:30 AM, Mike Hommey wrote:
Show 32 quoted lines
> On Fri, Nov 02, 2007 at 02:03:52AM -0700, Junio C Hamano  
> <gitster@pobox.com> wrote:
>> Blake Ramsdell <blaker@gmail.com> writes:
>>
>>> 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
>>
>> I do not have an access to a Darwin box, but do you mean 10.5
>> gives 9.0.0 as uname_R?
>
> 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 ?

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
Mike Hommey· Nov 2, 2007, 09:45 UTC · re: Benoit SIGOURE · lore

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

On Fri, Nov 02, 2007 at 10:39:35AM +0100, Benoit SIGOURE <tsuna@lrde.epita.fr> wrote:
Show 40 quoted lines
> On Nov 2, 2007, at 10:30 AM, Mike Hommey wrote:
> 
> >On Fri, Nov 02, 2007 at 02:03:52AM -0700, Junio C Hamano  
> ><gitster@pobox.com> wrote:
> >>Blake Ramsdell <blaker@gmail.com> writes:
> >>
> >>>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
> >>
> >>I do not have an access to a Darwin box, but do you mean 10.5
> >>gives 9.0.0 as uname_R?
> >
> >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 ?
> 
> 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.
The fact is you can also use Darwin without OSX...
Mike
David Symonds· Nov 2, 2007, 10:19 UTC · re: Junio C Hamano · lore

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

On 11/2/07, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> Blake Ramsdell <blaker@gmail.com> writes:
>
> > 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
>
> I do not have an access to a Darwin box, but do you mean 10.5
> gives 9.0.0 as uname_R?

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

Dave.
Junio C Hamano· Nov 2, 2007, 10:33 UTC · re: David Symonds · lore

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

"David Symonds" <dsymonds@gmail.com> writes:
Show 29 quoted lines
> On 11/2/07, Junio C Hamano <gitster@pobox.com> wrote:
>> Blake Ramsdell <blaker@gmail.com> writes:
>>
>> > 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
>>
>> I do not have an access to a Darwin box, but do you mean 10.5
>> gives 9.0.0 as uname_R?
>
> Further, that comparison is going to fail as soon as the next revision
> of Darwin (9.0.1, etc.) is released.

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

David Symonds· Nov 2, 2007, 11:23 UTC · re: Junio C Hamano · lore

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

On 11/2/07, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> "David Symonds" <dsymonds@gmail.com> writes:
>
> > Further, that comparison is going to fail as soon as the next revision
> > of Darwin (9.0.1, etc.) is released.
>
> Can we do something intelligent with $(shell iconv --version)
> there instead, I wonder, then?

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.
Blake Ramsdell· Nov 2, 2007, 20:03 UTC · re: David Symonds · lore

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

On 11/2/07, David Symonds <dsymonds@gmail.com> wrote:
> 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?

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
Blake Ramsdell· Nov 3, 2007, 00:00 UTC · re: Blake Ramsdell · lore

[PATCH] Removed OLD_ICONV in favor of checking _LIBICONV_VERSION directly

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

Makefile, utf8.c

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
Junio C Hamano· Nov 3, 2007, 00:07 UTC · re: Blake Ramsdell · lore

Re: [PATCH] Removed OLD_ICONV in favor of checking _LIBICONV_VERSION directly

Blake Ramsdell <blaker@gmail.com> writes:
Show 13 quoted lines
> 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;
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.
Blake Ramsdell· Nov 3, 2007, 00:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] Removed OLD_ICONV in favor of checking _LIBICONV_VERSION directly

On 11/2/07, Junio C Hamano <gitster@pobox.com> wrote:
> Does everybody's iconv use the same _LIBICONV_VERSION scheme?

Apparently not. And furthermore, my assumption about the version number being tied to the function prototype was not valid -- I have a DarwinPorts iconv.h that is version 0x010B but uses const for the second parameter.

Sorry for the mess. Let me try and come up with another plan. That Darwin version check doesn't seem so bad now ;).

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

← back to recent threads