threads / patch / 32880

patchInclude xmlparse.h instead of expat.h on QNX

Subject: [PATCH] Include xmlparse.h instead of expat.h on QNX

## tl;dr

11 messages between Feb 11, 2013 and Feb 11, 2013. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Matt Kraai· Feb 11, 2013, 20:59 UTC · lore
From: Matt Kraai <matt.kraai@amo.abbott.com>

QNX 6.3.2 through 6.5.0 include Expat 1.1, which provides xmlparse.h instead of expat.h, so include the former on QNX systems.

Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
---
 http-push.c | 4 ++++
 1 file changed, 4 insertions(+)
Show changes to http-push.c +4 −0
diff --git a/http-push.c b/http-push.c
index 9923441..55c575e 100644
--- a/http-push.c
+++ b/http-push.c
@@ -11,7 +11,11 @@
 #include "list-objects.h"
 #include "sigchain.h"
 
+#ifndef __QNX__
 #include <expat.h>
+#else
+#include <xmlparse.h>
+#endif
 
 static const char http_push_usage[] =
 "git http-push [--all] [--dry-run] [--force] [--verbose] <remote> [<head>...]\n";
-- 
1.8.1.2.547.g7ce9def
Jeff King· Feb 11, 2013, 21:06 UTC · re: Matt Kraai · lore

Re: [PATCH] Include xmlparse.h instead of expat.h on QNX

On Mon, Feb 11, 2013 at 12:59:55PM -0800, Matt Kraai wrote:
> From: Matt Kraai <matt.kraai@amo.abbott.com>
> 
> QNX 6.3.2 through 6.5.0 include Expat 1.1, which provides xmlparse.h
> instead of expat.h, so include the former on QNX systems.
So it is not just QNX, but rather older versions of expat?
Show 13 quoted lines
> diff --git a/http-push.c b/http-push.c
> index 9923441..55c575e 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -11,7 +11,11 @@
>  #include "list-objects.h"
>  #include "sigchain.h"
>  
> +#ifndef __QNX__
>  #include <expat.h>
> +#else
> +#include <xmlparse.h>
> +#endif

If that is the case, should this #ifdef look for EXPAT_NEEDS_XMLPARSE_H, and that macro triggered externally? Either in the QNX section of the Makefile, or potentially by an autoconf macro?

-Peff
Matt Kraai· Feb 11, 2013, 21:24 UTC · re: Jeff King · lore

Re: [PATCH] Include xmlparse.h instead of expat.h on QNX

On Mon, Feb 11, 2013 at 04:06:21PM -0500, Jeff King wrote:
Show 8 quoted lines
> On Mon, Feb 11, 2013 at 12:59:55PM -0800, Matt Kraai wrote:
> 
> > From: Matt Kraai <matt.kraai@amo.abbott.com>
> > 
> > QNX 6.3.2 through 6.5.0 include Expat 1.1, which provides xmlparse.h
> > instead of expat.h, so include the former on QNX systems.
> 
> So it is not just QNX, but rather older versions of expat?

Yes, Expat 1.1 and 1.2 provide xmlparse.h, whereas 1.95.0 and later provide expat.h.

Show 17 quoted lines
> > diff --git a/http-push.c b/http-push.c
> > index 9923441..55c575e 100644
> > --- a/http-push.c
> > +++ b/http-push.c
> > @@ -11,7 +11,11 @@
> >  #include "list-objects.h"
> >  #include "sigchain.h"
> >  
> > +#ifndef __QNX__
> >  #include <expat.h>
> > +#else
> > +#include <xmlparse.h>
> > +#endif
> 
> If that is the case, should this #ifdef look for EXPAT_NEEDS_XMLPARSE_H,
> and that macro triggered externally? Either in the QNX section of the
> Makefile, or potentially by an autoconf macro?

I'll submit another patch shortly that does so, defining the variable in the QNX section of config.mak.uname.

Matt Kraai· Feb 11, 2013, 22:03 UTC · re: Matt Kraai · lore

[PATCH] Allow building with xmlparse.h

From: Matt Kraai <matt.kraai@amo.abbott.com>

expat 1.1 and 1.2 provide xmlparse.h instead of expat.h. Include the former on systems that define the EXPAT_NEEDS_XMLPARSE_H variable and define that variable on QNX systems, which ship with expat 1.1.

Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
---
 Makefile         | 6 ++++++
 config.mak.uname | 1 +
 http-push.c      | 4 ++++
 3 files changed, 11 insertions(+)
Show changes to 3 files +11 −0

Makefile, config.mak.uname, http-push.c

diff --git a/Makefile b/Makefile
index 5a2e02d..720fc18 100644
--- a/Makefile
+++ b/Makefile
@@ -43,6 +43,9 @@ all::
 # Define EXPATDIR=/foo/bar if your expat header and library files are in
 # /foo/bar/include and /foo/bar/lib directories.
 #
