Code review comment for lp:~terceiro/lava-dispatcher/nexus

On Sun, Dec 02, 2012 at 10:16:20PM -0000, Michael Hudson-Doyle wrote:
> Antonio Terceiro <email address hidden> writes:
>
> > Antonio Terceiro has proposed merging lp:~terceiro/lava-dispatcher/nexus into lp:lava-dispatcher.
> >
> > Requested reviews:
> > Linaro Validation Team (linaro-validation)
> >
> > For more details, see:
> > https://code.launchpad.net/~terceiro/lava-dispatcher/nexus/+merge/136807
> >
> > This is work in progress; you will note the number of FIXME's and TODO's in lava_dispatcher/device/nexus.py. ;-)
>
> This looks pretty good on the whole!

Thanks! :)

> > With the current state, I can already run a full boot test job, like the one in the commit message of rev 475:
> >
> > http://bazaar.launchpad.net/~terceiro/lava-dispatcher/nexus/revision/475
> >
> > It would be nice to receive comments about the changes that were needed to other files outside of the Nexus implementation itself.
>
> I think they mostly seem reasonable. I'd be happy to see them merged
> without the nexus bits if that would make your life easier.

I will merge them, in that case. A cleaner diff will make my life a
little easier, for sure.

> > === added file 'lava_dispatcher/device/nexus.py'
> > --- lava_dispatcher/device/nexus.py 1970-01-01 00:00:00 +0000
> > +++ lava_dispatcher/device/nexus.py 2012-11-28 22:45:26 +0000
> > @@ -0,0 +1,106 @@
> > +# Copyright (C) 2012 Linaro Limited
> > +#
> > +# Author: Antonio Terceiro <email address hidden>
> > +#
> > +# This file is part of LAVA Dispatcher.
> > +#
> > +# LAVA Dispatcher is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# LAVA Dispatcher is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along
> > +# with this program; if not, see <http://www.gnu.org/licenses>.
> > +
> > +import subprocess
> > +import pexpect
> > +from time import sleep
> > +from lava_dispatcher.device.target import (
> > + Target
> > +)
> > +from lava_dispatcher.downloader import (
> > + download_image
> > +)
> > +from lava_dispatcher.utils import (
> > + logging_spawn
> > +)
> > +
> > +class NexusTarget(Target):
> > +
> > + def __init__(self, context, config):
> > + super(NexusTarget, self).__init__(context, config)
> > +
> > + def deploy_android(self, boot, system, userdata):
> > + sdir = self.scratch_dir
> > +
> > + # TODO it seems boot, system and userdata are usually .tar.gz files,
> > + # and I am currently assuming they are .img and flashing them directly
>
> Would be good to talk to the android team about what to do here I guess.

OK, will do it.

> > + boot = download_image(boot, self.context, sdir, decompress=False)
> > + # FIXME uncomment these two - skipping them makes testing faster
> > + #system = download_image(system, self.context, sdir, decompress=False)
> > + #userdata = download_image(userdata, self.context, sdir, decompress=False)
> > +
> > + self.reboot()
> > + sleep(10)
> > +
> > + self.fastboot(['erase', 'boot'])
> > + # FIXME uncomment these two - skipping them makes testing faster
> > + #self.fastboot(['erase', 'system'])
> > + #self.fastboot(['erase', 'userdata'])
> > +
> > + # FIXME uncomment these two - skipping them makes testing faster
> > + #self.fastboot(['flash', 'system', system])
> > + #self.fastboot(['flash', 'userdata', userdata])
>
> I suggest that you don't and the branch with these bits commented out
> though :-)

Sure. :-)

> Do you really need to do both 'erase' and 'flash'?

no, I don't.

--
Antonio Terceiro
Software Engineer - Linaro
http://www.linaro.org