# [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set 15 messages from 2007-11-02 to 2007-11-03. Participants: Blake Ramsdell, Junio C Hamano, Mike Hommey, Benoit SIGOURE, David Symonds. Thread: https://gitlist.dev/t/10583 ## Blake Ramsdell, 2007-11-02 02:38 Subject: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set Message-ID: <1193971102-61907-1-git-send-email-blaker@gmail.com> URL: https://gitlist.dev/e/1193971102-61907-1-git-send-email-blaker%40gmail.com ``` Signed-off-by: Blake Ramsdell --- 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 ``` ## Blake Ramsdell, 2007-11-02 02:38 Subject: [PATCH] Fixed a gcc 4.0.1 complaint about an uninitialized variable Message-ID: <1193971102-61907-2-git-send-email-blaker@gmail.com> URL: https://gitlist.dev/e/1193971102-61907-2-git-send-email-blaker%40gmail.com In-Reply-To: <1193971102-61907-1-git-send-email-blaker@gmail.com> ``` Signed-off-by: Blake Ramsdell --- 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 ``` ## Junio C Hamano, 2007-11-02 05:37 Subject: Re: [PATCH] Fixed a gcc 4.0.1 complaint about an uninitialized variable Message-ID: <7v8x5h6wd0.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8x5h6wd0.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1193971102-61907-2-git-send-email-blaker@gmail.com> ``` Blake Ramsdell writes: > 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, 2007-11-02 09:03 Subject: Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set Message-ID: <7v4pg55893.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4pg55893.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1193971102-61907-1-git-send-email-blaker@gmail.com> ``` Blake Ramsdell writes: > Signed-off-by: Blake Ramsdell > --- > 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, 2007-11-02 09:20 Subject: Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set Message-ID: <985966520711020220q5d0b0481r9b95f4429f8680c6@mail.gmail.com> URL: https://gitlist.dev/e/985966520711020220q5d0b0481r9b95f4429f8680c6%40mail.gmail.com In-Reply-To: <7v4pg55893.fsf@gitster.siamese.dyndns.org> ``` On 11/2/07, Junio C Hamano 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, 2007-11-02 09:30 Subject: Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set Message-ID: <20071102093028.GA29699@glandium.org> URL: https://gitlist.dev/e/20071102093028.GA29699%40glandium.org In-Reply-To: <7v4pg55893.fsf@gitster.siamese.dyndns.org> ``` On Fri, Nov 02, 2007 at 02:03:52AM -0700, Junio C Hamano wrote: > Blake Ramsdell writes: > > > Signed-off-by: Blake Ramsdell > > --- > > 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, 2007-11-02 09:39 Subject: Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set Message-ID: URL: https://gitlist.dev/e/B858412A-853A-4660-BA4B-F9751E4AAEAE%40lrde.epita.fr In-Reply-To: <20071102093028.GA29699@glandium.org> ``` On Nov 2, 2007, at 10:30 AM, Mike Hommey wrote: > On Fri, Nov 02, 2007 at 02:03:52AM -0700, Junio C Hamano > wrote: >> Blake Ramsdell writes: >> >>> Signed-off-by: Blake Ramsdell >>> --- >>> 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, 2007-11-02 09:45 Subject: Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set Message-ID: <20071102094508.GA29828@glandium.org> URL: https://gitlist.dev/e/20071102094508.GA29828%40glandium.org In-Reply-To: ``` On Fri, Nov 02, 2007 at 10:39:35AM +0100, Benoit SIGOURE wrote: > On Nov 2, 2007, at 10:30 AM, Mike Hommey wrote: > > >On Fri, Nov 02, 2007 at 02:03:52AM -0700, Junio C Hamano > > wrote: > >>Blake Ramsdell writes: > >> > >>>Signed-off-by: Blake Ramsdell > >>>--- > >>> 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, 2007-11-02 10:19 Subject: Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set Message-ID: URL: https://gitlist.dev/e/ee77f5c20711020319l26b4a8c1r99576dc29ea504d8%40mail.gmail.com In-Reply-To: <7v4pg55893.fsf@gitster.siamese.dyndns.org> ``` On 11/2/07, Junio C Hamano wrote: > Blake Ramsdell writes: > > > Signed-off-by: Blake Ramsdell > > --- > > 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, 2007-11-02 10:33 Subject: Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set Message-ID: <7vbqad3pjw.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vbqad3pjw.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` "David Symonds" writes: > On 11/2/07, Junio C Hamano wrote: >> Blake Ramsdell writes: >> >> > Signed-off-by: Blake Ramsdell >> > --- >> > 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, 2007-11-02 11:23 Subject: Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set Message-ID: URL: https://gitlist.dev/e/ee77f5c20711020423t6ce58818gcc5220b6427ded1%40mail.gmail.com In-Reply-To: <7vbqad3pjw.fsf@gitster.siamese.dyndns.org> ``` On 11/2/07, Junio C Hamano wrote: > "David Symonds" 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, 2007-11-02 20:03 Subject: Re: [PATCH] Mac OS X 10.5 does not require the OLD_ICONV flag set Message-ID: <985966520711021303g905f638y2056219c22e5edeb@mail.gmail.com> URL: https://gitlist.dev/e/985966520711021303g905f638y2056219c22e5edeb%40mail.gmail.com In-Reply-To: ``` On 11/2/07, David Symonds 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, 2007-11-03 00:00 Subject: [PATCH] Removed OLD_ICONV in favor of checking _LIBICONV_VERSION directly Message-ID: <1194048010-65955-1-git-send-email-blaker@gmail.com> URL: https://gitlist.dev/e/1194048010-65955-1-git-send-email-blaker%40gmail.com In-Reply-To: <985966520711021303g905f638y2056219c22e5edeb@mail.gmail.com> ``` Signed-off-by: Blake Ramsdell --- 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 ``` ## Junio C Hamano, 2007-11-03 00:07 Subject: Re: [PATCH] Removed OLD_ICONV in favor of checking _LIBICONV_VERSION directly Message-ID: <7vbqacyyx1.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vbqacyyx1.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1194048010-65955-1-git-send-email-blaker@gmail.com> ``` Blake Ramsdell writes: > 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 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, 2007-11-03 00:21 Subject: Re: [PATCH] Removed OLD_ICONV in favor of checking _LIBICONV_VERSION directly Message-ID: <985966520711021721i6345e4bfv8a016a1528ee4e23@mail.gmail.com> URL: https://gitlist.dev/e/985966520711021721i6345e4bfv8a016a1528ee4e23%40mail.gmail.com In-Reply-To: <7vbqacyyx1.fsf@gitster.siamese.dyndns.org> ``` On 11/2/07, Junio C Hamano 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 ```