[PATCH modular 2/3 (v3)] jhbuild: Support skipping packages on a per-architecture basis.

[Dan Nicholson]

On Tue, Nov 16, 2010 at 05:43:46PM +0100, Cyril Brulebois wrote:
> Thanks to an execfile() in jhbuildrc, allow managing packages to be
> skipped on this or that architecture to be listed in an external file.
> 
> Signed-off-by: Cyril Brulebois <kibi at debian.org>
> ---
>  arch.skip |   24 ++++++++++++++++++++++++
>  jhbuildrc |    8 +++++++-
>  2 files changed, 31 insertions(+), 1 deletions(-)
>  create mode 100644 arch.skip

Now that I've spent some more time with jhbuild, I have some comments that
I'd like to get your feedback on. Sorry this is so long after the 3(!)
previous reviews.

> diff --git a/arch.skip b/arch.skip
> new file mode 100644
> index 0000000..eba055d
> --- /dev/null
> +++ b/arch.skip
> @@ -0,0 +1,24 @@
> +# This module is supposed to be execfile()'d from jhbuildrc. It appends
> +# elements to the 'skip' variable depending on the detected architecture.
> +#
> +# For code clarity's sake, some variables are used in this file. To prevent
> +# jhbuild from complaining about keys it doesn't know about, they are prefixed
> +# with an underscore.
> +#
> +# To determine your architecture, run this:
> +#     python -c 'import os; print os.uname()[4]'
> +#
> +# There might be better ways to detect the architecture. Patches welcome.

First, I think it might be better to just stuff this code into our
jhbuildrc instead of keeping it in a separate file. When you first
proposed this, I didn't put it together that any python was possible
from jhbuildrc. Having the code in jhbuildrc means you don't have a
potentially broken reference to an external file.

> +
> +import os
> +_current_arch = os.uname()[4]

I think it would be better to use python's platform module. Here's the
doc on the two modules.

http://docs.python.org/library/os.html
http://docs.python.org/library/platform.html

According to the first, os.uname is only available on unix, whereas
platform is available everywhere for the things we want. The equivalent
to what you have is:

import platform
_current_arch = platform.machine()

> +# Dictionary: arch => list of packages to skip
> +_arch_blacklist = {
> +  'x86_64': ['xf86-video-geode',  # not on 64-bit   (#26341)
> +            ],
> +}

I think a dictionary might not be the way to go. Take geode for example.
Since you also can't build it on sparc or arm or anything else, you'd
have to repeat this dictionary entry for all other arches possible.
Instead, I think we should just check when we're not on i*86 and skip
then.

> +
> +# No package to skip if the architecture isn't known:
> +if _arch_blacklist.has_key(_current_arch):
> +    skip.extend(_arch_blacklist[_current_arch])

Last, I don't think extend does what we want.

>>> skip = []
>>> skip.extend('xf86-video-geode')
>>> skip
['x', 'f', '8', '6', '-', 'v', 'i', 'd', 'e', 'o', '-', 'g', 'e', 'o', 'd', 'e']
>>> skip = []
>>> skip.append('xf86-video-geode')
>>> skip
['xf86-video-geode']

Here's what I'm using in my x86_64 jhbuildrc:

import platform
import re
_machine = platform.machine()
if not re.match("i.86", _machine):
        skip.append("xf86-video-geode")

What do you think?

--
Dan