850572 - [Tracking] Cleanup And organize mobile (sut) code.

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Justin Wood (:Callek)]

So I'm filing this as a tracker for sake of argument, the order and specifics of the sub-tasks are mostly up to whomever does the work.

The basic plan here is "cleanup sut_tools/* and buildfarm/mobile/*"

The generic concepts are:

* Remove dead (and unused) code
 ** We have many scripts (and some files) both in sut_tools/ and buildfarm/mobile/* that we consider obsolete at this point, and can be removed, which cleans clutter.
 ** Along this vein we have functions/classes that are also unused/dead

* Factor out libraries from scritps
 ** sut_tools/ contains both libraries (sut_lib mostly) and scripts.
 ** many scripts (e.g. cleanup.py) are imported by other scripts (e.g. verify.py) we should keep a clear seperation between script and library

* Move libraries (and mozdevice) module to be in the library section of tools/ (lib/python)
 ** This is creating a sut_tools directory there, as well as a mozdevice directory (latter is just the clean import)
 ** We would need to utilize the site.addpath magic as used elsewhere in tools in any "top level script" (e.g. `path.join(path.dirname(path.realpath(__file__)), "../../lib/python"))` )

* Factor libraries out into distinct sub-units.
  ** This is mostly up-to-the-doer for the API to use, but ideas include "sut_tools/__init__.py" as a transitional file, for "import sut_tools" to continue to work as-is
  ** .. then things like "power.py" so you can do the power-management stuff here, etc.
  ** A lot of this will involved a brief understanding of what and why we do what we do.

* [enh] Improve logging code, by making all logging initiated by the top-level script that we use, rather than from sut_lib itself.
* [enh] Make all sut code not require an environment variable set for the specific device it is working on (e.g. SUT_NAME)
* [enh] update mozdevice code to a newer current release while here (it brings with it API incompats, including using python raise instead of return codes in some areas, so will need a diffed-audit.

The enh parts I don't consider needed to be tracked by this generic work-idea, but do present food-for-thought and future direction/work if desired.

I suspect the specific sub-bugs can be filed by whomever is going to do this work, once they have a rough plan-of-attack in mind, but I'm willing to file all the sub-bugs myself if we want me to play conductor :-)

:coop, once you figure out who you plan to assign this to [hence needinfo], I'll chat with them to give a brief overview/history and to get any Q's they have and answer them :-)

If you'd rather I play-conductor just let me know

Comment 1

[Chris Cooper [:coop] (he/him)]

We discussed this in our 1x1 today, but for posterity, I'm fine with Pete finally the specific dependent bugs for this as he unravels it.

Comment 2

[Pete Moore [:pmoore][:pete]]

Attachment: First round of cleanup

This first patch contains the following changes:

    * sut_lib has moved into its own package, under lib/python
    * all modules referencing sut_lib have been updated to load it from the new location, by dynamically changing the sys path (rather than requiring a PYTHONPATH update)
    * fix to cleanup.py; see below
    * fix to config.py; see below
    * unused imports have been removed
    * unused variables have been removed
    * overly long lines have been wrapped
    * comparison to True / False / None changed to use 'is' rather than '=='

Bugs fixed:

    * variable 'errorFile' was referenced, but did not exist, in cleanup.py.

The variable has now been created, and points to the error.flg file for the referenced device.

    * method 'dumpException' was called, but did not exist, in config.py

It is now using the dumpException method found in the sut_lib module.

Comment 3

[Pete Moore [:pmoore][:pete]]

Attachment: Consistent line endings

DOS style line endings converted to unix in python code, for the sake of consistency.

Comment 4

[Justin Wood (:Callek)]

Comment on attachment 761366 [details] [diff] [review]
Consistent line endings

Review of attachment 761366 [details] [diff] [review]:
-----------------------------------------------------------------

rubber stamping this one. :-)

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 761365 [details] [diff] [review]
First round of cleanup

Review of attachment 761365 [details] [diff] [review]:
-----------------------------------------------------------------

rest of review will come a bit later, wanted to leave this part now.

::: lib/python/sut_lib/__init__.py
@@ +1,1 @@
> +#!/usr/bin/env python

first nit, bugzilla shows this as (new file) with the old location (removed file)... what we want is moved, so that it brings the annotation/blame along with it. -- I know this display can be a factor of how git generates patches so just make sure mercurial shows it correctly when landing. (I'm poor with git, but can help on the mercurial side if its not displaying properly when you're ready)

Comment 6

[Justin Wood (:Callek)]

Comment on attachment 761365 [details] [diff] [review]
First round of cleanup

Review of attachment 761365 [details] [diff] [review]:
-----------------------------------------------------------------

It's worth updating http://mxr.mozilla.org/build/source/mozharness/mozharness/base/log.py#81 (which is just a comment) but no rush on that one.

We'll need to update PYTHONPATH here http://mxr.mozilla.org/build/source/tools/buildfarm/mobile/watch_devices.sh#39 I forget how its seperated (likely unix ":") but we can just add /builds/tools/lib/python to it and then it should should work.

r- mostly due to the watch_devices issue, otherwise this looks pretty good.

We might want to consider forcing a purge of sut_lib.pyc after we deploy this, with a temporarily puppet patch (for say a half day)... but not as worried about that :-)

::: sut_tools/check.py
@@ +14,2 @@
>  
>  # from multiprocessing import get_logger, log_to_stderr

while here, lets drop this dead-code comment

@@ +14,4 @@
>  
>  # from multiprocessing import get_logger, log_to_stderr
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), "../lib/python"))

Please instead use (has the same end result but doesn't change actual PATH):
 import site
 site.addsitedir(os.path.join(os.path.dirname(os.path.realpath(__file__)),
                 "../lib/python")
the .realpath is to be *sure* that symlinked sut_tools will find the right place in /builds/tools/


Then we can remove sys from imports for this file :-)

@@ +252,5 @@
>  
>      if options.tegra is None:
>          for f in os.listdir(options.bbpath):
> +            if os.path.isdir(os.path.join(options.bbpath, f)) \
> +               and 'tegra-' in f.lower():

for this spot only, lets leave the slightly longer line. (I don't think this line continuation is any better, and the 80-char pep8 thing is our recommendation but not a hard req :-)

::: sut_tools/cleanup.py
@@ +7,5 @@
>  import os
>  import sys
>  from mozdevice import devicemanagerSUT as devicemanager
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), "../lib/python"))

Use `site` here as well

@@ +24,5 @@
>      if errcode == 2:
>          log.error("processes from previous run were detected and cleaned up")
>      elif errcode == 3:
> +        pidDir = os.path.join('/builds/', device)
> +        errorFile = os.path.join(pidDir, 'error.flg')

*great* catch :-)

::: sut_tools/config.py
@@ +6,5 @@
>  import random
>  import socket
>  from mozdevice import devicemanagerSUT as devicemanager
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), "../lib/python"))

use `site` here as well.

@@ +8,5 @@
>  from mozdevice import devicemanagerSUT as devicemanager
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), "../lib/python"))
> +
> +from sut_lib import soft_reboot_and_verify, dumpException

good catch on dumpException ;-)

::: sut_tools/installApp.py
@@ +7,4 @@
>  import zipfile
>  from mozdevice import devicemanagerSUT as devicemanager
>  
> +sys.path.append(os.path.join(os.path.dirname(__file__), "../lib/python"))

`site` again

@@ +169,3 @@
>          sys.exit(1)
>  
>      # N.B. 3rd arg not used anywhere

Good find, but while here please trim the above "usage" string as well, to not advertise processName

::: sut_tools/reboot.py
@@ +3,5 @@
>  import os
>  import sys
>  from mozdevice import devicemanagerSUT as devicemanager
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), "../lib/python"))

`site`

@@ +23,5 @@
>          if sname.strip():
>              deviceName = sname.strip()
>          else:
> +            log.info("Unable to find a proper devicename, will attempt to \
> +                reboot device")

python trick:
  log.info("Unable to find a proper devicename, will attempt to "
           "reboot device")
will collapse the two strings together

Where what you wrote is actually output as:
 "Una....ll attempt to                reboot device"
(note the large space)

::: sut_tools/smoketest.py
@@ +8,5 @@
>  from mozdevice import devicemanagerSUT as devicemanager
>  import subprocess
>  import re
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), "../lib/python"))

`site`

@@ +25,5 @@
>      # TODO: fix the host/port information so we don't have conflicts in
>      # parallel runs
>      cmd = ["python",
> +           os.path.join(os.path.dirname(__file__),
> +           "tests/mochitest/runtestsremote.py"),

I personally think un-split here is better, but if you must split please indent to the opening bracket of os.path.join so we don't think its a seperate arg for the cmd.

@@ +30,5 @@
>             "--app=%s" % proc_name,
>             "--deviceIP=%s" % device,
>             "--xre-path=%s" % os.path.join(os.path.dirname(__file__), "xre"),
> +           "--utility-path=%s" % os.path.join(os.path.dirname(__file__),
> +                                              "bin"),

for 5 chars easier to leave it on the same line imo.

@@ +41,5 @@
>          p = subprocess.Popen(cmd, stdout=subprocess.PIPE)
>          proc = p.communicate()[0]
>      except:
> +        log.error("Exception found while running unittests: %s"
> +                  % sys.exc_info()[1])

personal pref of same line again :-)

@@ +54,5 @@
>              numfailed = int(failed.group(2))
>          if finished:
>              if numfailed > 0:
> +                log.error("Found %s failures while running mochitest"
> +                          % numfailed)

here too

@@ +83,5 @@
> +          % (dm, dm._sock)
> +    if installOneApp(dm, deviceRoot, os.path.abspath(appFileName), None,
> +                     logcat=False):
> +        log.error("failed to install %s on device %s"
> +                  % (appFileName, device_name))

I'm not as big a fan of the line length forcing on these lines, but these are actually close enough to too long that I can defer to your personal opinion.

::: sut_tools/stop.py
@@ +10,4 @@
>  import signal
>  import logging
>  
> +sys.path.append(os.path.join(os.path.dirname(__file__), "../lib/python"))

`site`

::: sut_tools/tegra_checkstalled.py
@@ +3,3 @@
>  import sys
>  
> +sys.path.append(os.path.join(os.path.dirname(__file__), "../lib/python"))

`site`

::: sut_tools/tegra_powercycle.py
@@ +6,5 @@
>  
>  import os
>  import sys
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), "../lib/python"))

`site`

::: sut_tools/verify.py
@@ +7,5 @@
>  import sys
>  import os
>  import time
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), "../lib/python"))

`site`

Comment 7

[Justin Wood (:Callek)]

Comment on attachment 761365 [details] [diff] [review] [diff] [review]
First round of cleanup

---------AUTOMATIC COMMENT---------
--  filter pep8-callek-june-2013 --
---------AUTOMATIC COMMENT---------

$pep8 <dir> --diff --max-line-length=159 --show-source < attachment.diff
/tmp/tmp.C3CAHir6kx/lib/python/sut_lib/__init__.py:218:160: E501 line too long (236 > 159 characters)
    # 18456     1 18455 /opt/local/Libra   ??  S      0:00.88 /opt/local/Library/Frameworks/Python.framework/Versions/2.6/Resources/Python.app/Contents/MacOS/Python /opt/local/Library/Frameworks/Python.framework/Versions/2.6/bin/twistd$
                                                                                                                                                               ^

Comment 8

[Pete Moore [:pmoore][:pete]]

Attachment: Refactoring of sut_lib into a new package with creation of new modules, code cleanup, and additional code commenting

Hey Callek,

This is the bulk of the changes, including:

* creation of a sut_lib package under lib/python
* creation of a new powermanagement module
* creation of a new devices module
* several cleanup operations
* a couple of bug fixes where variables were referenced but not defined
* a change to use the DMError class for mozdevice errors
* code styling changes
* conversion of python scripts to unix line endings where they were DOS
* removal of some dead code

At the moment, files that were updated and moved are shown as deleted/new since I created this patch in git - maybe we can discuss how this should be adapted for the mercurial patch, and I can resubmit with the correct "mv" syntax so we can preserve history.

Thanks,
Pete

Comment 9

[Pete Moore [:pmoore][:pete]]

Attachment: Upgrade of mozdevice (not including changes to calling code since we should go through the testing of this together)

Hi Callek,

This patch contains the actual migration of mozdevice to the most recent version (0.26). Please note the first patch took care of making a couple of minor forward compatible changes, so that the old or the new mozdevice version could be used.

For example, the exception class AgentError is deprecated in the new version of mozdevice, so any AgentErrors were replaced in the last patch with the already existing generic DMError, so that a change to mozdevice could be transplanted or removed without problems. Also the last patch took care of updating the library path at runtime to include the lib/python directory, and the imports of mozdevice were moved *after* the library path update, so that if mozdevice is upgraded, it will already be in the path. Therefore this change is also not needed in this patch, since the last patch handles the forward compatibility of this (i.e. where mozdevice was imported before the change to the python library path, it was placed after, so that by the time mozdevice is imported, the full library path has been set including also the lib/python directory).

This patch does not include changes to calling code (required since failure exit codes have been replaced with python exceptions) since we need to go through the testing strategy of this together, before those changes are made, tested, and applied.

However, this is the major part of the mozdevice upgrade that is already available for review and initial feedback. :)

Thanks,
Pete

Comment 10

[Pete Moore [:pmoore][:pete]]

Attachment: An optional patch for IDE config

Comment 11

[Pete Moore [:pmoore][:pete]]

Comment on attachment 766837 [details] [diff] [review]
An optional patch for IDE config

It is up for discussion whether this is generally useful. Essentially this is the config file used by the Eclipse IDE for the tools project, using the pydev plugin. I am happy to share this config (which is not specific

Comment 12

[Pete Moore [:pmoore][:pete]]

... to my user).

Comment 13

[Justin Wood (:Callek)]

Comment on attachment 766836 [details] [diff] [review] [diff] [review]
Upgrade of mozdevice (not including changes to calling code since we should go through the testing of this together)

---------AUTOMATIC COMMENT---------
--  filter pep8-callek-june-2013 --
---------AUTOMATIC COMMENT---------

Full Pep8 output was inappropriately long for a bugzilla comment
Please run locally for more information

$pep8 <dir> --diff --max-line-length=159 --statistics < attachment.diff

6       E111 indentation is not a multiple of four
13      E124 closing bracket does not match visual indentation
1       E125 continuation line does not distinguish itself from next logical line
4       E127 continuation line over-indented for visual indent
7       E128 continuation line under-indented for visual indent
106     E201 whitespace after '{'
84      E202 whitespace before '}'
26      E203 whitespace before ':'
5       E221 multiple spaces before operator
4       E222 multiple spaces after operator
2       E225 missing whitespace around operator
24      E231 missing whitespace after ':'
43      E251 unexpected spaces around keyword / parameter equals
22      E261 at least two spaces before inline comment
2       E301 expected 1 blank line, found 0
32      E302 expected 2 blank lines, found 1
2       E303 too many blank lines (2)
3       E501 line too long (162 > 159 characters)
2       E502 the backslash is redundant between brackets
2       E701 multiple statements on one line (colon)
7       E703 statement ends with a semicolon
17      E711 comparison to None should be 'if cond is None:'
1       E712 comparison to False should be 'if cond is False:' or 'if not cond:'
3       W291 trailing whitespace
41      W293 blank line contains whitespace
1       W391 blank line at end of file
1       W603 '<>' is deprecated, use '!='

Comment 14

[Justin Wood (:Callek)]

Comment on attachment 766827 [details] [diff] [review] [diff] [review]
Refactoring of sut_lib into a new package with creation of new modules, code cleanup, and additional code commenting

---------AUTOMATIC COMMENT---------
--  filter pep8-callek-june-2013 --
---------AUTOMATIC COMMENT---------

$pep8 <dir> --diff --max-line-length=159 --show-source < attachment.diff

/tmp/tmp.NhkIEhZqJv/lib/python/sut_lib/__init__.py:198:160: E501 line too long (236 > 159 characters)
    # 18456     1 18455 /opt/local/Libra   ??  S      0:00.88 /opt/local/Library/Frameworks/Python.framework/Versions/2.6/Resources/Python.app/Contents/MacOS/Python /opt/local/Library/Frameworks/Python.framework/Versions/2.6/bin/twistd$
                                                                                                                                                               ^
/tmp/tmp.NhkIEhZqJv/sut_tools/config.py:114:160: E501 line too long (164 > 159 characters)
                setFlag(errorFile, "Remote Device Error: current resolution X:%d Y:%d does not match what was set X:%d Y:%d" % (width, height, refWidth, refHeight))
                                                                                                                                                               ^

Comment 15

[Justin Wood (:Callek)]

Comment on attachment 766837 [details] [diff] [review]
An optional patch for IDE config

Review of attachment 766837 [details] [diff] [review]:
-----------------------------------------------------------------

IFF we take this, we should take it in a seperate bug (imo). And I don't personally care for it, but if at least one other person agrees its useful in-tree I won't block on it. (so feel free to get someone else's review :-), elsewhere)

Comment 16

[Justin Wood (:Callek)]

Comment on attachment 766827 [details] [diff] [review]
Refactoring of sut_lib into a new package with creation of new modules, code cleanup, and additional code commenting

Review of attachment 766827 [details] [diff] [review]:
-----------------------------------------------------------------

Need to fix http://mxr.mozilla.org/build/source/tools/buildfarm/mobile/watch_devices.sh#40 specifically make sure PYTHONPATH is updated on the line prior to *also* point at the new sut_lib location.

--- 
Overall a pretty good patch, a few minor things before I'm comfortable landing this given all the testing I know you already did!

::: buildfarm/maintenance/manage_foopies.py
@@ -1,1 @@
> -#!/usr/bin/env python

for self-notes, this files changes are strictly line-ending whitespace, not actually substantive.

::: lib/python/sut_lib/__init__.py
@@ +475,5 @@
> +    log.info("Setting device time to %s" % s)
> +    try:
> +        dm._runCmds([{'cmd': 'settime %s' % s}])
> +        return True
> +    except devicemanager.DMError, e:

note the switch of AgentError to DMError is not directly swappable right now.

current mozdevice has both, but neither inherit each other so if we catch DMError here, where we used to catch AgentError if the code raises AgentError we'll fail.

A syntax change that would work for both is:

+    except (devicemanager.AgentError, devicemanager.DMError), e:

::: lib/python/sut_lib/devices.py
@@ +14,5 @@
> +
> +allDevices = loadDevicesData('/builds/tools/buildfarm/mobile')
> +if len(allDevices) == 0:
> +    allDevices = loadDevicesData(
> +        os.path.join(os.path.dirname(__file__), '../buildfarm/mobile'))

this path is wrong now, correct [untested] should be:

os.path.join(os.path.dirname(os.path.realpath(__file__)), '../../../buildfarm/mobile')

::: sut_tools/installApp.py
@@ +157,4 @@
>                  setFlag(errorFile, "Remote Device Error: Resolution change failed.  Should be %d/%d but is %d/%d" % (1024, 768, width, height))
>                  return None, None
>  
> +    except devicemanager.DMError, err:

same issue about AgentError vs DMError here

::: sut_tools/relay.py
@@ -1,1 @@
> -#!/usr/bin/python

this file should move wholesale to the sut_lib directory. Even though it can be called directly its meant to be called as part of our sut_lib powermanagement. (e.g. from relay .... there)

::: sut_tools/smoketest.py
@@ +1,1 @@
> +#!/usr/bin/env python

self-comment: since this file is not used in automation directly (but we still want to keep it here) I merely skimmed it.

::: sut_tools/verify.py
@@ +264,4 @@
>      try:
>          dm._runCmds([{'cmd': 'push %s %s' % (tmpname, len(
>              watcherINI)), 'data': watcherINI}])
> +    except devicemanager.DMError, err:

same comments about AgentError vs DMError (and elsewhere in this file)

Comment 17

[Pete Moore [:pmoore][:pete]]

(In reply to Justin Wood (:Callek) from comment #13)
> Comment on attachment 766836 [details] [diff] [review]
> Upgrade of mozdevice (not including changes to calling code since we should
> go through the testing of this together)
> 
> ---------AUTOMATIC COMMENT---------
> --  filter pep8-callek-june-2013 --
> ---------AUTOMATIC COMMENT---------
> 
> Full Pep8 output was inappropriately long for a bugzilla comment
> Please run locally for more information
> 
> $pep8 <dir> --diff --max-line-length=159 --statistics < attachment.diff
> 
> 6       E111 indentation is not a multiple of four
> 13      E124 closing bracket does not match visual indentation
> 1       E125 continuation line does not distinguish itself from next logical
> line
> 4       E127 continuation line over-indented for visual indent
> 7       E128 continuation line under-indented for visual indent
> 106     E201 whitespace after '{'
> 84      E202 whitespace before '}'
> 26      E203 whitespace before ':'
> 5       E221 multiple spaces before operator
> 4       E222 multiple spaces after operator
> 2       E225 missing whitespace around operator
> 24      E231 missing whitespace after ':'
> 43      E251 unexpected spaces around keyword / parameter equals
> 22      E261 at least two spaces before inline comment
> 2       E301 expected 1 blank line, found 0
> 32      E302 expected 2 blank lines, found 1
> 2       E303 too many blank lines (2)
> 3       E501 line too long (162 > 159 characters)
> 2       E502 the backslash is redundant between brackets
> 2       E701 multiple statements on one line (colon)
> 7       E703 statement ends with a semicolon
> 17      E711 comparison to None should be 'if cond is None:'
> 1       E712 comparison to False should be 'if cond is False:' or 'if not
> cond:'
> 3       W291 trailing whitespace
> 41      W293 blank line contains whitespace
> 1       W391 blank line at end of file
> 1       W603 '<>' is deprecated, use '!='

Hey Callek,

These are the mozdevice changes in the upgrade to 0.26. I think it might be best if we leave it the same as it is upstream, to avoid divergence. What do you think?

Thanks,
Pete

Comment 18

[Pete Moore [:pmoore][:pete]]

(In reply to Justin Wood (:Callek) from comment #14)
> Comment on attachment 766827 [details] [diff] [review]
> Refactoring of sut_lib into a new package with creation of new modules, code
> cleanup, and additional code commenting
> 
> ---------AUTOMATIC COMMENT---------
> --  filter pep8-callek-june-2013 --
> ---------AUTOMATIC COMMENT---------
> 
> $pep8 <dir> --diff --max-line-length=159 --show-source < attachment.diff
> 
> /tmp/tmp.NhkIEhZqJv/lib/python/sut_lib/__init__.py:198:160: E501 line too
> long (236 > 159 characters)
>     # 18456     1 18455 /opt/local/Libra   ??  S      0:00.88
> /opt/local/Library/Frameworks/Python.framework/Versions/2.6/Resources/Python.
> app/Contents/MacOS/Python
> /opt/local/Library/Frameworks/Python.framework/Versions/2.6/bin/twistd$
>                                                                             
> ^
> /tmp/tmp.NhkIEhZqJv/sut_tools/config.py:114:160: E501 line too long (164 >
> 159 characters)
>                 setFlag(errorFile, "Remote Device Error: current resolution
> X:%d Y:%d does not match what was set X:%d Y:%d" % (width, height, refWidth,
> refHeight))
>                                                                             
> ^

Hi Callek,

These two overly long lines highlighted in this pep8 report lie outside of the patches I've provided.

The first is a python comment which provides example output from a "ps" command. I think it is beneficial to leave this output unaltered, since it serves as an example of expected output. To change it could be misleading.

The second pep8 objection raised above, I can fix, but I think it shouldn't block the landing of any existing patches, since it is an already existing issue. Happy to provide an additional patch to clean this line up though.

Let me know your thoughts!
Pete

Comment 19

[Pete Moore [:pmoore][:pete]]

Comment on attachment 766827 [details] [diff] [review]
Refactoring of sut_lib into a new package with creation of new modules, code cleanup, and additional code commenting

Review of attachment 766827 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Callek, I've responded to some of your comments inline...

::: lib/python/sut_lib/devices.py
@@ +14,5 @@
> +
> +allDevices = loadDevicesData('/builds/tools/buildfarm/mobile')
> +if len(allDevices) == 0:
> +    allDevices = loadDevicesData(
> +        os.path.join(os.path.dirname(__file__), '../buildfarm/mobile'))

It seems correct to me, e.g. on foopy46 (as a random example):

[[e-mail] ~]$ ls -l /builds/tools/buildfarm/mobile
total 172
-rwxrwxr-x 1 cltbld cltbld    110 Jan  3 16:36 check.sh
-rw-rw-r-- 1 cltbld cltbld 143457 Jun 14 13:21 devices.json
-rw-rw-r-- 1 cltbld cltbld   1897 Feb  6 08:09 devices_per_foopy.py
-rwxrwxr-x 1 cltbld cltbld    398 Jan  3 16:36 kill_stalled.sh
-rwxrwxr-x 1 cltbld cltbld   1872 May 22 12:35 manage_buildslave.sh
-rwxrwxr-x 1 cltbld cltbld    593 Jan  3 16:36 tegra_stats.sh
-rwxrwxr-x 1 cltbld cltbld   4260 Jun 11 03:21 watch_devices.sh
[[e-mail] ~]$ 

This was transplanted from the original sut_tools/sut_lib.py - so this is also how it was working until now.

::: sut_tools/relay.py
@@ -1,1 @@
> -#!/usr/bin/python

How about the functions move into sut_lib, but the
    if __name__ == '__main__':
block remains here? I'd prefer not to introduce runnable scripts into sut_lib, since sut_lib contains just libraries, and tools are in sut_tools.

Let me know your thoughts.

Comment 20

[Pete Moore [:pmoore][:pete]]

Comment on attachment 766827 [details] [diff] [review]
Refactoring of sut_lib into a new package with creation of new modules, code cleanup, and additional code commenting

Review of attachment 766827 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/python/sut_lib/devices.py
@@ +14,5 @@
> +
> +allDevices = loadDevicesData('/builds/tools/buildfarm/mobile')
> +if len(allDevices) == 0:
> +    allDevices = loadDevicesData(
> +        os.path.join(os.path.dirname(__file__), '../buildfarm/mobile'))

I've just realised you were talking about line 18 - so you are quite right! When I read your comment I thought you were referring to line 15! Sorry, my bad! Will fix. :)

Comment 21

[Pete Moore [:pmoore][:pete]]

Attachment: Updated patch for main refactorings, incorporating feedback from Callek's review

Updated patch for main refactorings, incorporating feedback from Callek's review

Comment 22

[Justin Wood (:Callek)]

Comment on attachment 767728 [details] [diff] [review]
Updated patch for main refactorings, incorporating feedback from Callek's review

Review of attachment 767728 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but See my two comments, yes /just/ two comments!

Also for clarity I only checked against what I made comments on in the past review, I did not directly compare the two diffs or re-review all the same things. So if there was anything extra snuck in feel free to call it out!

::: buildfarm/mobile/watch_devices.sh
@@ +37,4 @@
>  function device_check() {
>    local device=$1
>    export PYTHONPATH=/builds/sut_tools
> +  deviceIP="$(nslookup "${device}" 2>/dev/null | sed -n '/^Name/,$s/^Address: *//p')"

