webkitpy: Implement coredumpctl support on linux
Created attachment 336580 [details] Patch
Created attachment 336581 [details] Patch
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.
Created attachment 336766 [details] Patch
(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).
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.
(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.
Created attachment 336776 [details] Patch
Comment on attachment 336776 [details] Patch r=me if Daniel is OK with this
Created attachment 336983 [details] Patch
(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.
(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.
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.
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)
Created attachment 337070 [details] Patch
(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.
Created attachment 337700 [details] Patch
(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...
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.
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.
Comment on attachment 339979 [details] webkitpy: Implement coredumpctl support on linux r=me
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?
(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.
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.
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.
Comment on attachment 342576 [details] Updated patch Clearing flags on attachment: 342576 Committed r232786: <https://trac.webkit.org/changeset/232786>
All reviewed patches have been landed. Closing bug.
<rdar://problem/41097213>