Code review comment for lp:~wookey/xdeb/xdeb-archfix

+++ Loïc Minier [2010-10-07 22:25 -0000]:
> On Thu, Oct 07, 2010, Wookey wrote:
> > @@ -269,8 +271,10 @@
> > """Decide whether a package needs to be (re)built."""
> > if src in needs_build:
> > return
> > -
> > - ver = all_srcs[src]
> > + try:
> > + ver = all_srcs[src]
> > + except Keyerror:
> > + recover
> > newer = False
> >
> > if not options.force_rebuild and not force:
>
> It's not Keyerror but KeyError; "recover" doesn't exist either; it was
> just an illustration I gave when I wrote:
> try:
> stuff
> except KeyError:
> recover
> which was just meant to be the place where you type the recovery code

oops. Can you tell I don't know much python yet :-) I assumed it was
some magic that would just DTRT. Did wonder what it actually did :-)

> If you want to add safety checks for programming errors in xdeb, you
> want asserts:
> assert src in all_srcs
>
> But I'd rather not add any since in this case a KeyError will be
> thrown, and that's as easy to understand as an assertion error like the
> above.

Alright, I'll get rid of it, but it was certainly useful for me.

> > @@ -444,10 +448,15 @@
> >
> > debs = []
> > for binary in aptutils.get_src_binaries(src):
> > + if options.debug:
> > + print "Considering binary %s" % binary
> > try:
> > - bin_cache = get_output(['apt-cache', 'show', binary]).splitlines()
> > + apt_opts = '-oAPT::Architecture=%s' % options.architecture
> > + bin_cache = get_output(['apt-cache', apt_opts, 'show', binary]).splitlines()
>
> wrap on multiple lines?

Can I wrap on a dot or only on spaces? I thought about it but thought
            bin_cache = get_output(['apt-cache', apt_opts, 'show',
            binary]).splitlines()

was harder to read

and assumed that
            bin_cache = get_output(['apt-cache', apt_opts, 'show', binary])
     .splitlines()

probably wouldn't work.

Can python use 2-char indenting instead of 4? That would save a load
of space.

> > + # until we get this automatically from apt sources one has to be
> > + # specified in the config
> > + if ((options.only_explicit or options.apt_source) and
> > + not target_config.native_import_source):
> > + sys.exit("No apt source available for architecture: "+
> > + "%s. Specify one in config or command line"
> > + % options.architecture)
> > if not options.builddirs:
> > options.builddirs = ['.']
> > if options.destdir is None:
>
> I still think this reads nebulously and should be fixed differently;
> I would be fine if you'd fail at the time you realize the download URL
> is empty, e.g. just before the wget, assert
> target_config.native_import_source and define the url for the other
> arches in the default config

Why is it better to wait until a duff download is attempted rather
than check for invalid combination of options after reading the command
line and config options? Checking up front saves waiting until the
fault actually occurs, so the user gets immediate feedback. I suppose
it's possible that no native downloads would be attempted so the issue
wouldn't trigger - is that your thinking?

> > @@ -771,7 +788,8 @@
> > # like); we just need to make sure that native imports happen first
> > # and then that explicit requests happen in command-line order.
> > build_sequence = [d for d in depends if d not in explicit_requests]
> > - build_sequence.extend(args)
> > + for pkg in args:
> > + build_sequence.append(get_src_name(options, pkg))
> > else:
> > try:
> > build_sequence = tsort.topo_sort(depends)
>
> I'd still like to not allow specifying binary packages on the
> command-line; people using xdeb certainly know the difference IMNSHO

I thought the whole point about xdeb was to be a helpful tool. If you
as a user want to cross-build 'foo', why should you have to look up
the source package that provides foo when the tool can do it for you so
trivially?

To run pdebuild-cross on a package from the alip package list I have to do this:
sourcepackage=apt-cache show $package | awk '/Source: .*/ {print $2}'
if [ -n sourcepackage ]; then sourcepackage=$package; fi
pdebuild-cross $sourcepackage

for xdeb I can just do
xdeb <options> $package

isn't that better?

I'll take it out if you insist, but I don't understand the objection.

Wookey
--
Principal hats: Linaro, Emdebian, Wookware, Balloonboard, ARM
http://wookware.org/