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

[Cyril Brulebois]

Hi again,

Dan Nicholson <dbn.lists at gmail.com> (06/03/2011):
> 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.

unfortunately, I might need some time to look into it, I'm not sure I
want to update my tinderbox setups (which is how I tested my various
patches) right before possibly deploying them elsewhere.

Anyway, a few quick comments follow.

> 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.

Agreed.

> 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()

Go for portability, ACK.

> > +# 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.

Whatever works best in your opinion; I must confess I didn't give it
too much thought, I only maintain tinderboxes for x86-64 right now. :)

It looks like it would do the right thing, a quick test says:
$ python -c "import platform; print platform.machine()"
x86_64
$ linux32 python -c "import platform; print platform.machine()"
i686

so we should be fine.

> > +# 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']

Your test is actually incorrect (the dict contains lists):
| >>> skip=[]
| >>> foo=['geode']
| >>> skip.extend(foo)
| >>> skip
| ['geode']

For a single addition, I guess append() is OK. extend() just made it
possible to add a whole list in a single call. From python's doc:
| list.append(x)
| Add an item to the end of the list; equivalent to a[len(a):] = [x].
vs.
| list.extend(L)
| Extend the list by appending all the items in the given list; equivalent to a[len(a):] = L.


> 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")

As said above, didn't check, but probably better than the ugly hackery
I initially proposed.

KiBi.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg-devel/attachments/20110306/c715bbff/attachment.pgp>