[PATCH] tools/check: check for yajl (needed by libxl)

[PATCH] tools/check: check for yajl (needed by libxl)

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1317367995 -3600
# Node ID 4b98868690218126b90620d9b43fdd4140145a43
# Parent  e50da6b98e3d5933b9c98e8f43096fd3ebbae00d
tools/check: check for yajl (needed by libxl)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
(note to committer, ensure the new file is executable)

diff -r e50da6b98e3d -r 4b9886869021 tools/check/check_yajl_lib
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/check/check_yajl_lib	Fri Sep 30 08:33:15 2011 +0100
@@ -0,0 +1,7 @@
+#!/bin/sh
+# CHECK-BUILD CHECK-INSTALL
+
+. ./funcs.sh
+
+
+has_lib libyajl.so || fail "can't find yajl"

Re: [PATCH] tools/check: check for yajl (needed by libxl)

[Anthony PERARD]

On Fri, Sep 30, 2011 at 08:36, Ian Campbell <ian.campbell@citrix.com> wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1317367995 -3600
> # Node ID 4b98868690218126b90620d9b43fdd4140145a43
> # Parent  e50da6b98e3d5933b9c98e8f43096fd3ebbae00d
> tools/check: check for yajl (needed by libxl)
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> (note to committer, ensure the new file is executable)
>
> diff -r e50da6b98e3d -r 4b9886869021 tools/check/check_yajl_lib
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/check/check_yajl_lib        Fri Sep 30 08:33:15 2011 +0100
> @@ -0,0 +1,7 @@
> +#!/bin/sh
> +# CHECK-BUILD CHECK-INSTALL
> +
> +. ./funcs.sh
> +
> +
> +has_lib libyajl.so || fail "can't find yajl"

You probably want to check the yajl headers as well, no ?
#include <yajl/yajl_parse.h>
#include <yajl/yajl_gen.h>

Regards,

-- 
Anthony PERARD

Re: [PATCH] tools/check: check for yajl (needed by libxl)

[Ian Campbell]

On Fri, 2011-09-30 at 11:35 +0100, Anthony PERARD wrote:
> On Fri, Sep 30, 2011 at 08:36, Ian Campbell <ian.campbell@citrix.com> wrote:
> > # HG changeset patch
> > # User Ian Campbell <ian.campbell@citrix.com>
> > # Date 1317367995 -3600
> > # Node ID 4b98868690218126b90620d9b43fdd4140145a43
> > # Parent  e50da6b98e3d5933b9c98e8f43096fd3ebbae00d
> > tools/check: check for yajl (needed by libxl)
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > (note to committer, ensure the new file is executable)
> >
> > diff -r e50da6b98e3d -r 4b9886869021 tools/check/check_yajl_lib
> > --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> > +++ b/tools/check/check_yajl_lib        Fri Sep 30 08:33:15 2011 +0100
> > @@ -0,0 +1,7 @@
> > +#!/bin/sh
> > +# CHECK-BUILD CHECK-INSTALL
> > +
> > +. ./funcs.sh
> > +
> > +
> > +has_lib libyajl.so || fail "can't find yajl"
> 
> You probably want to check the yajl headers as well, no ?
> #include <yajl/yajl_parse.h>
> #include <yajl/yajl_gen.h>

tools/check seems a bit inconsistent and I'd expect them both to be in
the -dev package but I guess it can't hurt.

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1317379175 -3600
# Node ID de602616358b7def9351850f518e453c68141c4f
# Parent  302b7556edd91a7506f2215bed5302b4b5eaa52a
tools/check: check for yajl (needed by libxl)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
(note to committer, ensure the new files are executable)

diff -r 302b7556edd9 -r de602616358b tools/check/check_yajl_devel
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/check/check_yajl_devel	Fri Sep 30 11:39:35 2011 +0100
@@ -0,0 +1,7 @@
+#!/bin/sh
+# CHECK-BUILD CHECK-INSTALL
+
+. ./funcs.sh
+
+has_header yajl/yajl_parse.h || fail "can't find yajl/yajl_parse.h"
+has_header yajl/yajl_gen.h || fail "can't find yajl/yajl_gen.h"
diff -r 302b7556edd9 -r de602616358b tools/check/check_yajl_lib
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/check/check_yajl_lib	Fri Sep 30 11:39:35 2011 +0100
@@ -0,0 +1,6 @@
+#!/bin/sh
+# CHECK-BUILD CHECK-INSTALL
+
+. ./funcs.sh
+
+has_lib libyajl.so || fail "can't find libyajl.so"

