Merge lp://staging/~wookey/xdeb/xdeb-installbuilt into lp://staging/xdeb

+++ Wookey [2011-02-24 00:01 -0000]:
> Wookey has proposed merging lp:~wookey/xdeb/xdeb-installbuilt into lp:xdeb.
>

This has been updated to include current trunk. Could someone take a
look and comment or merge it?

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

Could you explain the use case a bit, I'm not sure I understand it properly; I can see two cases:
* xdeb builds some stuff, but fails to install it for some reason, and this isn't retried
* xdeb builds and installs stuff, user uninstalls it, xdeb fails to reinstall it

Is it one of these cases that you wanted to address with these changes?

I'd rather not change "native architecture" to "build architecture" in this patch; "native" is used quite consistenly in xdeb, so if we fix this we would want to fix this globally; I don't think that's very urgent though, and it would be quite messy to change.

+++ Loïc Minier [2011-03-16 01:59 -0000]:
> Could you explain the use case a bit, I'm not sure I understand it properly; I can see two cases:
> * xdeb builds some stuff, but fails to install it for some reason, and this isn't retried
> * xdeb builds and installs stuff, user uninstalls it, xdeb fails to reinstall it
>
> Is it one of these cases that you wanted to address with these changes?

Almost. If you ask to build package foo then it's buidl-deps are
instaled/crossed as required, but it itself is built, but not installed
(correctly).

If you then ask to build something depending on foo it notes that it
has been built and incorrectly assumes this means it has been
installed, so carries on and the build either fails or has stuff
missing. This fix simply teaches xdeb that 'built' != 'installed'.

> I'd rather not change "native architecture" to "build architecture"
> in this patch; "native" is used quite consistenly in xdeb, so if we
> fix this we would want to fix this globally; I don't think that's very
> urgent though, and it would be quite messy to change.

OK. Yes. I'm keep to have that revamp to remove the
contrary-to-multiarch naming, but I agree it should be done everywhere
at once, and there are more important things to fix.

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

Sorry for the delay in reviewing; comments below

> --- xdeb.py 2011-03-16 01:52:32 +0000
> +++ xdeb.py 2011-04-06 15:39:28 +0000
> @@ -166,6 +166,9 @@
> builddep_pkgs = {}
> depends = {}
> needs_build = {}
> +needs_install = {}
> +install_version = {}
> +
> # set of graph edges in dependency tree
> dot_relationships = dict()
>
> @@ -343,6 +346,9 @@
> print "%s already built at version %s" % (src,
> built_vers[-1])
> newer = False
> + install_version[src] = built_vers[-1]
> + if check_if_src_needs_install(src, built_vers[-1], options):
> + needs_install[src] = True
> else:
> newer = True
> else:

 Purely cosmetic, but would you think check_if_src_needs_install would
 be more intuitively named is_changes_installed, check_src_installed, or
 check_all_installed? (Obviously the semantics would be reversed with
 such a name)

 The name "install_version" isn't too intuitive either as it's really
 about "versions to install" for the packages in needs_install; I wonder
 whether it should simply a single list with everything in it:
    need_install = []
    [...]
    needs_install.append({'source': src, 'version': built_vers[-1]})
 but perhaps a simpler solution is to have check_if_src_needs_install()
 return a .changes file or directly .debs and change install_built_pkg()
 to take .changes / .debs instead.

> @@ -656,6 +662,9 @@
>
> outdir = os.path.normpath(os.path.join(
> tree.get_src_directory(options, src), '..'))
> + install_built_pkg(src, ver_no_epoch, options, outdir)
> +
> +def install_built_pkg(src, ver_no_epoch, options, outdir, force=True):
> changes = '%s_%s_%s.changes' % (src, ver_no_epoch, options.architecture)
> build_log = '%s_%s_%s.build' % (src, ver_no_epoch, options.architecture)
> debs = get_output(['dcmd', '--deb', changes], cwd=outdir).splitlines()

 Looks fine

> @@ -705,7 +714,30 @@
>
> update_apt_repository(options)
>
> -
> +def check_if_src_needs_install(src, ver, options):
> + ver_no_epoch = re_no_epoch.sub('', str(install_version[src]))
> + changes = '%s_%s_%s.changes' % (src, ver_no_epoch, options.architecture)
> + debs = get_output(['dcmd', '--deb', changes], cwd=options.destdir).splitlines()
> + install_reqd = False
> + for deb in debs:
> + pkg = re_package.sub(r"\1", deb)
> + if pkg in all_builddeps:
> + print "builddep: %s" % pkg
> + pkg_cross = "%s-%s-cross" % (pkg, options.architecture)
> + try:
> + if not aptutils.cache[pkg].is_installed:
> + if options.debug:
> + print ("% is not installed" % pkg)
> + install_reqd = True
> + if not aptutils.cache[pkg_cross].is_installed:
> + if options.debug:
> + print ("% is not installed" % pkg)
> + install_reqd = True
> + except:
> + print "failed to find pkg in cache:" + pkg
> + install_...