Last Comment Bug 792084 - Add a method to mozdevice which runs a command and returns standard output
: Add a method to mozdevice which runs a command and returns standard output
Status: RESOLVED FIXED
:
Product: Testing
Classification: Components
Component: Mozbase (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla18
Assigned To: Malini Das [:mdas] - Away, not checking bugmail
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-18 09:33 PDT by William Lachance (:wlach) [on PTO Jul 30 -> Aug 7 2016]
Modified: 2012-09-26 17:12 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1.0 (1.29 KB, patch)
2012-09-25 11:42 PDT, Malini Das [:mdas] - Away, not checking bugmail
wlachance: review+
Details | Diff | Splinter Review

Description William Lachance (:wlach) [on PTO Jul 30 -> Aug 7 2016] 2012-09-18 09:33:05 PDT
The current shell function in devicemanager is great for when you want to read a bunch of data and write it directly to a file (or standard output), but it's unwieldly if we just want to run a simple command and operate directly on the output.

The most recent demand for something like this was in bug 792048, but it comes up quite often. I'd written a workaround for this myself in eideticker: 

https://github.com/mozilla/eideticker/blob/master/src/eideticker/eideticker/device.py#L62

Inspired by python's subprocess module, let's add a method called shellCheckOutput to devicemanager that works as above. For the cases that the command we're calling returns 0 (or otherwise fails), let's throw an exception. Yes, this amounts to a change in policy in devicemanager, but jmaher and I agreed it's for the better.
Comment 1 Malini Das [:mdas] - Away, not checking bugmail 2012-09-25 11:42:30 PDT
Created attachment 664593 [details] [diff] [review]
patch v1.0

This patch adds the shellCheckOutput command to devicemanager.py. The implementation is the same regardless of environment, so it's not an abstract method.

I've tested this in an adb and sut environment and this works as expected.
Comment 2 William Lachance (:wlach) [on PTO Jul 30 -> Aug 7 2016] 2012-09-26 07:28:36 PDT
Comment on attachment 664593 [details] [diff] [review]
patch v1.0

Wonderful! Thank you so much. Hopefully we can guide people to using this now as opposed to checkCmd and sendCmd.

>diff --git a/mozdevice/mozdevice/devicemanager.py b/mozdevice/mozdevice/devicemanager.py
>index 3150176..c0c72f9 100644

>+        if retval == None:
>+            raise DMError("Did not successfully run command %s (output: '%s', retval: 'None')" % (cmd, output))

PEP8 suggests that this be written as retval is None

"Comparisons to singletons like None should always be done with is or is not, never the equality operators.
Comment 3 Malini Das [:mdas] - Away, not checking bugmail 2012-09-26 09:34:11 PDT
> PEP8 suggests that this be written as retval is None
> 
> "Comparisons to singletons like None should always be done with is or is
> not, never the equality operators.

Cool, thanks, I've changed it, tested it and pushed to mozilla-inbound:


https://hg.mozilla.org/integration/mozilla-inbound/rev/0450cb4ef66a
Comment 4 Andrew Halberstadt [:ahal] 2012-09-26 09:40:17 PDT
Can you push to the github repo as well?

FYI (someone correct me if I'm wrong), we are now pushing to the github repo first, and then the changes will get mirrored in jhammel's weekly mozbase merge that he has started doing. If you need it right away you can mirror the mozbase repo immediately (see https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Mirroring or ask jhammel)
Comment 5 William Lachance (:wlach) [on PTO Jul 30 -> Aug 7 2016] 2012-09-26 09:47:55 PDT
(In reply to Andrew Halberstadt [:ahal] from comment #4)
> Can you push to the github repo as well?
> 
> FYI (someone correct me if I'm wrong), we are now pushing to the github repo
> first, and then the changes will get mirrored in jhammel's weekly mozbase
> merge that he has started doing. If you need it right away you can mirror
> the mozbase repo immediately (see
> https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Mirroring or ask
> jhammel)

Yes, this is correct. The policy here is to commit to the github repo first and then use the mirroring script to move changes to m-c. For more details, see the wiki page that Andrew linked to.

That said, there is no particular harm done in this case. I think we can just commit to the github repository and be done with it: this would only be a problem if we did a mirror before that was done.
Comment 6 Andrew Halberstadt [:ahal] 2012-09-26 09:48:54 PDT
Yeah, it won't cause problems, just for future reference.
Comment 7 Malini Das [:mdas] - Away, not checking bugmail 2012-09-26 09:56:36 PDT
Agh, sorry, I forgot which way the mirror went. I'll do the commit to github now
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-09-26 17:12:40 PDT
https://hg.mozilla.org/mozilla-central/rev/0450cb4ef66a

Note You need to log in before you can comment on or make changes to this bug.