this sed/code is a bit too dependant on nslookup behavior for my liking, can we just update PYTHONPATH above this line instead. -- yes I know this is how the python behaves though I just find sed scripts harder to read as well.

export PYTHONPATH=/builds/tools/lib/python:/builds/sut_tools

::: sut_tools/relay.py
@@ +1,1 @@
> +#!/usr/bin/python

I personally prefer the whole relay.py file moved, since we don't actually have a need to use it by hand (and "modules" having __main__ stuff is not all that uncommon) especially since this was written JUST for our use in automation anyway, and is never called directly in automation. additionally a human using this script as-is is unlikely (because we have mozpool which does the relay stuff better for humans)

That said, the changes look relatively correct so I will not block on the correction.

Comment 23

[Pete Moore [:pmoore][:pete]]

Hey Callek,

The sed change I preferred because what we currently have is a shell script invoking python, which then invokes system commands, in order to perform the relatively simple task of converting a name to an IP. This seemed like an unnecessary overhead both in terms of resources (firing up a python interpreter) and complexity (23 lines instead of 1). You could argue running sed is as much of an overhead as starting up python. I haven't done any performance testing to confirm one way or the other. :)

However this simple task currently takes 23 lines of code:

the shell script:

PYTHON=`which python`
export PYTHONPATH=/builds/tools/lib/python:/builds/sut_tools
deviceIP=`"${PYTHON}" -c "import sut_lib;print sut_lib.getIPAddress('$device')" 2> /dev/null`

