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)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Callek, Assigned: Callek)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached patch [tools] v1 (obsolete) — 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 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 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+
Attachment #672462 - Flags: review?(armenzg) → review+
no idea how I asked :armenzg for review on this...quirky bugzilla.
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)?
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
Attached patch [tools] v2Splinter Review
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)
Attached patch interdiffSplinter Review
For anyone curious, this is an interdiff between v1 and v2
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+
Attachment #672931 - Flags: feedback?(coop) → feedback+
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
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)
reopen because not yet deployed and re: c#10
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #675915 - Flags: review?(kmoir) → review+
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: