Hi again, Dan Nicholson <[email protected]> (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.
signature.asc
Description: Digital signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
