Patchwork [1/2] pixman: require 0.16.4 as minimum version

login
register
mail settings
Submitter Gerd Hoffmann
Date Dec. 10, 2012, 12:48 p.m.
Message ID <1355143684-16850-2-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/204865/
State New
Headers show

Comments

Gerd Hoffmann - Dec. 10, 2012, 12:48 p.m.
Lower the bar a bit.  0.16.4 is known-good, and is shipped by debian.
Fixes build failures on the debian-based buildbot slaves.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 configure |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Alexander Graf - Dec. 13, 2012, 2:31 p.m.
On 10.12.2012, at 13:48, Gerd Hoffmann wrote:

> Lower the bar a bit.  0.16.4 is known-good, and is shipped by debian.
> Fixes build failures on the debian-based buildbot slaves.

SLES11 ships 0.16.0 for example. It'd be nice to stay compatible there.

What exactly is the compatibility bar? The fact that it compiles? Without the configure check it's compiling just fine on 0.12.0 (openSUSE 11.1) here.


Alex

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> configure |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/configure b/configure
> index e5aedef..a4e62c4 100755
> --- a/configure
> +++ b/configure
> @@ -2127,7 +2127,7 @@ fi
> # pixman support probe
> 
> if test "$pixman" = ""; then
> -  if $pkg_config --atleast-version=0.18.4 pixman-1 > /dev/null 2>&1; then
> +  if $pkg_config --atleast-version=0.16.4 pixman-1 > /dev/null 2>&1; then
>     pixman="system"
>   else
>     pixman="internal"
> @@ -2138,7 +2138,7 @@ if test "$pixman" = "system"; then
>   pixman_libs=`$pkg_config --libs pixman-1 2>/dev/null`
> else
>   if test ! -d ${source_path}/pixman/pixman; then
> -    echo "ERROR: pixman not present (or older than 0.18.4). Your options:"
> +    echo "ERROR: pixman not present (or older than 0.16.4). Your options:"
>     echo "  (1) Prefered: Install the pixman devel package (any recent"
>     echo "      distro should have packages as Xorg needs pixman too)."
>     echo "  (2) Fetch the pixman submodule, using:"
> -- 
> 1.7.1
> 
>
Gerd Hoffmann - Dec. 13, 2012, 2:48 p.m.
On 12/13/12 15:31, Alexander Graf wrote:
> 
> On 10.12.2012, at 13:48, Gerd Hoffmann wrote:
> 
>> Lower the bar a bit.  0.16.4 is known-good, and is shipped by
>> debian. Fixes build failures on the debian-based buildbot slaves.
> 
> SLES11 ships 0.16.0 for example. It'd be nice to stay compatible
> there.

If 0.16.0 is known good we can pick that one.

> What exactly is the compatibility bar? The fact that it compiles?
> Without the configure check it's compiling just fine on 0.12.0
> (openSUSE 11.1) here.

Hmm,  /me saw reports of *not* building with 0.12.0, which was the
reason to add this in the first place ...

confused,
  Gerd
Alexander Graf - Dec. 13, 2012, 2:50 p.m.
On 13.12.2012, at 15:48, Gerd Hoffmann wrote:

> On 12/13/12 15:31, Alexander Graf wrote:
>> 
>> On 10.12.2012, at 13:48, Gerd Hoffmann wrote:
>> 
>>> Lower the bar a bit.  0.16.4 is known-good, and is shipped by
>>> debian. Fixes build failures on the debian-based buildbot slaves.
>> 
>> SLES11 ships 0.16.0 for example. It'd be nice to stay compatible
>> there.
> 
> If 0.16.0 is known good we can pick that one.
> 
>> What exactly is the compatibility bar? The fact that it compiles?
>> Without the configure check it's compiling just fine on 0.12.0
>> (openSUSE 11.1) here.
> 
> Hmm,  /me saw reports of *not* building with 0.12.0, which was the
> reason to add this in the first place ...

Yeah, check out

  6e72719e721a40fe1224701ca10edc1caf0cd708

You had 2 branches that were using a new flag, one of which you had an #ifdef around and one of which you didn't. This is all fixed for quite a while now :).

If that's the culprit for all this mess, can we remove the configure check again? QEMU compiles just fine without it today on pixman 0.12.0.


Alex
Alexander Graf - Dec. 13, 2012, 2:56 p.m.
On 13.12.2012, at 15:48, Gerd Hoffmann wrote:

> On 12/13/12 15:31, Alexander Graf wrote:
>> 
>> On 10.12.2012, at 13:48, Gerd Hoffmann wrote:
>> 
>>> Lower the bar a bit.  0.16.4 is known-good, and is shipped by
>>> debian. Fixes build failures on the debian-based buildbot slaves.
>> 
>> SLES11 ships 0.16.0 for example. It'd be nice to stay compatible
>> there.
> 
> If 0.16.0 is known good we can pick that one.
> 
>> What exactly is the compatibility bar? The fact that it compiles?
>> Without the configure check it's compiling just fine on 0.12.0
>> (openSUSE 11.1) here.
> 
> Hmm,  /me saw reports of *not* building with 0.12.0, which was the
> reason to add this in the first place ...

So basically it boils down to support for PIXMAN_TYPE_BGRA. How about we just check for that one instead rather than the version number? Also, it'd be very consistent if the #ifdefery in qemu-pixman.c gets removed if that's the whole purpose of the configure check.


Alex
Gerd Hoffmann - Dec. 13, 2012, 3:30 p.m.
Hi,

> 6e72719e721a40fe1224701ca10edc1caf0cd708

