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

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

 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.

> @@ -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?

> except Exception:
> - continue # might be a udeb
> + if options.debug:
> + print "skipping - %s" % binary
> + continue # might be a udeb or not built for specified arch
> bin_stanzas = deb822.Packages.iter_paragraphs(bin_cache)
> while True:
> try:
> @@ -491,6 +500,7 @@
> return crossed_debs
>
> def install_native_import(options, crossed_debs):
> + print "Installing native packages: %s" % crossed_debs
> if crossed_debs:
> install = ['dpkg', '-i']
> install.extend(crossed_debs)
> @@ -727,6 +737,13 @@
> else:
> setattr(options, name, value)
>
> + # 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

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

 Thanks for fixing the other things I had raised

--
Loïc Minier