… that calls the python code:

def getIPAddress(hostname):
    """Parse the output of nslookup to determine what is the
    IP address for the device ID that is to be monitored.
    """
    ipAddress = None
    f = False
    p, o = runCommand(['nslookup', hostname], logEcho=False)
    for s in o:
        if '**' in s:
            break
        if f:
            if s.startswith('Address:'):
                ipAddress = s.split()[1]
        else:
            if s.startswith('Name:'):
                f = True

    return ipAddress


My proposed alternative is a simple one-liner containing just a single pipe:

deviceIP="$(nslookup "${device}" 2>/dev/null | sed -n '/^Name/,$s/^Address: *//p')"


I do appreciate that most people in the team are more python-oriented than bash-oriented, so I am happy to concede, and leave the python implementation in place. I personally find the shorter version much easier to read and understand though. :)

Comment 24

[Justin Wood (:Callek)]

(In reply to Pete Moore [:pete][:pmoore] from comment #23)
> My proposed alternative is a simple one-liner containing just a single pipe:

So my reasons for having preferred the python here:
* I'm more familiar with it than sed, so I can more easily tell a python script is correct than a single sed line
* in the theoretical, non-exact distant future when we change the python this will be harder to catch.

That said, your reasoning for changing it convinced me that we should take your version.

Comment 25

[Pete Moore [:pmoore][:pete]]

Attachment: Main refactorings, with bug fixes applied

Updates after Staging testing completed.

Comment 26

[Pete Moore [:pmoore][:pete]]

Comment on attachment 770725 [details] [diff] [review]
Main refactorings, with bug fixes applied

Checked in a year ago, looks like I didn't set checked-in flag...

changeset:   3816:8057f3d38bd0
user:        Peter Moore <[e-mail]>
date:        Wed Jul 03 14:56:44 2013 +0200
summary:     Bug 850572: Refactoring of sut_lib,r=Callek

Comment 27

[Pete Moore [:pmoore][:pete]]

I don't think there is anything more to do here now.

Comment 28

[Pete Moore [:pmoore][:pete]]

Comment on attachment 766836 [details] [diff] [review]
Upgrade of mozdevice (not including changes to calling code since we should go through the testing of this together)

This will have probably bit-rotted by now, removing review request.