It's broken, version is wrong, and I told you.
Seems you sneaked it in nevertheless.

> You had 2 branches that were using a new flag, one of which you had
> an #ifdef around and one of which you didn't. This is all fixed for
> quite a while now :).

Can you check which of your pixman versions have PIXMAN_TYPE_BGRA?

0.18.4 has it
0.12.0 has not

The 0.16.x  versions would be interesting so we can fix the #ifdef to be
correct (or at least correct enougth for all practical purposes).

> If that's the culprit for all this mess, can we remove the configure
> check again? QEMU compiles just fine without it today on pixman
> 0.12.0.

If you send me a fix for 6e72719e721a40fe1224701ca10edc1caf0cd708 I'll
go revert 288fa40736e6eb63132d01aa6dc21ee831b796ae

cheers,
  Gerd
Alexander Graf - Dec. 13, 2012, 4:10 p.m.
On 13.12.2012, at 16:30, Gerd Hoffmann wrote:

>  Hi,
> 
>> 6e72719e721a40fe1224701ca10edc1caf0cd708
> 
> It's broken, version is wrong, and I told you.
> Seems you sneaked it in nevertheless.

It's consistent now with the #ifdef 5 lines above the line that the patch does.

> 
>> You had 2 branches that were using a new flag, one of which you had
>> an #ifdef around and one of which you didn't. This is all fixed for
>> quite a while now :).
> 
> Can you check which of your pixman versions have PIXMAN_TYPE_BGRA?
> 
> 0.18.4 has it
> 0.12.0 has not
> 
> The 0.16.x  versions would be interesting so we can fix the #ifdef to be
> correct (or at least correct enougth for all practical purposes).

0.16.0 does.

> 
>> If that's the culprit for all this mess, can we remove the configure
>> check again? QEMU compiles just fine without it today on pixman
>> 0.12.0.
> 
> If you send me a fix for 6e72719e721a40fe1224701ca10edc1caf0cd708 I'll
> go revert 288fa40736e6eb63132d01aa6dc21ee831b796ae

6e72719 is perfectly sane, because it brings consistency into the file. Now both users of PIXMAN_TYPE_BGRA are guarded by the same #ifdef. I do agree though that instead of

#if SOME_MAGIC_VERSION

we rather either do

#ifdef PIXMAN_TYPE_BGRA

or no #ifdef at all and a proper configure check.

Having a configure check and the #ifdef is wrong. Doing #ifdef MAGIC_VERSION is also wrong.


So a quick fix would be to bump the configure check to 0.16.0 and remove the two #ifdefs in qemu-pixman.c.


Alex
Gerd Hoffmann - Dec. 14, 2012, 7:54 a.m.
Hi,

>> If you send me a fix for 6e72719e721a40fe1224701ca10edc1caf0cd708
>> I'll go revert 288fa40736e6eb63132d01aa6dc21ee831b796ae
> 
> 6e72719 is perfectly sane, because it brings consistency into the
> file. Now both users of PIXMAN_TYPE_BGRA are guarded by the same
> #ifdef.

Both users?  Hello?  Its two *different* types: PIXMAN_TYPE_RGBA and
PIXMAN_TYPE_BGRA.  And, yes, they have been added in different versions.
 So guarding them with the *same* #ifdef is wrong.

> #ifdef PIXMAN_TYPE_BGRA

It's a enum, not a #define, so this isn't going to fly.

cheers,
  Gerd
Alexander Graf - Dec. 14, 2012, 9:57 a.m.
On 14.12.2012, at 08:54, Gerd Hoffmann <kraxel@redhat.com> wrote:

>  Hi,
> 
>>> If you send me a fix for 6e72719e721a40fe1224701ca10edc1caf0cd708
>>> I'll go revert 288fa40736e6eb63132d01aa6dc21ee831b796ae
>> 
>> 6e72719 is perfectly sane, because it brings consistency into the
>> file. Now both users of PIXMAN_TYPE_BGRA are guarded by the same
>> #ifdef.
> 
> Both users?  Hello?  Its two *different* types: PIXMAN_TYPE_RGBA and
> PIXMAN_TYPE_BGRA.  And, yes, they have been added in different versions.
> So guarding them with the *same* #ifdef is wrong.

Ugh :(. Sorry then.

> 
>> #ifdef PIXMAN_TYPE_BGRA
> 
> It's a enum, not a #define, so this isn't going to fly.

In my old versions it's a define...


Alex

> 
> cheers,
>  Gerd
>

Patch

diff --git a/configure b/configure
index e5aedef..a4e62c4 100755
--- a/configure
+++ b/configure
@@ -2127,7 +2127,7 @@  fi
 # pixman support probe
 
 if test "$pixman" = ""; then
-  if $pkg_config --atleast-version=0.18.4 pixman-1 > /dev/null 2>&1; then
+  if $pkg_config --atleast-version=0.16.4 pixman-1 > /dev/null 2>&1; then
     pixman="system"
   else
     pixman="internal"
@@ -2138,7 +2138,7 @@  if test "$pixman" = "system"; then
   pixman_libs=`$pkg_config --libs pixman-1 2>/dev/null`
 else
   if test ! -d ${source_path}/pixman/pixman; then
-    echo "ERROR: pixman not present (or older than 0.18.4). Your options:"
+    echo "ERROR: pixman not present (or older than 0.16.4). Your options:"
     echo "  (1) Prefered: Install the pixman devel package (any recent"
     echo "      distro should have packages as Xorg needs pixman too)."
     echo "  (2) Fetch the pixman submodule, using:"