+# Define EXPAT_NEEDS_XMLPARSE_H if you have an old version of expat (e.g.,
+# 1.1 or 1.2) that provides xmlparse.h instead of expat.h.
+#
 # Define NO_GETTEXT if you don't want Git output to be translated.
 # A translated Git requires GNU libintl or another gettext implementation,
 # plus libintl-perl at runtime.
@@ -1089,6 +1092,9 @@ else
 		else
 			EXPAT_LIBEXPAT = -lexpat
 		endif
+		ifdef EXPAT_NEEDS_XMLPARSE_H
+			BASIC_CFLAGS += -DEXPAT_NEEDS_XMLPARSE_H
+		endif
 	endif
 endif
 
diff --git a/config.mak.uname b/config.mak.uname
index bea34f0..8743a6d 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -523,6 +523,7 @@ endif
 endif
 ifeq ($(uname_S),QNX)
 	COMPAT_CFLAGS += -DSA_RESTART=0
+	EXPAT_NEEDS_XMLPARSE_H = YesPlease
 	HAVE_STRINGS_H = YesPlease
 	NEEDS_SOCKET = YesPlease
 	NO_FNMATCH_CASEFOLD = YesPlease
diff --git a/http-push.c b/http-push.c
index 9923441..7202e2d 100644
--- a/http-push.c
+++ b/http-push.c
@@ -11,7 +11,11 @@
 #include "list-objects.h"
 #include "sigchain.h"
 
+#ifndef EXPAT_NEEDS_XMLPARSE_H
 #include <expat.h>
+#else
+#include <xmlparse.h>
+#endif
 
 static const char http_push_usage[] =
 "git http-push [--all] [--dry-run] [--force] [--verbose] <remote> [<head>...]\n";
-- 
1.8.1.2.547.g7ce9def
Junio C Hamano· Feb 11, 2013, 22:11 UTC · re: Matt Kraai · lore

Re: [PATCH] Allow building with xmlparse.h

Matt Kraai <kraai@ftbfs.org> writes:
Show 22 quoted lines
> From: Matt Kraai <matt.kraai@amo.abbott.com>
>
> expat 1.1 and 1.2 provide xmlparse.h instead of expat.h.  Include the
> former on systems that define the EXPAT_NEEDS_XMLPARSE_H variable and
> define that variable on QNX systems, which ship with expat 1.1.
>
> Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
> ---
> ...
> diff --git a/http-push.c b/http-push.c
> index 9923441..7202e2d 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -11,7 +11,11 @@
>  #include "list-objects.h"
>  #include "sigchain.h"
>  
> +#ifndef EXPAT_NEEDS_XMLPARSE_H
>  #include <expat.h>
> +#else
> +#include <xmlparse.h>
> +#endif
Thanks for a quick re-roll.

Is it just me who finds the above hard to read and find the below much more natural?

	#ifdef NEEDS_FOO_H
        #include <foo.h>
        #else
        #include <bar.h>
        #endif
Matt Kraai· Feb 11, 2013, 22:30 UTC · re: Junio C Hamano · lore

[PATCH] Allow building with xmlparse.h

From: Matt Kraai <matt.kraai@amo.abbott.com>

expat 1.1 and 1.2 provide xmlparse.h instead of expat.h. Include the former on systems that define the EXPAT_NEEDS_XMLPARSE_H variable and define that variable on QNX systems, which ship with expat 1.1.

Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
---
 Makefile         | 6 ++++++
 config.mak.uname | 1 +
 http-push.c      | 4 ++++
 3 files changed, 11 insertions(+)

I've changed #ifndef to #ifdef and changed the order of the branches in http-push.c. If you'd also like me to rename the variable (e.g., to NEEDS_XMLPARSE_H), please let me know.

Show changes to 3 files +11 −0

Makefile, config.mak.uname, http-push.c

diff --git a/Makefile b/Makefile
index 5a2e02d..720fc18 100644
--- a/Makefile
+++ b/Makefile
@@ -43,6 +43,9 @@ all::
 # Define EXPATDIR=/foo/bar if your expat header and library files are in
 # /foo/bar/include and /foo/bar/lib directories.
 #
+# Define EXPAT_NEEDS_XMLPARSE_H if you have an old version of expat (e.g.,
+# 1.1 or 1.2) that provides xmlparse.h instead of expat.h.
+#
 # Define NO_GETTEXT if you don't want Git output to be translated.
 # A translated Git requires GNU libintl or another gettext implementation,
 # plus libintl-perl at runtime.
@@ -1089,6 +1092,9 @@ else
 		else
 			EXPAT_LIBEXPAT = -lexpat
 		endif
