Bug 184039 – webkitpy: Implement coredumpctl support on linux

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Thibault Saunier]

webkitpy: Implement coredumpctl support on linux

[Thibault Saunier]

Created attachment 336580 [details]
Patch

[Thibault Saunier]

Created attachment 336581 [details]
Patch

[Daniel Bates]

Comment on attachment 336581 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336581&action=review

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:32
> +import datetime

Not in sorted order. Please sort all the  import lines suchthat they are in sorted order according to the UNIX sort tool.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:60
> +        for ntry in range(5):

This is a suboptimal way to loop as we malloc a list. What does ntry stand for? Did you mean to write retries?

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:62
> +                # Loopping, it means we conceder the logs might not be ready

Please remove this comment as it is redundant given the comment on line 59. Also, I am surprised we don’t have a convenience function in webktpy to do waiting for us.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:64
> +                time.sleep(1)

Is there not a more direct way to know if systemd generating the backtrack so we can avoid busy waiting?

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:67
> +                info = subprocess.check_output(['coredumpctl', 'info',

Can we write this logic using the Executive object? Can this class take an Executive object in its constructor?

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:71
> +                # The trace might not be ready yet

:( Can we find a more deterministic and direct way to test this. Writing code that sleeps and hopes the data becomes available when we wake artificially slows down testing.

[Thibault Saunier]

Created attachment 336766 [details]
Patch

[Thibault Saunier]

(In reply to Daniel Bates from comment #3)
> Comment on attachment 336581 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336581&action=review
> 
> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:32
> > +import datetime
>
> Not in sorted order. Please sort all the  import lines suchthat they are in
> sorted order according to the UNIX sort tool.

Done.
 
> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:60
> > +        for ntry in range(5):
> 
> This is a suboptimal way to loop as we malloc a list. What does ntry stand
> for? Did you mean to write retries?

s/ntry/try_number/ (0 is not a retry). 

> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:62
> > +                # Loopping, it means we conceder the logs might not be ready
> 
> Please remove this comment as it is redundant given the comment on line 59.
> Also, I am surprised we don’t have a convenience function in webktpy to do
> waiting for us.

Done - how more convenient than that could it be? (I have not found anything special, but might be wrong).

> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:64
> > +                time.sleep(1)
> 
> Is there not a more direct way to know if systemd generating the backtrack
> so we can avoid busy waiting?

Well, systemd can't know if it is going to generate a trace it might not now about yet (apart from doing what we do "wait and see") - I checked the CLI but couldn't see anything better.

> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:67
> > +                info = subprocess.check_output(['coredumpctl', 'info',
> 
> Can we write this logic using the Executive object? Can this class take an
> Executive object in its constructor?

Done. And moved all the Popen calls to using it.

> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:71
> > +                # The trace might not be ready yet
> 
> :( Can we find a more deterministic and direct way to test this. Writing
> code that sleeps and hopes the data becomes available when we wake
> artificially slows down testing.

Yeah it is not optimal but I don't think we can do anything better. Ideally that code paths should never be reached (generating backtraces for crashes)... and in practice it is not going to happen often (and in my limited tested, and on my quite powerful machine, retrying won't ever happen more than once, so 1sec lost in the worst case of the worst case).

[Michael Catanzaro]

Comment on attachment 336766 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336766&action=review

FWIW, I can't think of a better way to check for the backtrace than this sleep loop. It's not like we can poll a filesystem location or hook into some form of notifications or anything, so the sleep loop looks OK to me.

So this looks good to me. And I believe Carlos Lopez was also OK with it, right?

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:63
> +        for try_number in xrange(5):

Use range() so that it works with python3

Um, I see Daniel asked you to not do that, but reality is going to catch up with us pretty soon here, so my vote is definitely for range().

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:65
> +                # Loopping, it means we consider the logs might not be ready yet.

Looping

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:71
> +            except ScriptError:

What happens if coredumpctl is not installed? Are you sure it only throws ScriptError? I think it can throw an OSError. Check by changing 'coredumpctl' to gibberish.

[Thibault Saunier]

(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 336766 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336766&action=review
> 
> FWIW, I can't think of a better way to check for the backtrace than this
> sleep loop. It's not like we can poll a filesystem location or hook into
> some form of notifications or anything, so the sleep loop looks OK to me.
> 
> So this looks good to me. And I believe Carlos Lopez was also OK with it,
> right?
> 
> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:63
> > +        for try_number in xrange(5):
> 
> Use range() so that it works with python3

Done... xrange sounds so old to me, well python2.

> Um, I see Daniel asked you to not do that, but reality is going to catch up
> with us pretty soon here, so my vote is definitely for range().
> 
> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:65
> > +                # Loopping, it means we consider the logs might not be ready yet.
> 
> Looping

Done.

> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:71
> > +            except ScriptError:
> 
> What happens if coredumpctl is not installed? Are you sure it only throws
> ScriptError? I think it can throw an OSError. Check by changing
> 'coredumpctl' to gibberish.

Fixed, you were right.

[Thibault Saunier]

Created attachment 336776 [details]
Patch

[Michael Catanzaro]

Comment on attachment 336776 [details]
Patch

r=me if Daniel is OK with this

[Thibault Saunier]

Created attachment 336983 [details]
Patch

[Michael Catanzaro]

(In reply to Thibault Saunier from comment #10)
> Created attachment 336983 [details]
> Patch

Wrong bug. You'll want to obsolete this new patch and un-obsolete the old one.

[Thibault Saunier]

(In reply to Michael Catanzaro from comment #11)
> (In reply to Thibault Saunier from comment #10)
> > Created attachment 336983 [details]
> > Patch
> 
> Wrong bug. You'll want to obsolete this new patch and un-obsolete the old
> one.

Sorry about that, I do not get what `webkit-patch` did.

[Daniel Bates]

Comment on attachment 336776 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336776&action=review

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:70
> +                                                   return_stderr=True)

Please write this on one line.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:79
> +                        r'Timestamp: .*(\d\d\d\d-\d+-\d+ \d+:\d+:\d+)', info):

Please write this on one line.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:90
> +            tf = tempfile.NamedTemporaryFile()

A more canonical name for this variable is temp or temp_file.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:92
> +                                            '--output=' + tf.name],

I take it coredumptctl only parses “output=“? Otherwise I would suggest passing the temporary file name as its own argument and avoid the string concatenation.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:93
> +                                           return_stderr=True,

This will be ignored when return_exit_code is True. So, please remove this.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:99
> +            return res

Please do not abbreviate names. This should be result.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:125
> +                log_directory, file_filter=match_filename)

Please write this on one line. We follow PEP-8 style for Python code plus the WebKit code style guidelines for variable and function naming.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:132
> +            crash_log, errors = self._get_trace_from_systemd(

Ditto.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:138
> +            ['c++filt', ], stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE)

Ditto.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:144
> +            core_pattern = self._filesystem.join(

Ditto.

> Tools/Scripts/webkitpy/port/linux_get_crash_log_unittest.py:35
> +from webkitpy.common.system.executive_mock import MockExecutive

Please sort these from lines by their from clause. So, webkitpy.common should come before webkitpy.port.

[Thibault Saunier]

Comment on attachment 336776 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336776&action=review

I did not change the line length and kept following PEP8, if you really think we should not follow the 79 line length, I could change.

>> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:70
>> +                                                   return_stderr=True)
> 
> Please write this on one line.

I am following pep8 as advised here: https://webkit.org/code-style-guidelines/#python

pep8 is pretty clear about line length: https://www.python.org/dev/peps/pep-0008/#maximum-line-length

Previous code was not following the proper code style but I did not fix it to avoid filling my patch with unrelated changes.

>> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:90
>> +            tf = tempfile.NamedTemporaryFile()
> 
> A more canonical name for this variable is temp or temp_file.

Done.

>> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:92
>> +                                            '--output=' + tf.name],
> 
> I take it coredumptctl only parses “output=“? Otherwise I would suggest passing the temporary file name as its own argument and avoid the string concatenation.

Done, it also supports `--output outfilename`

>> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:93
>> +                                           return_stderr=True,
> 
> This will be ignored when return_exit_code is True. So, please remove this.

Done.

>> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:99
>> +            return res
> 
> Please do not abbreviate names. This should be result.

Fixed (just returned).

>> Tools/Scripts/webkitpy/port/linux_get_crash_log_unittest.py:35
>> +from webkitpy.common.system.executive_mock import MockExecutive
> 
> Please sort these from lines by their from clause. So, webkitpy.common should come before webkitpy.port.

Well, I just followed what was there. (fixed all)

[Thibault Saunier]

Created attachment 337070 [details]
Patch

[Daniel Bates]

(In reply to Thibault Saunier from comment #14)
> Comment on attachment 336776 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336776&action=review
> 
> I did not change the line length and kept following PEP8, if you really
> think we should not follow the 79 line length, I could change.
> 
> >> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:70
> >> +                                                   return_stderr=True)
> > 
> > Please write this on one line.
> 
> I am following pep8 as advised here:
> https://webkit.org/code-style-guidelines/#python
> 
> pep8 is pretty clear about line length:
> https://www.python.org/dev/peps/pep-0008/#maximum-line-length

It is an unwritten rule (for now - we should change this) that we do not honor PEP8's maximum line length rule. It is archaic and hurts readability. These sentiments are echoed in the thread that includes <https://lists.webkit.org/pipermail/webkit-dev/2010-April/012486.html> and is consistent with the question raised in bug 179387, comment 2.

[Thibault Saunier]

Created attachment 337700 [details]
Patch

[Thibault Saunier]

(In reply to Daniel Bates from comment #16)
> It is an unwritten rule (for now - we should change this) that we do not
> honor PEP8's maximum line length rule. It is archaic and hurts readability.
> These sentiments are echoed in the thread that includes
> <https://lists.webkit.org/pipermail/webkit-dev/2010-April/012486.html> and
> is consistent with the question raised in bug 179387, comment 2.

I did what you asked though I disagree that the rule is archaic, as I use split views often having the code less than 80 chars makes it more readable to me, anyway tastes...

[Daniel Bates]

Comment on attachment 337700 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337700&action=review

Thank you Thibault for iterating on this patch. The patch looks sane to me.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:77
> +                for datestr in re.findall(r'Timestamp: .*(\d\d\d\d-\d+-\d+ \d+:\d+:\d+)', info):

I do not see the need to abbreviate the word “string” and “date string” is two words. For your consideration I suggest that we name this variable timestamp given that the we are matching against the extracted “Timestamp” values. On another note, we can simplify this regular expression by using repetition syntax, \d{x}, where x is the number of repetitions: ‘Timestamp .*(\d{4}-\d+-\d+ \d+:\d+:\d+’. We could simplify this further using repetition syntax and non-capture groups though I am unclear whether that would hurt readability.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:107
> +        # Poor man which, ignore any failure.

For your consideration I suggest using manual page syntax when referencing another program name. So, “which”  => “which(1)”. This makes it straightforward for a reader to identify a command from an English word.

[Thibault Saunier]

Created attachment 339979 [details]
webkitpy: Implement coredumpctl support on linux

(In reply to Daniel Bates from comment #19)
> Comment on attachment 337700 [details]
>
> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:77
> > +                for datestr in re.findall(r'Timestamp: .*(\d\d\d\d-\d+-\d+ \d+:\d+:\d+)', info):
> 
> I do not see the need to abbreviate the word “string” and “date string” is
> two words. For your consideration I suggest that we name this variable
> timestamp given that the we are matching against the extracted “Timestamp”
> values. On another note, we can simplify this regular expression by using
> repetition syntax, \d{x}, where x is the number of repetitions: ‘Timestamp
> .*(\d{4}-\d+-\d+ \d+:\d+:\d+’. We could simplify this further using
> repetition syntax and non-capture groups though I am unclear whether that
> would hurt readability.

Updated to regex to use repetition syntax.

> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:107
> > +        # Poor man which, ignore any failure.
> 
> For your consideration I suggest using manual page syntax when referencing
> another program name. So, “which”  => “which(1)”. This makes it
> straightforward for a reader to identify a command from an English word.

Done.

[Daniel Bates]

Comment on attachment 339979 [details]
webkitpy: Implement coredumpctl support on linux

r=me

[Daniel Bates]

Comment on attachment 339979 [details]
webkitpy: Implement coredumpctl support on linux

View in context: https://bugs.webkit.org/attachment.cgi?id=339979&action=review

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:127
> +            ['c++filt', ], stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE)

Is the “, “ intentional in the array provided in the first argument?

[Thibault Saunier]

(In reply to Daniel Bates from comment #22)
> Comment on attachment 339979 [details]
> webkitpy: Implement coredumpctl support on linux
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339979&action=review
> 
> > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:127
> > +            ['c++filt', ], stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> 
> Is the “, “ intentional in the array provided in the first argument?

It is not my code.

[Thibault Saunier]

Created attachment 342576 [details]
Updated patch

(In reply to Thibault Saunier from comment #23)
> (In reply to Daniel Bates from comment #22)
> > Comment on attachment 339979 [details]
> > webkitpy: Implement coredumpctl support on linux
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=339979&action=review
> > 
> > > Tools/Scripts/webkitpy/port/linux_get_crash_log.py:127
> > > +            ['c++filt', ], stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> > 
> > Is the “, “ intentional in the array provided in the first argument?

Removed the trailling comma.

[Michael Catanzaro]

You already had your r+ on the previous version of this patch, so instead of requesting review again (which slows things down waiting for one specific developer to change the flag again) it is better to ensure Daniel's name is in the Reviewed by field in the ChangeLog and request only commit queue. That way, any committer can land it.

If you use 'webkit-patch apply-from-bug --no-update' (--no-update is to avoid it doing a git pull) then it would have filled in his name automatically.

[WebKit Commit Bot]

Comment on attachment 342576 [details]
Updated patch

Clearing flags on attachment: 342576

Committed r232786: <https://trac.webkit.org/changeset/232786>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41097213>