Re: [PATCH] tools/check: check for yajl (needed by libxl)

[Olaf Hering]

On Fri, Sep 30, Ian Campbell wrote:

> (note to committer, ensure the new file is executable)

Please dont, thats not patch friendly.

Shouldnt all script files be called with like 
'$interpreter $script' instead of './$script'?

Olaf

Re: [PATCH] tools/check: check for yajl (needed by libxl)

[Ian Campbell]

On Fri, 2011-09-30 at 13:02 +0100, Olaf Hering wrote:
> On Fri, Sep 30, Ian Campbell wrote:
> 
> > (note to committer, ensure the new file is executable)
> 
> Please dont, thats not patch friendly.
> 
> Shouldnt all script files be called with like 
> '$interpreter $script' instead of './$script'?

In general I agree, but that isn't how it the infrastructure in
tools/check/chk works right now.

Ian.

Re: [PATCH] tools/check: check for yajl (needed by libxl)

[Olaf Hering]

On Fri, Sep 30, Ian Campbell wrote:

> On Fri, 2011-09-30 at 13:02 +0100, Olaf Hering wrote:
> > On Fri, Sep 30, Ian Campbell wrote:
> > 
> > > (note to committer, ensure the new file is executable)
> > 
> > Please dont, thats not patch friendly.
> > 
> > Shouldnt all script files be called with like 
> > '$interpreter $script' instead of './$script'?
> 
> In general I agree, but that isn't how it the infrastructure in
> tools/check/chk works right now.

There is tools/check/check_logging, which says it requires code with is
included in Python 2.3 and later.
The toplevel README states 2.3 or later is required, so
tools/check/check_logging could be removed and line 63 in
tools/check/chk be updated.

Olaf

Re: [PATCH] tools/check: check for yajl (needed by libxl)

[Ian Campbell]

On Fri, 2011-09-30 at 13:15 +0100, Olaf Hering wrote:
> On Fri, Sep 30, Ian Campbell wrote:
> 
> > On Fri, 2011-09-30 at 13:02 +0100, Olaf Hering wrote:
> > > On Fri, Sep 30, Ian Campbell wrote:
> > > 
> > > > (note to committer, ensure the new file is executable)
> > > 
> > > Please dont, thats not patch friendly.
> > > 
> > > Shouldnt all script files be called with like 
> > > '$interpreter $script' instead of './$script'?
> > 
> > In general I agree, but that isn't how it the infrastructure in
> > tools/check/chk works right now.
> 
> There is tools/check/check_logging, which says it requires code with is
> included in Python 2.3 and later.
> The toplevel README states 2.3 or later is required, so
> tools/check/check_logging could be removed and line 63 in
> tools/check/chk be updated.

Patches accepted, I don't think this should block the patch in this
thread though.

Ian.

Re: [PATCH] tools/check: check for yajl (needed by libxl)

[Andre Przywara]

On 09/30/2011 12:39 PM, Ian Campbell wrote:
> On Fri, 2011-09-30 at 11:35 +0100, Anthony PERARD wrote:
>> On Fri, Sep 30, 2011 at 08:36, Ian Campbell<ian.campbell@citrix.com>  wrote:
>>  ...
>> You probably want to check the yajl headers as well, no ?
>> #include<yajl/yajl_parse.h>
>> #include<yajl/yajl_gen.h>
>
> tools/check seems a bit inconsistent and I'd expect them both to be in
> the -dev package but I guess it can't hurt.
>
> # HG changeset patch
> # User Ian Campbell<ian.campbell@citrix.com>
> # Date 1317379175 -3600
> # Node ID de602616358b7def9351850f518e453c68141c4f
> # Parent  302b7556edd91a7506f2215bed5302b4b5eaa52a
> tools/check: check for yajl (needed by libxl)
>
> Signed-off-by: Ian Campbell<ian.campbell@citrix.com>
> ---
> (note to committer, ensure the new files are executable)
>
> diff -r 302b7556edd9 -r de602616358b tools/check/check_yajl_devel
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/check/check_yajl_devel	Fri Sep 30 11:39:35 2011 +0100
> @@ -0,0 +1,7 @@
> +#!/bin/sh
> +# CHECK-BUILD CHECK-INSTALL

Shouldn't you be able to omit the CHECK_INSTALL on this one?
The header files are not a runtime requirement, right?

Also wouldn't it be nicer to link this small library statically? This 
would omit the runtime requirement at all.
The test team just spelled yajl as "yet another library to get Xen 
running"...

