Merge lp:~wookey/xdeb/xdeb-archfix into lp:xdeb

[Wookey]

Use correct architecture apt cache when calculating dependencies.
Ensure --only-explicit builds corresponding source package if binary package specified
Improve debug feedback

[Loïc Minier (lool)]

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

[Loïc Minier (lool)]

review: Needs Fixing

[Steve Langasek (vorlon)]

-
- ver = all_srcs[src]
+ try:
+ ver = all_srcs[src]
+ except Keyerror:
+ recover

exception names are case-sensitive; this should be KeyError, not Keyerror

What is 'recover' meant to be? That's not a python keyword - do you mean 'pass' here? In the previous review, Loïc commented that hitting this would be a bug in xdeb if it ever happened; if you do mean 'pass', why do we want to ignore the exception instead of letting it raise and generate a backtrace? (If you didn't mean 'pass', I'm not sure what you did mean here.)

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

This was previously nacked by Loïc out of concern that mixing binary and source packages on the commandline will give unexpected and confusing results. Please drop this change, or provide a specific rationale why this is a necessary feature. (Also, if this isn't critical path for making xdeb work on source packages, then it's a new feature and requires a freeze exception.)

review: Needs Fixing

[Wookey (wookey)]

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

[Steve Langasek (vorlon)]

On Thu, Oct 07, 2010 at 11:13:39PM -0000, Wookey wrote:

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

You could write it this way:

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

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

It can, but I think Loïc will agree with me that any use of 2-char indents
would be rejected. :) 2-char isn't a deep enough indent to let developers
scan the code efficiently when reading.

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

IMHO the point is not that xdeb shouldn't be helpful, but that any such
helpfulness should not have built-in ambiguity - as is the case when trying
to decide whether a package name refers to a source or a binary package.

Regardless, this is a new feature, however small, and subject to the feature
freeze. I think this should be deferred to the next cycle and implemented
cleanly without ambiguous side-effects.

--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>

[Loïc Minier (lool)]

On Thu, Oct 07, 2010, Wookey wrote:
> 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()

    bin_cache = get_output(['apt-cache',
                            '-oAPT::Architecture=%s' % options.architecture,
                            'show',
                            binary]).splitlines()

 Or:
    apt_cache = ['apt-cache', 'show', binary]
    apt_cache.append('-oAPT::Architecture=%s' % options.architecture)
    bin_cache = get_output(apt_cache).splitlines()

 (untested)

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

 If you're playing these tricks, you have indented too much and need to
 refactor for readibility; moving to 2-char indents deteriorates
 readibility

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

 Because maintaining the piece of logic which knows when native imports
 are going to happen is not going to be easy over time; the closer the
 test is to the use of the variable, the better

 I think we should just arrange for this situation not to happen by
 providing the right native_import URLs in the default config or have
 some good fallback mechanism

> 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

 this means that if you pass linux to the above, you'll actually
 cross-build linux-meta. If you take both package types (source and
 binary), you can break expectations both ways, but if you only expect
 source packages you can't break expectations; also, it means we can
 save us this subtlety in xdeb which has experienced users to start with
 (as you just demonstrated)

--
Loïc Minier

[lp:~wookey/xdeb/xdeb-archfix]

237. By Wookey on 2010-10-08

Adjust patch according to review
Add native_import_sources for i386 and amd64

238. By Wookey on 2010-10-08

remove accidental change to filter_arch call

=== modified file 'aptutils.py'
--- aptutils.py	2010-10-01 11:09:56 +0000
+++ aptutils.py	2010-10-08 03:27:45 +0000
@@ -58,6 +58,7 @@
         filename = matchobj.group(1)
         if filename.endswith('_Sources'):
             sources.append(filename)
+            print "Using file %s for apt cache" % filename
 
     for source in sources:
         try:

=== modified file 'debian/changelog'
--- debian/changelog	2010-10-01 11:09:56 +0000
+++ debian/changelog	2010-10-08 03:27:45 +0000
@@ -1,3 +1,12 @@
+xdeb (0.6.3) UNRELEASED; urgency=low
+
+  * Use correct architecture apt cache when determining dependencies
+    Fixes LP:#616617
+  * Fix --only-explicit code to build corresponding source packages
+    when binary names suppplied
+
+ -- Wookey <[e-mail]>  Tue, 28 Sep 2010 18:05:12 +0100
+
 xdeb (0.6.2) UNRELEASED; urgency=low
 
   * Clean up indentation in xdeb.py and changelog.

=== modified file 'xdeb.cfg'
--- xdeb.cfg	2010-08-01 20:29:46 +0000
+++ xdeb.cfg	2010-10-08 03:27:45 +0000
@@ -123,12 +123,14 @@
 
 [i386]
 parent: baseline
+native_import_source: http://archive.ubuntu.com/ubuntu/
 
 [target-i386-generic]
 parent: i386
 
 [amd64]
 parent: baseline
+native_import_source: http://archive.ubuntu.com/ubuntu/
 
 [target-amd64-generic]
 parent: amd64

=== modified file 'xdeb.py'
--- xdeb.py	2010-10-01 11:09:56 +0000
+++ xdeb.py	2010-10-08 03:27:45 +0000
@@ -205,6 +205,8 @@
                 src = realsrc
     src_record = get_src_record(options, src)
     if not src_record:
+        if options.debug:
+            print "Did not find a source record for %s" % src 
         return
 
     if parent is not None and parent != src:
@@ -269,7 +271,8 @@
     """Decide whether a package needs to be (re)built."""
     if src in needs_build:
         return
-
+    #moan if we end up getting a binary package name here
+    assert src in all_srcs
     ver = all_srcs[src]
     newer = False
 
@@ -444,10 +447,17 @@
 
     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',
+                                    '-oAPT::Architecture=%s' % options.architecture, 
+                                    'show', binary]).splitlines()
         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:
@@ -464,6 +474,9 @@
                 if deb_bits.group(3) == build_arch:
                     deb = re_deb_filename.sub(
                         r'\1_\2_%s.deb' % options.architecture, deb)
+                assert target_config.native_import_source, \
+                       "No native_import_source configured for arch %s" \
+                       % options.architecture
                 spawn(['wget', '-N',
                        '%s/%s' % (target_config.native_import_source, deb)],
                       cwd=options.builddirs[0])
@@ -491,6 +504,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)
@@ -765,13 +779,19 @@
     for pkg in args:
         expand_depends(options, pkg, 0, 0)
 
+    for pkg in args:
+        if pkg not in all_srcs:
+            print "No source package found: %s" % pkg
+            sys.exit(1)
+
     if options.only_explicit:
         # In --only-explicit mode, we don't need to do a full topological
         # sort (which relieves us from concerns of dependency cycles and the
         # 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(pkg)
     else:
         try:
             build_sequence = tsort.topo_sort(depends)
@@ -816,11 +836,16 @@
         install_native_import(options, native_crossed)
 
     for src in build_sequence:
+        if options.debug:
+            print "Considering source package %s" % src
         if needs_build[src]:
             if src in real_build:
                 build(options, src, all_srcs[src])
             elif not options.only_explicit:
                 install_native_import(options, native_import(options, src))
+        else:
+            if options.debug:
+                print "Skipping %s (already built)" % src
 
 if __name__ == '__main__':
     main()