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

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!

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

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

> + 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 :-) Do you really need to do both 'erase' and 'flash'?

> + self.deployment_data = Target.android_deployment_data
> + self.deployment_data['boot_image'] = boot
> +
> + def power_on(self):
> + self.fastboot(['reboot'])
> + sleep(10) # wait for the bootloader to reboot
> + self.fastboot(['boot', self.deployment_data['boot_image']])
> + self.adb(['wait-for-device'])
> + proc = self.adb(['shell'], spawn = True)
> + proc.sendline("") # required to put the adb shell in a reasonable state
> + proc.sendline("export PS1='%s'" % self.deployment_data['TESTER_PS1'])
> + return proc
> +
> + def reboot(self):
> + # tell android to reboot. A failure probably means that the device is not
> + # booted on android, and we ignore that.
> + self.adb(['reboot'], ignore_failure = True)
> +
> + # TODO implement power_off
> +
> + # TODO implement file_system
> +
> + # TODO implement extract_tarball
> +
> + # TODO implement get_device_version
> +
> + # TODO implement get_test_data_attachments (??)
> +
> + def adb(self, args, ignore_failure = False, spawn = False):
> + cmd = ['sudo', 'adb'] + args
> + if spawn:
> + return logging_spawn(" ".join(cmd), timeout = 60)
> + else:
> + self._call(cmd, ignore_failure)
> +
> + def fastboot(self, args, ignore_failure = False):
> + self._call(['sudo', 'fastboot'] + args, ignore_failure)
> +
> + def _call(self, cmd, ignore_failure):
> + if ignore_failure:
> + subprocess.call(cmd)
> + else:
> + subprocess.check_call(cmd)
> +
> +target_class = NexusTarget