Also be advised that it looks like this is relying on version 1 of yajl. 
It looks like the new version 2.0.1 is not compatible, I got compile 
errors with the newer version. If the stable version 1.x is required, 
this should be stated somewhere or at least checked properly in check_yajl.

Regards,
Andre.

> +
> +. ./funcs.sh
> +
> +has_header yajl/yajl_parse.h || fail "can't find yajl/yajl_parse.h"
> +has_header yajl/yajl_gen.h || fail "can't find yajl/yajl_gen.h"
> diff -r 302b7556edd9 -r de602616358b tools/check/check_yajl_lib
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/check/check_yajl_lib	Fri Sep 30 11:39:35 2011 +0100
> @@ -0,0 +1,6 @@
> +#!/bin/sh
> +# CHECK-BUILD CHECK-INSTALL
> +
> +. ./funcs.sh
> +
> +has_lib libyajl.so || fail "can't find libyajl.so"
>
>

Re: [PATCH] tools/check: check for yajl (needed by libxl)

[Ian Campbell]

On Tue, 2011-10-04 at 15:48 +0100, Andre Przywara wrote:
> On 09/30/2011 12:39 PM, Ian Campbell wrote:
> > On Fri, 2011-09-30 at 11:35 +0100, Anthony PERARD wrote:
> >> On Fri, Sep 30, 2011 at 08:36, Ian Campbell<ian.campbell@citrix.com>  wrote:
> >>  ...
> >> You probably want to check the yajl headers as well, no ?
> >> #include<yajl/yajl_parse.h>
> >> #include<yajl/yajl_gen.h>
> >
> > tools/check seems a bit inconsistent and I'd expect them both to be in
> > the -dev package but I guess it can't hurt.
> >
> > # HG changeset patch
> > # User Ian Campbell<ian.campbell@citrix.com>
> > # Date 1317379175 -3600
> > # Node ID de602616358b7def9351850f518e453c68141c4f
> > # Parent  302b7556edd91a7506f2215bed5302b4b5eaa52a
> > tools/check: check for yajl (needed by libxl)
> >
> > Signed-off-by: Ian Campbell<ian.campbell@citrix.com>
> > ---
> > (note to committer, ensure the new files are executable)
> >
> > diff -r 302b7556edd9 -r de602616358b tools/check/check_yajl_devel
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/tools/check/check_yajl_devel	Fri Sep 30 11:39:35 2011 +0100
> > @@ -0,0 +1,7 @@
> > +#!/bin/sh
> > +# CHECK-BUILD CHECK-INSTALL
> 
> Shouldn't you be able to omit the CHECK_INSTALL on this one?
> The header files are not a runtime requirement, right?

Correct, it was a cut'n'paste-o.

> Also wouldn't it be nicer to link this small library statically? This 
> would omit the runtime requirement at all.
> The test team just spelled yajl as "yet another library to get Xen 
> running"...

I don't think this is necessary, it's just a library you need to install
and for most users the packaging system will transparently cover this.
All the usual reasons for not statically linking stuff apply here.
There's nothing special about that wrt Xen and/or the size of the
library.

> Also be advised that it looks like this is relying on version 1 of yajl. 
> It looks like the new version 2.0.1 is not compatible, I got compile 
> errors with the newer version. If the stable version 1.x is required, 
> this should be stated somewhere or at least checked properly in check_yajl.

Any idea what we could test for here?

> 
> Regards,
> Andre.
> 
> > +
> > +. ./funcs.sh
> > +
> > +has_header yajl/yajl_parse.h || fail "can't find yajl/yajl_parse.h"
> > +has_header yajl/yajl_gen.h || fail "can't find yajl/yajl_gen.h"
> > diff -r 302b7556edd9 -r de602616358b tools/check/check_yajl_lib
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/tools/check/check_yajl_lib	Fri Sep 30 11:39:35 2011 +0100
> > @@ -0,0 +1,6 @@
> > +#!/bin/sh
> > +# CHECK-BUILD CHECK-INSTALL
> > +
> > +. ./funcs.sh
> > +
> > +has_lib libyajl.so || fail "can't find libyajl.so"
> >
> >
> 

Re: [PATCH] tools/check: check for yajl (needed by libxl)

[Andre Przywara]