+		ifdef EXPAT_NEEDS_XMLPARSE_H
+			BASIC_CFLAGS += -DEXPAT_NEEDS_XMLPARSE_H
+		endif
 	endif
 endif
 
diff --git a/config.mak.uname b/config.mak.uname
index bea34f0..8743a6d 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -523,6 +523,7 @@ endif
 endif
 ifeq ($(uname_S),QNX)
 	COMPAT_CFLAGS += -DSA_RESTART=0
+	EXPAT_NEEDS_XMLPARSE_H = YesPlease
 	HAVE_STRINGS_H = YesPlease
 	NEEDS_SOCKET = YesPlease
 	NO_FNMATCH_CASEFOLD = YesPlease
diff --git a/http-push.c b/http-push.c
index 9923441..9fa47a7 100644
--- a/http-push.c
+++ b/http-push.c
@@ -11,7 +11,11 @@
 #include "list-objects.h"
 #include "sigchain.h"
 
+#ifdef EXPAT_NEEDS_XMLPARSE_H
+#include <xmlparse.h>
+#else
 #include <expat.h>
+#endif
 
 static const char http_push_usage[] =
 "git http-push [--all] [--dry-run] [--force] [--verbose] <remote> [<head>...]\n";
-- 
1.8.1.2.547.g7ce9def
Junio C Hamano· Feb 11, 2013, 22:35 UTC · re: Matt Kraai · lore

Re: [PATCH] Allow building with xmlparse.h

Matt Kraai <kraai@ftbfs.org> writes:
Show 16 quoted lines
> From: Matt Kraai <matt.kraai@amo.abbott.com>
>
> expat 1.1 and 1.2 provide xmlparse.h instead of expat.h.  Include the
> former on systems that define the EXPAT_NEEDS_XMLPARSE_H variable and
> define that variable on QNX systems, which ship with expat 1.1.
>
> Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
> ---
>  Makefile         | 6 ++++++
>  config.mak.uname | 1 +
>  http-push.c      | 4 ++++
>  3 files changed, 11 insertions(+)
>
> I've changed #ifndef to #ifdef and changed the order of the branches
> in http-push.c.  If you'd also like me to rename the variable (e.g.,
> to NEEDS_XMLPARSE_H), please let me know.

I do not think renaming is necessary (the name you used in the original and this patch is better than NEEDS_XMLPARSE_H).

I take that you also think the updated order is easier to read; thanks for sanity-checking ;-).

Junio C Hamano· Feb 11, 2013, 21:41 UTC · re: Jeff King · lore

Re: [PATCH] Include xmlparse.h instead of expat.h on QNX

Jeff King <peff@peff.net> writes:
Show 25 quoted lines
> On Mon, Feb 11, 2013 at 12:59:55PM -0800, Matt Kraai wrote:
>
>> From: Matt Kraai <matt.kraai@amo.abbott.com>
>> 
>> QNX 6.3.2 through 6.5.0 include Expat 1.1, which provides xmlparse.h
>> instead of expat.h, so include the former on QNX systems.
>
> So it is not just QNX, but rather older versions of expat?
>
>> diff --git a/http-push.c b/http-push.c
>> index 9923441..55c575e 100644
>> --- a/http-push.c
>> +++ b/http-push.c
>> @@ -11,7 +11,11 @@
>>  #include "list-objects.h"
>>  #include "sigchain.h"
>>  
>> +#ifndef __QNX__
>>  #include <expat.h>
>> +#else
>> +#include <xmlparse.h>
>> +#endif
>
> If that is the case, should this #ifdef look for EXPAT_NEEDS_XMLPARSE_H,
> and that macro triggered externally?

Heh, our mails crossed. Another thing neither of us mentioned is how compatible the subset of libexpat our codebase uses to what was offered by the older versions of expat. I would not be surprised if nobody has tried running the resulting binary linked with expat 1.

Junio C Hamano· Feb 11, 2013, 21:34 UTC · re: Matt Kraai · lore

Re: [PATCH] Include xmlparse.h instead of expat.h on QNX

Matt Kraai <kraai@ftbfs.org> writes:
Show 7 quoted lines
> From: Matt Kraai <matt.kraai@amo.abbott.com>
>
> QNX 6.3.2 through 6.5.0 include Expat 1.1, which provides xmlparse.h
> instead of expat.h, so include the former on QNX systems.
>
> Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
> ---
Two points and a possibly irrelevant half:
 - If a fix is platform specific (i.e. tempts to use #ifdef
   PLATFORM_NAME), we would prefer to see a patch that that is
   isolated to platform-specific compatibility layer, which would
   involve:
   . add compat/qnx/expat.h file that #include <xmlparse.h>
   . to Makefile, add -Icompat/qnx/ to CFLAGS
 - Is this really a fix for a problem specific to QNX?  It looks
   like this is for any platform with expat 1, no?
 - What happens to people with QNX older than 6.3.2 or newer than
   6.5.0 (assuming they will eventually start shipping expat 2) with
   your patch?

