1156812 - [mozdevice] Support specifying alternate log buffers when retrieving or clearing logcat

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

We need to support specifying alternate log buggers in ADBDevice.get_logcat in order for us to switch to using adb.py in mozrunner.

See https://developer.android.com/tools/debugging/debugging-log.html#alternativeBuffers for more details on alternate log buffers. See also bug 1155029, which retrieves the logcat with an alternate buffer independently of adb.py

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Support specifying alternate log buffers. v1.0

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19dc0de094e7

Comment 2

[Bob Clary [:bc] (inactive)]

Comment on attachment 8595412 [details] [diff] [review]
Support specifying alternate log buffers. v1.0

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

let's do the same for clear_logcat

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +1143,5 @@
>              throwing an ADBTimeoutError.
>              This timeout is per adb call. The total time spent
>              may exceed this value. If it is not specified, the value
>              set in the ADBDevice constructor is used.
> +        :param: buffer: optional list of log buffers to retrieve. Defaults to

buffers

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Support specifying alternate log buffers. v1.1

Checks against valid list of buffers and now also applies to clear_logcat.

Comment 4

[Bob Clary [:bc] (inactive)]

Comment on attachment 8595978 [details] [diff] [review]
Support specifying alternate log buffers. v1.1

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

r+ with these changes.

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +1111,5 @@
> +            if b not in valid:
> +                raise ADBError('%s is not an available log buffer. '
> +                               'Use one of: %s.' % (b, ', '.join(valid)))
> +            approved.extend(['-b', b])
> +        return approved

The name is a little confusing since we are really returning the args and not the buffers themselves.

Also, the exception will only ever list one of the buffers and the exception message implies we should only use one buffer at a time when we could use all of them.

What about using:

def _get_logcat_buffer_args(self, buffers):
    valid_set = set(["radio", "main", "events"])
    invalid_buffers_set = set(buffers).difference(valid_set)
    if invalid_buffers_set:
        raise Exception("Invalid logcat buffers %s not in %s " %  (
            ', '.join(list(invalid_buffers_set)),
            ', '.join(list(valid_set))))
    args = []
    for buffer in buffers:
        args.extend(['-b', buffer])
    return args

This will produce exceptions like:

self._get_logcat_buffer_args(["invalidbuffer"])
Invalid logcat buffers invalidbuffer not in main, radio, events 

self._get_logcat_buffer_args(["radio", "main", "events", "invalidbuffer1", "invalidbuffer2"])
Invalid logcat buffers invalidbuffer2, invalidbuffer1 not in main, radio, events

@@ +1124,5 @@
>              value. If it is not specified, the value set
>              in the ADBDevice constructor is used.
> +        :param buffers: log buffers to clear. Valid buffers are "radio",
> +            "events", and "main". Defaults to "main".
> +        :type buffers: list

We don't use :type ...: elsewhere. What is the benefit of using it here rather than just saying:

:param buffers: list of log buffers to clear. Valid buffers are "radio", 
    "events", and "main". Defaults to "main".

@@ +1129,5 @@
>          :raises: * ADBTimeoutError
>                   * ADBError
>  
>          """
> +        buffers = self._get_logcat_buffers(buffers)

new name: self._get_logcat_buffer_args(buffers)

@@ +1160,5 @@
>              may exceed this value. If it is not specified, the value
>              set in the ADBDevice constructor is used.
> +        :param buffers: log buffers to retrieve. Valid buffers are "radio",
> +            "events", and "main". Defaults to "main".
> +        :type buffers: list

ditto

@@ +1166,5 @@
>          :raises: * ADBTimeoutError
>                   * ADBError
>  
>          """
> +        buffers = self._get_logcat_buffers(buffers)

ditto

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Support specifying alternate log buffers. v1.2

Thanks for the nice suggestion for comparing against the valid buffers. The only difference I made was removing the join as the lists are presented clearly in the errors as they are: "Invalid logcat buffers ['foo', 'bar'] not in ['main', 'radio', 'events']"

(In reply to Bob Clary [:bc:] from comment #4)
> 
> @@ +1124,5 @@
> >              value. If it is not specified, the value set
> >              in the ADBDevice constructor is used.
> > +        :param buffers: log buffers to clear. Valid buffers are "radio",
> > +            "events", and "main". Defaults to "main".
> > +        :type buffers: list
> 
> We don't use :type ...: elsewhere. What is the benefit of using it here
> rather than just saying:
> 
> :param buffers: list of log buffers to clear. Valid buffers are "radio", 
>     "events", and "main". Defaults to "main".

I was following http://sphinx-doc.org/domains.html#info-field-lists which indicated specifying type separately. It suggests that the type would be linked where possible, and it's presented after the name in parenthesis. I've change it to include the type inline as also suggested on the above page: "It is also possible to combine parameter type and description, if the type is a single word". I would be happy to revert this and add the type into the description for consistency (or I could open a bug to move the types out of the descriptions for other docstrings).

Comment 6

[Bob Clary [:bc] (inactive)]

(In reply to Dave Hunt (:davehunt) from comment #5)
> Created attachment 8596084 [details] [diff] [review]
> Support specifying alternate log buffers. v1.2
> 
> Thanks for the nice suggestion for comparing against the valid buffers. The
> only difference I made was removing the join as the lists are presented
> clearly in the errors as they are: "Invalid logcat buffers ['foo', 'bar']
> not in ['main', 'radio', 'events']"
> 

Heh. I originally just output the list but change it to match what you did originally. :-)


> (In reply to Bob Clary [:bc:] from comment #4)
> > 
> > @@ +1124,5 @@
> > >              value. If it is not specified, the value set
> > >              in the ADBDevice constructor is used.
> > > +        :param buffers: log buffers to clear. Valid buffers are "radio",
> > > +            "events", and "main". Defaults to "main".
> > > +        :type buffers: list
> > 
> > We don't use :type ...: elsewhere. What is the benefit of using it here
> > rather than just saying:
> > 
> > :param buffers: list of log buffers to clear. Valid buffers are "radio", 
> >     "events", and "main". Defaults to "main".
> 
> I was following http://sphinx-doc.org/domains.html#info-field-lists which
> indicated specifying type separately. It suggests that the type would be
> linked where possible, and it's presented after the name in parenthesis.
> I've change it to include the type inline as also suggested on the above
> page: "It is also possible to combine parameter type and description, if the
> type is a single word". I would be happy to revert this and add the type
> into the description for consistency (or I could open a bug to move the
> types out of the descriptions for other docstrings).

Let's do a separate bug and do them all at once and be consistent across all of the adb*.py files.

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Support specifying alternate log buffers. v1.3

I've triggered a try run, but as :bc points out, nothing on try will use the new buffers so perhaps this is not necessary:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d0f78f5f5d5

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8596089 [details] [diff] [review]
Support specifying alternate log buffers. v1.3

Carrying r+

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/619f030b0d23

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2d6650a8382a for android m7 orange:
https://treeherder.mozilla.org/logviewer.html#?job_id=9252854&repo=mozilla-inbound

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: traceback

I don't see how the patch could have caused this. The buffers are not even specified.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Sorry, didn't catch that bug 1157948 just got filed for this (and then the failure happened on other trees without this patch. Will reland soon.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/493ad8a8f38b

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/493ad8a8f38b