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/143385
>
> Hi,
>
> This time is for real. :-)
>
> I don't see any pending issues, so know you can have a hard review on the code.
> -- 
> https://code.launchpad.net/~terceiro/lava-dispatcher/nexus/+merge/143385
> You are subscribed to branch lp:lava-dispatcher.
> === modified file 'lava_dispatcher/config.py'
> --- lava_dispatcher/config.py	2013-01-10 21:37:58 +0000
> +++ lava_dispatcher/config.py	2013-01-15 19:17:23 +0000
> @@ -91,6 +91,9 @@
>      arm_probe_config = schema.StringOption(default='/usr/local/etc/arm-probe-config')
>      arm_probe_channels = schema.ListOption(default=['VDD_VCORE1'])
>  
> +    adb_command = schema.StringOption()
> +    fastboot_command = schema.StringOption()
> +    nexus_working_directory = schema.StringOption(default = None)

It's super nitty, but "default=None" (i.e. without the spaces) would be
more conventional.

>  class OptionDescriptor(object):
>      def __init__(self, name):
>
> === added file 'lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf'
> --- lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf	1970-01-01 00:00:00 +0000
> +++ lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf	2013-01-15 19:17:23 +0000
> @@ -0,0 +1,40 @@
> +client_type = nexus
> +
> +# The ADB command line.
> +#
> +# In the case where there are more than one android devices plugged into a

s/are/is/ or s/more than one/multiple/ (English grammar makes no sense)

> +# single host, this connection command must be overriden on each device to
> +# include the serial number of the device, e.g.
> +#
> +#   serial_number = XXXXXXXXXXXXXXXX
> +#   adb_command = adb -s %(serial_number)s
> +adb_command = adb
> +
> +# The fastboot command.
> +#
> +# The same as above: if you have more than one device, you will want to
> +# override this in your device config to add a serial number, e.g.
> +#
> +#   serial_number = XXXXXXXXXXXXXXXX
> +#   fastboot_command = sudo fastboot -s %(serial_number)s
> +#
> +# Of course, in the case you override both adb_command *and* fastboot_command,
> +# you don't need to specify `serial_number` twice.
> +fastboot_command = sudo fastboot
> +
> +# Working directory for temporary. By default, the usual place for LAVA images
> +# will be used.
> +#
> +# This is useful when the lava dispatcher is controlling Nexus phones that are
> +# physically plugged to other machines. In this case, you should set
> +# nexus_work_directory to a shared directory between the machine running the
> +# dispatcher and the machine where the phone is plugged.

I don't entirely understand when this would be useful.

> +# OBS: this shared directory must have the same path in both machines.

OBS?

> +nexus_working_directory =
> +
> +connection_command = %(adb_command)s shell
> +
> +enable_network_after_boot_android = false
> +android_adb_over_usb = true
> +android_adb_over_tcp = false
>
> === added file 'lava_dispatcher/device/nexus.py'
> --- lava_dispatcher/device/nexus.py	1970-01-01 00:00:00 +0000
> +++ lava_dispatcher/device/nexus.py	2013-01-15 19:17:23 +0000
> @@ -0,0 +1,168 @@
> +# 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
> +import logging
> +import contextlib
> +
> +from lava_dispatcher.device.target import (
> +    Target
> +)
> +from lava_dispatcher.downloader import (
> +    download_image
> +)
> +from lava_dispatcher.utils import (
> +    logging_spawn,
> +    mkdtemp
> +)

OK, so this is probably mostly the dispatcher's fault for being
confusing, but there are a few things I don't really understand here.
If I get you to explain them to me in excruciating detail, maybe we can
make the code clearer for the next person.

> +class NexusTarget(Target):
> +
> +    def __init__(self, context, config):
> +        super(NexusTarget, self).__init__(context, config)
> +        self._powered_on = False

This seems like an assumption.  How likely is it that the device is
actually powered down?  If it's important that it is, maybe we should
power it down explicitly (although probably not in __init__).

> +        self._working_dir = None
> +
> +    def deploy_android(self, boot, system, userdata):
> +
> +        boot = self._get_image(boot)
> +        system = self._get_image(system)
> +        userdata = self._get_image(userdata)
> +
> +        self.reboot_os()

So, let me get this straight.  There are a few states we could be in:

1. At fastboot.
2. Powered down.
3. In android.
4. Hung.

We want to be in the fastboot state.  Maybe we should call a function
that makes that clear?  I guess I'm saying that you should name
functions in this class by the state they result the device being in,
rather than what they do, "intent revealing names" and all that.  In
addition, I think I'd find this clearer if the internal methods had
_underscore names.

If we're in state "4. Hung" we're doomed?