Assuming that this change is about building with expat1, it would probably be better to do something like this instead, I would think.

 Makefile         | 5 +++++
 config.mak.uname | 1 +
 http-push.c      | 4 ++++
 3 files changed, 10 insertions(+)
Show changes to 3 files +10 −0

Makefile, config.mak.uname, http-push.c

diff --git a/Makefile b/Makefile
index 5a2e02d..57032cc 100644
--- a/Makefile
+++ b/Makefile
@@ -43,6 +43,8 @@ all::
 # Define EXPATDIR=/foo/bar if your expat header and library files are in
 # /foo/bar/include and /foo/bar/lib directories.
 #
+# Define EXPAT_VERSION=1 if you are trying to build with expat 1.x (e.g. QNX).
+#
 # Define NO_GETTEXT if you don't want Git output to be translated.
 # A translated Git requires GNU libintl or another gettext implementation,
 # plus libintl-perl at runtime.
@@ -1089,6 +1091,9 @@ else
 		else
 			EXPAT_LIBEXPAT = -lexpat
 		endif
+		ifdef EXPAT_VERSION
+		BASIC_CFLAGS += -DEXPAT_VERSION=$(EXPAT_VERSION)
+		endif
 	endif
 endif
 
diff --git a/config.mak.uname b/config.mak.uname
index bea34f0..281d834 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -536,4 +536,5 @@ ifeq ($(uname_S),QNX)
 	NO_R_TO_GCC_LINKER = YesPlease
 	NO_STRCASESTR = YesPlease
 	NO_STRLCPY = YesPlease
+	EXPAT_VERSION = 1
 endif
diff --git a/http-push.c b/http-push.c
index 3e72e84..2fdb0cd 100644
--- a/http-push.c
+++ b/http-push.c
@@ -11,7 +11,11 @@
 #include "list-objects.h"
 #include "sigchain.h"
 
+#if EXPAT_VERSION == 1
+#include <xmlparse.h>
+#else
 #include <expat.h>
+#endif
 
 static const char http_push_usage[] =
 "git http-push [--all] [--dry-run] [--force] [--verbose] <remote> [<head>...]\n";
Matt Kraai· Feb 11, 2013, 21:49 UTC · re: Junio C Hamano · lore

Re: [PATCH] Include xmlparse.h instead of expat.h on QNX

On Mon, Feb 11, 2013 at 01:34:52PM -0800, Junio C Hamano wrote:
Show 12 quoted lines
> Two points and a possibly irrelevant half:
> 
>  - If a fix is platform specific (i.e. tempts to use #ifdef
>    PLATFORM_NAME), we would prefer to see a patch that that is
>    isolated to platform-specific compatibility layer, which would
>    involve:
> 
>    . add compat/qnx/expat.h file that #include <xmlparse.h>
>    . to Makefile, add -Icompat/qnx/ to CFLAGS
> 
>  - Is this really a fix for a problem specific to QNX?  It looks
>    like this is for any platform with expat 1, no?

It should apply to anyone trying to build with expat 1.1 or 1.2, but not with 1.95.0 or later.

>  - What happens to people with QNX older than 6.3.2 or newer than
>    6.5.0 (assuming they will eventually start shipping expat 2) with
>    your patch?

Git will fail to build http-push.c. I don't know if QNX will ever update expat, though. expat 1.95.0 was released in 2000, expat 2.0.0 was released in 2006, and QNX 6.5.0 was released in 2010.

> Assuming that this change is about building with expat1, it would
> probably be better to do something like this instead, I would think.

expat 1.95.0 through 1.95.8 used expat.h; should I still use EXPAT_VERSION = 1 to signify that it should use xmlparse.h, use EXPAT_NEEDS_XMLPARSE_H as Jeff suggested, or something else entirely?

Junio C Hamano· Feb 11, 2013, 21:53 UTC · re: Matt Kraai · lore

Re: [PATCH] Include xmlparse.h instead of expat.h on QNX

Matt Kraai <kraai@ftbfs.org> writes:
Show 6 quoted lines
>> Assuming that this change is about building with expat1, it would
>> probably be better to do something like this instead, I would think.
>
> expat 1.95.0 through 1.95.8 used expat.h; should I still use
> EXPAT_VERSION = 1 to signify that it should use xmlparse.h, use
> EXPAT_NEEDS_XMLPARSE_H as Jeff suggested, or something else entirely?

Oh, please do not take it as a request to use that exact name (in case you didn't know, I am bad at naming things). It was merely an illustration to show the direction, written without knowing that Peff was essentially giving the same suggestion.

Thanks.

Oh, by the way, please do not deflect an attempt to directly send a response to you with a Mail-Followup-To header.

← back to recent threads