Closed
Bug 802768
Opened 12 years ago
Closed 12 years ago
Cleanup code, begin to add official panda support to sut_tools
Categories
(Release Engineering :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: Callek, Assigned: Callek)
References
Details
Attachments
(3 files, 1 obsolete file)
37.30 KB,
patch
|
kmoir
:
review+
coop
:
feedback+
|
Details | Diff | Splinter Review |
5.28 KB,
patch
|
Details | Diff | Splinter Review | |
1.58 KB,
patch
|
kmoir
:
review+
|
Details | Diff | Splinter Review |
So this patch does a lot of s/tegra/device/ stuff, and needed attention throughout.
I explicitly tried to steer clear of the check.py//dashboard code for now, just making sure it doesn't break with any of these other changes.
Wanting at laest 2 sets of eyes on this code, as a sanity, over-requesting because I don't know which 2 will look sooner. (Others can feel free to chime in)
Attachment #672462 -
Flags: review?(kmoir)
Attachment #672462 -
Flags: review?(jmaher)
Attachment #672462 -
Flags: review?(coop)
Attachment #672462 -
Flags: review?(armenzg)
Comment 1•12 years ago
|
||
Comment on attachment 672462 [details] [diff] [review]
[tools] v1
Review of attachment 672462 [details] [diff] [review]:
-----------------------------------------------------------------
I would normally give r- with the EXPECTED_DEVICE_SCREEN but I would rather see this committed and make sure that it works with tegra and then follow up with any adjustments for the pandas.
This patch as-is does not seem like it could give any trouble to the tegras.
::: sut_tools/check.py
@@ +15,5 @@
>
> # from multiprocessing import get_logger, log_to_stderr
> from sut_lib import checkSlaveAlive, checkSlaveActive, getIPAddress, dumpException, loadOptions, \
> + checkCPAlive, checkCPActive, getLastLine, stopProcess, runCommand, pingDevice, \
> + reboot_tegra, stopDevice, getMaster
reboot_device rather than reboot_tegra
@@ +189,4 @@
>
> if options.reset and sTegra == 'INACTIVE' and status['cp'] == 'INACTIVE':
> log.info('stopping hung clientproxy')
> + stopDevice(tegra)
stopDevice(device) instead of stopDevice(tegra)?
::: sut_tools/verify.py
@@ +137,5 @@
> info = dm.getInfo("screen")
> + if not info["screen"][0] == EXPECTED_DEVICE_SCREEN:
> + setFlag(errorFile, "Remote Device Error: Unexpected Screen on device, got '%s' expected '%s'" % \
> + (info["screen"][0], EXPECTED_DEVICE_SCREEN))
> + if not dm.adjustResolution(**EXPECTED_DEVICE_SCREEN_ARGS):
Not sure if this code should be re-used for pandas as of now.
Attachment #672462 -
Flags: review?(armenzg) → review+
Comment 2•12 years ago
|
||
Comment on attachment 672462 [details] [diff] [review]
[tools] v1
Review of attachment 672462 [details] [diff] [review]:
-----------------------------------------------------------------
this looks good. I really want to make sure that all scripts and tools which call this from a command line are modified.
::: sut_tools/clientproxy.py
@@ +35,5 @@
>
> Parameters
> --bbpath <path> Parent directory where the buildslave to control is located.
> + --device Device to manage. If not given it will be determined.
> + --deviceIP IP address of Device to manage. Will be discovered if not given.
who calls this? buildbot? can we make sure we update all upstream consumers of these tools.
@@ +489,3 @@
>
> try:
> + n = int(options.device.split('-')[1])
will this work with panda-0001 ?
Attachment #672462 -
Flags: review?(jmaher)
Attachment #672462 -
Flags: review?(armenzg)
Attachment #672462 -
Flags: review+
Updated•12 years ago
|
Attachment #672462 -
Flags: review?(armenzg) → review+
Comment 3•12 years ago
|
||
no idea how I asked :armenzg for review on this...quirky bugzilla.
Comment 4•12 years ago
|
||
Comment on attachment 672462 [details] [diff] [review]
[tools] v1
Review of attachment 672462 [details] [diff] [review]:
-----------------------------------------------------------------
Just the one nit about changing reboot_tegra() to reboot_device(). r+ with that change made.
::: sut_tools/clientproxy.py
@@ +489,3 @@
>
> try:
> + n = int(options.device.split('-')[1])
Just tested it in a python shell, and yes, n=1 for panda-0001.
::: sut_tools/sut_lib.py
@@ +491,3 @@
> sys.exit(1)
>
> def reboot_tegra(tegra, debug=False):
Change to reboot_device (as I believe Armen suggested elsewhere)?
Assignee | ||
Comment 5•12 years ago
|
||
Comment on attachment 672462 [details] [diff] [review]
[tools] v1
Review of attachment 672462 [details] [diff] [review]:
-----------------------------------------------------------------
::: buildfarm/mobile/stop_cp.sh
@@ +13,3 @@
> if [ -d $i ] ; then
> if [ -e $i/clientproxy.pid ] ; then
> python sut_tools/stop.py -t $i
self-review catch #1:
stop.py --device $i rather than -t
::: sut_tools/check.py
@@ +189,4 @@
>
> if options.reset and sTegra == 'INACTIVE' and status['cp'] == 'INACTIVE':
> log.info('stopping hung clientproxy')
> + stopDevice(tegra)
With this patch, check.py still only supports tegras, so this line is correct. Above though, s/reboot_tegra/reboot_device/ was asked for by not just you, so will do.
::: sut_tools/clientproxy.py
@@ +35,5 @@
>
> Parameters
> --bbpath <path> Parent directory where the buildslave to control is located.
> + --device Device to manage. If not given it will be determined.
> + --deviceIP IP address of Device to manage. Will be discovered if not given.
This is actually only ever called by start_cp.sh -- or rather I also see: http://mxr.mozilla.org/build/source/tools/sut_tools/check.py#196 now but I have no idea if/how that has *ever* been supposed to work. I will fix its command arg anyway, for added sanity.
::: sut_tools/sut_lib.py
@@ +491,3 @@
> sys.exit(1)
>
> def reboot_tegra(tegra, debug=False):
done
::: sut_tools/verify.py
@@ +137,5 @@
> info = dm.getInfo("screen")
> + if not info["screen"][0] == EXPECTED_DEVICE_SCREEN:
> + setFlag(errorFile, "Remote Device Error: Unexpected Screen on device, got '%s' expected '%s'" % \
> + (info["screen"][0], EXPECTED_DEVICE_SCREEN))
> + if not dm.adjustResolution(**EXPECTED_DEVICE_SCREEN_ARGS):
It's actually not:
# Resolution Check disabled for now; Bug 737427
if False and not checkAndFixScreen(dm):
But fairly stated
Assignee | ||
Comment 6•12 years ago
|
||
review/feedback just for extra set of eyes. About to throw this into staging.
Attachment #672462 -
Attachment is obsolete: true
Attachment #672462 -
Flags: review?(kmoir)
Attachment #672462 -
Flags: review?(coop)
Attachment #672931 -
Flags: review?(kmoir)
Attachment #672931 -
Flags: feedback?(coop)
Assignee | ||
Comment 7•12 years ago
|
||
For anyone curious, this is an interdiff between v1 and v2
Comment 8•12 years ago
|
||
Comment on attachment 672931 [details] [diff] [review]
[tools] v2
Looks good, I noticed when testing that in the start_cp.sh the condition to check for the existence of pandas and tegras directory matches even if tegra-* doesn't exist
if [ -n "$pandas" -a -n "$tegras" ] ; then
# Preserve the assertion that a single foopy won't handle both device types
31 echo ERROR: Found both panda-* and tegra-* in directory.
so this needs to be fixed
Attachment #672931 -
Flags: review?(kmoir) → review+
Updated•12 years ago
|
Attachment #672931 -
Flags: feedback?(coop) → feedback+
Assignee | ||
Comment 9•12 years ago
|
||
http://hg.mozilla.org/build/tools/rev/e6ebee9b679d with changes to start_cp as kim suggested
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•12 years ago
|
||
Soo after testing other patches in more detail, I found a few issues with what landed here.
sut_lib.pandas and tegras were failing to actually get populated (s/devices/allDevices/) and doing that sed also caused problems (was an array with no keys, vs a dictionary like code expected) the update here fixes that.
On linux the start_cp was failing due to the bad stderr redir. I added the ls command to stop_cp as well due to that trying to stop a device called |tegra-*| on the panda foopy. Also the stop/start files on mac foopies are not symlinked to the files in /builds/tools so will need manual updating.
Attachment #675915 -
Flags: review?(kmoir)
Assignee | ||
Comment 11•12 years ago
|
||
reopen because not yet deployed and re: c#10
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•12 years ago
|
Attachment #675915 -
Flags: review?(kmoir) → review+
Assignee | ||
Comment 12•12 years ago
|
||
bustage fix landed and deployed: http://hg.mozilla.org/build/tools/rev/0c2c85858515
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
Updated•7 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•