> +        self.fastboot('erase boot')
> +
> +        self.fastboot('flash system %s' % system)
> +        self.fastboot('flash userdata %s' % userdata)
> +
> +        self.deployment_data = Target.android_deployment_data
> +        self.deployment_data['boot_image'] = boot
> +
> +    def power_on(self):
> +        self.reboot_os()
> +        self.reboot_bootloader()

I don't understand why reboot_bootloader is here.  It seems to me that
we're in the same situation as deploy_android -- we don't know the state
and we want to be in fastboot.

> +        self.boot_test_image()
> +
> +        self._powered_on = True
> +        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'])
> +        self._runner = self._get_runner(proc)
> +
> +        return proc
> +
> +    def reboot_os(self):
> +        # tell android to reboot. A failure probably means that the device is not
> +        # booted on android, and we ignore that.

It would be nice not to make assumptions here.

> +        self.adb('reboot', ignore_failure = True)
> +        sleep(10)

Are these sleeps really necessary?  AIUI, issuing a fastboot command
will wait for the device if it's not yet ready for it.

> +    def reboot_bootloader(self):
> +        self.fastboot('reboot')
> +        sleep(10)
> +
> +    def boot_test_image(self):
> +        self.fastboot('boot %s' % self.deployment_data['boot_image'])
> +        self.adb('wait-for-device')
> +
> +    def power_off(self, proc):
> +        # there is no way to power off the Nexus while USB is plugged on; even
> +        # if you remove power, it will stay on.
> +        pass

:( Maybe we'll get some of Andy Green's usb disconnector devices.

> +    @contextlib.contextmanager
> +    def file_system(self, partition, directory):
> +
> +        if not self._powered_on:
> +            self.power_on()
> +
> +        mount_point = self.get_partition_mount_point(partition)
> +
> +        with self.make_filesystem_readwrite(mount_point):

I /thought/ we generally didn't make modifications to /system.  But I
guess you wouldn't have done this unless it was needed...

> +            host_dir = '%s/mnt/%s' % (self.working_dir, directory)
> +            target_dir = '%s/%s' % (mount_point, directory)
> +
> +            subprocess.check_call(['mkdir', '-p', host_dir])
> +            self.adb('pull %s %s' % (target_dir, host_dir), ignore_failure = True)
> +
> +            yield host_dir
> +
> +            self.adb('push %s %s' % (host_dir, target_dir))
> +
> +    def get_partition_mount_point(self, partition):
> +        lookup = {
> +            self.config.data_part_android_org: '/data',
> +            self.config.sys_part_android_org: '/system',
> +        }
> +        return lookup[partition]
> +
> +    @contextlib.contextmanager
> +    def make_filesystem_readwrite(self, mount_point):
> +        if mount_point  == '/system':
> +            self._runner.run("mount -o remount,rw %s" % mount_point)
> +        yield
> +        if mount_point  == '/system':
> +            self._runner.run("mount -o remount,ro %s" % mount_point)
> +
> +    def get_device_version(self):
> +        # this is tricky, because fastboot does not have a visible version
> +        # number. For now let's use just the adb version number.
> +        return subprocess.check_output(
> +            "%s version | sed 's/.* version //'" % self.config.adb_command,
> +            shell = True
> +        ).strip()
> +
> +    def adb(self, args, ignore_failure = False, spawn = False):
> +        cmd = self.config.adb_command + ' ' + args
> +        if spawn:
> +            return logging_spawn(cmd, timeout = 60)
> +        else:
> +            self._call(cmd, ignore_failure)
> +
> +    def fastboot(self, args, ignore_failure = False):
> +        self._call(self.config.fastboot_command + ' ' + args, ignore_failure)
> +
> +    def _call(self, cmd, ignore_failure):
> +        logging.debug("Running on the host: %s" % cmd)
> +        if ignore_failure:
> +            subprocess.call(cmd, shell = True)
> +        else:
> +            subprocess.check_call(cmd, shell = True)

It would be nice to have a timeout for all calls somehow.  Not sure how, though.

> +    def _get_image(self, url):
> +        sdir = self.working_dir
> +        image = download_image(url, self.context, sdir, decompress=False)
> +        return image
> +
> +    @property
> +    def working_dir(self):
> +        if (self.config.nexus_working_directory is None or
> +            self.config.nexus_working_directory.strip() == ''):
> +            return self.scratch_dir
> +
> +        if self._working_dir is None:
> +            self._working_dir = mkdtemp(self.config.nexus_working_directory)
> +        return self._working_dir
> +
> +
> +target_class = NexusTarget

Sorry for all the questions!  Having a slow-brained day today it seems.

Cheers,
mwh