Closed
Bug 728993
Opened 13 years ago
Closed 13 years ago
Need to check output to capture run-as failure in devicemanagerADB
Categories
(Testing :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla13
People
(Reporter: wlach, Assigned: wlach)
Details
Attachments
(1 file, 1 obsolete file)
2.32 KB,
patch
|
wlach
:
review+
|
Details | Diff | Splinter Review |
Props to Christian Holler for noticing this and providing a patch:
run-as returns 0 even when it won't work, so we'll need to check for its error message. Also, we should still specify the packageName even when run-as won't work.
Assignee | ||
Comment 1•13 years ago
|
||
Here's Christian's patch against mozilla-central, modified very slightly by me for style. It looks quite straightforward and seems to give the expected behaviour to me (that is, verifyRunAs will return the appropriate exception against a non-debuggable application).
Assignee: nobody → wlachance
Attachment #599033 -
Flags: review?(jmaher)
Comment 2•13 years ago
|
||
Comment on attachment 599033 [details] [diff] [review]
Need to check output to capture run-as failure in devicemanagerADB
Review of attachment 599033 [details] [diff] [review]:
-----------------------------------------------------------------
I like what the patch is doing, just have a couple small things to address below.
::: build/mobile/devicemanagerADB.py
@@ +688,5 @@
> + # when failing because of a non-existent or non-debuggable package :(
> + runAsOut = self.runCmd(["shell", "run-as", packageName, "mkdir", devroot + "/sanity"]).communicate()[0].splitlines()
> + if len(runAsOut) > 0 and runAsOut[0].startswith("run-as:") and \
> + ("not debuggable" in runAsOut[0] or "is unknown" in runAsOut[0]):
> + raise DMError("run-as failed sanity check")
Do we want this to raise an error? Also I think this is a lot of and/or conditions for one long line, we should split it up a bit so it is easier to debug and modify in the future.
Attachment #599033 -
Flags: review?(jmaher) → review+
Comment 3•13 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #2)
> ::: build/mobile/devicemanagerADB.py
> @@ +688,5 @@
> > + # when failing because of a non-existent or non-debuggable package :(
> > + runAsOut = self.runCmd(["shell", "run-as", packageName, "mkdir", devroot + "/sanity"]).communicate()[0].splitlines()
> > + if len(runAsOut) > 0 and runAsOut[0].startswith("run-as:") and \
> > + ("not debuggable" in runAsOut[0] or "is unknown" in runAsOut[0]):
> > + raise DMError("run-as failed sanity check")
>
> Do we want this to raise an error?
Raiding the error is required, otherwise the caller (in this case the init function) cannot detect that run-as is not working and will try to use run-as although it is broken.
Comment 4•13 years ago
|
||
(In reply to Christian Holler (:decoder) from comment #3)
> Raiding the error
Should of course be "Raising" >.<
Assignee | ||
Comment 5•13 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #2)
> Comment on attachment 599033 [details] [diff] [review]
> Need to check output to capture run-as failure in devicemanagerADB
>
> Review of attachment 599033 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I like what the patch is doing, just have a couple small things to address
> below.
>
> ::: build/mobile/devicemanagerADB.py
> @@ +688,5 @@
> > + # when failing because of a non-existent or non-debuggable package :(
> > + runAsOut = self.runCmd(["shell", "run-as", packageName, "mkdir", devroot + "/sanity"]).communicate()[0].splitlines()
> > + if len(runAsOut) > 0 and runAsOut[0].startswith("run-as:") and \
> > + ("not debuggable" in runAsOut[0] or "is unknown" in runAsOut[0]):
> > + raise DMError("run-as failed sanity check")
>
> Do we want this to raise an error? Also I think this is a lot of and/or
> conditions for one long line, we should split it up a bit so it is easier to
> debug and modify in the future.
(In reply to Christian Holler (:decoder) from comment #3)
> (In reply to Joel Maher (:jmaher) from comment #2)
>
> > ::: build/mobile/devicemanagerADB.py
> > @@ +688,5 @@
> > > + # when failing because of a non-existent or non-debuggable package :(
> > > + runAsOut = self.runCmd(["shell", "run-as", packageName, "mkdir", devroot + "/sanity"]).communicate()[0].splitlines()
> > > + if len(runAsOut) > 0 and runAsOut[0].startswith("run-as:") and \
> > > + ("not debuggable" in runAsOut[0] or "is unknown" in runAsOut[0]):
> > > + raise DMError("run-as failed sanity check")
> >
> > Do we want this to raise an error?
>
> Raiding the error is required, otherwise the caller (in this case the init
> function) cannot detect that run-as is not working and will try to use
> run-as although it is broken.
Yep. Honestly raising an exception seems like an odd way of communicating that a feature isn't supported, but that's how the function is designed to work at present (see Init() for where it's called). Perhaps we should consider changing this later.
Assignee | ||
Comment 6•13 years ago
|
||
This simplifies checking for the run-as string from four conditions to three, and removes the unnecessary step of splitting the output of calling the process into lines.
Attachment #599033 -
Attachment is obsolete: true
Attachment #599166 -
Flags: review+
Comment 7•13 years ago
|
||
Comment 8•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in
before you can comment on or make changes to this bug.
Description
•