webkitpy should understand crash logs

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

webkitpy should understand crash logs

[Adam Barth]

Created attachment 98748 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 98748 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/crashlog.py:44
> +        if sys.platform == "darwin":
> +            self._capture_log_darwin(process_name)

Its funny how initing one of these does the search.  I think I would have called this thing CrashLogs and had a most_recent_log(process_name=) function instead.

> Tools/Scripts/webkitpy/common/system/crashlog.py:58
> +        log_directory = self._filesystem.expanduser("~")
> +        log_directory = os.path.join(log_directory, "Library", "Logs")
> +        if self._filesystem.exists(os.path.join(log_directory, "DiagnosticReports")):
> +            log_directory = os.path.join(log_directory, "DiagnosticReports")
> +        else:
> +            log_directory = os.path.join(log_directory, "CrashReporter")

I would have made this a separate function.

> Tools/Scripts/webkitpy/common/system/executive.py:301
> +        ps_process = self.popen(['ps', '-eo', 'pid,comm'], stdout=self.PIPE, stderr=self.PIPE)

Really? Python doesn't offer us a better way than running ps?  I guess not: http://ubuntuforums.org/showthread.php?t=463850

> Tools/Scripts/webkitpy/common/system/executive.py:311
> +        return running_pids

Do we need to sort these?  Might add a comment which explains why we don't.  Does this handle pid-wrapping? do we care?

> Tools/Scripts/webkitpy/common/system/executive.py:323
> +        while self.check_running_pid(pid):
> +            time.sleep(0.25)

Can't we just waitpid?  I guess on unix it has to be a child process?  I wonder if the same restriction applies to windows?

[Adam Barth]

(In reply to comment #2)
> (From update of attachment 98748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98748&action=review
> 
> > Tools/Scripts/webkitpy/common/system/crashlog.py:44
> > +        if sys.platform == "darwin":
> > +            self._capture_log_darwin(process_name)
> 
> Its funny how initing one of these does the search.  I think I would have called this thing CrashLogs and had a most_recent_log(process_name=) function instead.

I'm not sure I fully understand.  You're saying we should use a free function instead of a constructor?  What would the CrashLog class have in it?

> > Tools/Scripts/webkitpy/common/system/crashlog.py:58
> > +        log_directory = self._filesystem.expanduser("~")
> > +        log_directory = os.path.join(log_directory, "Library", "Logs")
> > +        if self._filesystem.exists(os.path.join(log_directory, "DiagnosticReports")):
> > +            log_directory = os.path.join(log_directory, "DiagnosticReports")
> > +        else:
> > +            log_directory = os.path.join(log_directory, "CrashReporter")
> 
> I would have made this a separate function.

Done.

> > Tools/Scripts/webkitpy/common/system/executive.py:301
> > +        ps_process = self.popen(['ps', '-eo', 'pid,comm'], stdout=self.PIPE, stderr=self.PIPE)
> 
> Really? Python doesn't offer us a better way than running ps?  I guess not: http://ubuntuforums.org/showthread.php?t=463850

This is what the internets recommended.

> > Tools/Scripts/webkitpy/common/system/executive.py:311
> > +        return running_pids
> 
> Do we need to sort these?  Might add a comment which explains why we don't.  Does this handle pid-wrapping? do we care?

I've sorted it.  I'm sure it matters that much.  I suspect that we'll have some race conditions because of parallelism, but we'll have to deal with them when they occur.

> > Tools/Scripts/webkitpy/common/system/executive.py:323
> > +        while self.check_running_pid(pid):
> > +            time.sleep(0.25)
> 
> Can't we just waitpid?  I guess on unix it has to be a child process?  I wonder if the same restriction applies to windows?

Under the covers, check_running_pid does what the perl code used to do and what the Internet recommends.  The perl code specifically has a comment about not using waitpid for this purpose.

[Adam Barth]

Created attachment 98751 [details]
Patch

[Adam Barth]

Created attachment 98757 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 98757 [details]
Patch

Missing your new file. ;(

[Adam Barth]

Created attachment 98759 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 98759 [details]
Patch

LGTM.

[Adam Barth]

Committed r89843: <http://trac.webkit.org/changeset/89843>