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

[Dan Nicholson]

On Sun, Mar 6, 2011 at 8:29 AM, Cyril Brulebois <kibi at debian.org> wrote:
> 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.

Ah, I didn't know that. I'll keep that in mind if the dictiionary stays.

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

No big rush. I wasn't the only one to review this patch, so maybe
other people will chime in.

--
Dan