On 10/04/2011 05:00 PM, Ian Campbell wrote:
> On Tue, 2011-10-04 at 15:48 +0100, Andre Przywara wrote:
>> On 09/30/2011 12:39 PM, Ian Campbell wrote:
>>> On Fri, 2011-09-30 at 11:35 +0100, Anthony PERARD wrote:
>>>> On Fri, Sep 30, 2011 at 08:36, Ian Campbell<ian.campbell@citrix.com>   wrote:
>>>>   ...
>>>> You probably want to check the yajl headers as well, no ?
>>>> #include<yajl/yajl_parse.h>
>>>> #include<yajl/yajl_gen.h>
>>>
> ...
>> Also wouldn't it be nicer to link this small library statically? This
>> would omit the runtime requirement at all.
>> The test team just spelled yajl as "yet another library to get Xen
>> running"...
>
> I don't think this is necessary, it's just a library you need to install
> and for most users the packaging system will transparently cover this.
> All the usual reasons for not statically linking stuff apply here.
> There's nothing special about that wrt Xen and/or the size of the
> library.

Unfortunately this package is not omnipresent in the distributions. 
Though I find it already in Fedora 13, it is not in the official RHEL5 
repos, also not in anything before OpenSuSE 11.4 and before SLES11SP2. 
This creates quite some hassle for any people using one of these systems 
to build and run Xen. And pulling it from github will push most people 
(like me ;-) into version 2 in the first place.

But I guess this will not convince you to drop it from unstable. That's 
why I thought static linking (or including in the tree?) would ease the 
pain here.

>> Also be advised that it looks like this is relying on version 1 of yajl.
>> It looks like the new version 2.0.1 is not compatible, I got compile
>> errors with the newer version. If the stable version 1.x is required,
>> this should be stated somewhere or at least checked properly in check_yajl.
>
> Any idea what we could test for here?

The sanest way would be to do it like configure and check explicitly for 
the data structure that we rely on, but I guess just looking for a 
symlink called libyajl.so.1 in /usr/lib64 should suffice here.


>>> diff -r 302b7556edd9 -r de602616358b tools/check/check_yajl_lib
>>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>>> +++ b/tools/check/check_yajl_lib	Fri Sep 30 11:39:35 2011 +0100
>>> @@ -0,0 +1,6 @@
>>> +#!/bin/sh
>>> +# CHECK-BUILD CHECK-INSTALL
>>> +
>>> +. ./funcs.sh
>>> +
>>> +has_lib libyajl.so || fail "can't find libyajl.so"

+has_lib libyajl.so.1 || fail "can't find libyajl.so version 1"

Regards,
Andre.

-- 
Andre Przywara
AMD-OSRC (Dresden)
Tel: x29712

Re: [PATCH] tools/check: check for yajl (needed by libxl)

[Ian Campbell]

On Wed, 2011-10-05 at 09:24 +0100, Andre Przywara wrote:
> On 10/04/2011 05:00 PM, Ian Campbell wrote:
> > On Tue, 2011-10-04 at 15:48 +0100, Andre Przywara wrote:
> >> On 09/30/2011 12:39 PM, Ian Campbell wrote:
> >>> On Fri, 2011-09-30 at 11:35 +0100, Anthony PERARD wrote:
> >>>> On Fri, Sep 30, 2011 at 08:36, Ian Campbell<ian.campbell@citrix.com>   wrote:
> >>>>   ...
> >>>> You probably want to check the yajl headers as well, no ?
> >>>> #include<yajl/yajl_parse.h>
> >>>> #include<yajl/yajl_gen.h>
> >>>
> > ...
> >> Also wouldn't it be nicer to link this small library statically? This
> >> would omit the runtime requirement at all.
> >> The test team just spelled yajl as "yet another library to get Xen
> >> running"...
> >
> > I don't think this is necessary, it's just a library you need to install
> > and for most users the packaging system will transparently cover this.
> > All the usual reasons for not statically linking stuff apply here.
> > There's nothing special about that wrt Xen and/or the size of the
> > library.
> 
> Unfortunately this package is not omnipresent in the distributions. 
> Though I find it already in Fedora 13, it is not in the official RHEL5 
> repos, also not in anything before OpenSuSE 11.4 and before SLES11SP2. 
> This creates quite some hassle for any people using one of these systems 
> to build and run Xen. And pulling it from github will push most people 
> (like me ;-) into version 2 in the first place.

Hrm, that's unfortunate. It seems to be the only even vaguely widespread
JSON library for C so I'm not sure there was much alternative here given
that we have to speak JSON in order to speak QMP to upstream Qemu.

AIUI it is usually sufficient to find either a binary RPM from a closely
related distro (i.e. a newer version of said distro or e.g. OpenSUSE for
SLES) or to find a .src.rpm and run rpmbuild on it. I don't suppose
these distros have the equivalent of Debian's backports.org? If
necessary we should have a page on the wiki which gives pointers to
where to obtain these libraries.

> But I guess this will not convince you to drop it from unstable. That's 
> why I thought static linking

But to statically link you must have already obtained the library from
somewhere, right? So statically linking doesn't really help.

>  (or including in the tree?)

Absolutely not.

IMHO the Xen repos should not be ingesting/bundling every project or
library which we want to make use of. We have done this in the past
(i.e. libaio) and it was a mistake.

There is nothing wrong with a project having dependencies on libraries
and other various software. We should strive to clearly document these
dependencies but we should not feel obliged to import them into the Xen
repos.

Including stuff in the Xen tree makes it harder for our downstreams
(i.e. distros, who want to use the existing packages of a library not
some bundled version) to package Xen in a sane way and adds to
maintenance burden, both for us and them.

> >> Also be advised that it looks like this is relying on version 1 of yajl.
> >> It looks like the new version 2.0.1 is not compatible, I got compile
> >> errors with the newer version. If the stable version 1.x is required,
> >> this should be stated somewhere or at least checked properly in check_yajl.
> >
> > Any idea what we could test for here?
> 
> The sanest way would be to do it like configure and check explicitly for 
> the data structure that we rely on, but I guess just looking for a 
> symlink called libyajl.so.1 in /usr/lib64 should suffice here.
> 
> 
> >>> diff -r 302b7556edd9 -r de602616358b tools/check/check_yajl_lib
> >>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> >>> +++ b/tools/check/check_yajl_lib	Fri Sep 30 11:39:35 2011 +0100
> >>> @@ -0,0 +1,6 @@
> >>> +#!/bin/sh
> >>> +# CHECK-BUILD CHECK-INSTALL
> >>> +
> >>> +. ./funcs.sh
> >>> +
> >>> +has_lib libyajl.so || fail "can't find libyajl.so"
> 
> +has_lib libyajl.so.1 || fail "can't find libyajl.so version 1"

Thanks, I'll regroove the patch with that and the other suggestion about
CHECK-INSTALL.

Ian.

[PATCH] tools/check: check for yajl (needed by libxl)

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1317807757 -3600
# Node ID be6610d3477ebd77eb37c5961bf19a7afd31676f
# Parent  af62449dc68e952b8efbbb8302bc9ef0c801c9f9
tools/check: check for yajl (needed by libxl)

We need precisely version 1 so check for .so.1. In the future we should handle
yajl v2 as well.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
(note to committer, ensure the new file is executable)

diff -r af62449dc68e -r be6610d3477e tools/check/check_yajl_devel
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/check/check_yajl_devel	Wed Oct 05 10:42:37 2011 +0100
@@ -0,0 +1,8 @@
+#!/bin/sh
+# CHECK-BUILD
+
+. ./funcs.sh
+
+has_header yajl/yajl_parse.h || fail "can't find yajl/yajl_parse.h"
+has_header yajl/yajl_gen.h || fail "can't find yajl/yajl_gen.h"
+has_lib libyajl.so || fail "can't find libyajl.so"
diff -r af62449dc68e -r be6610d3477e tools/check/check_yajl_lib
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/check/check_yajl_lib	Wed Oct 05 10:42:37 2011 +0100
@@ -0,0 +1,6 @@
+#!/bin/sh
+# CHECK-BUILD CHECK-INSTALL
+
+. ./funcs.sh
+
+has_lib libyajl.so.1 || fail "can't find libyajl.so.1 version 1"

Re: [PATCH] tools/check: check for yajl (needed by libxl)

[Olaf Hering]

On Wed, Oct 05, Ian Campbell wrote:

> AIUI it is usually sufficient to find either a binary RPM from a closely
> related distro (i.e. a newer version of said distro or e.g. OpenSUSE for
> SLES) or to find a .src.rpm and run rpmbuild on it. I don't suppose
> these distros have the equivalent of Debian's backports.org? If
> necessary we should have a page on the wiki which gives pointers to
> where to obtain these libraries.

The openSuSE Buildservice has almost everything:

http://software.opensuse.org/search?q=libyajl1&baseproject=SUSE%3ASLE-11%3ASP1&include_home=true&exclude_debug=true


Olaf

Re: [PATCH] tools/check: check for yajl (needed by libxl)

[Ian Jackson]

Ian Campbell writes ("[Xen-devel] [PATCH] tools/check: check for yajl (needed by libxl)"):
> tools/check: check for yajl (needed by libxl)

Thanks everyone.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>