Bug 117098

<rdar://problem/14039502>

Created attachment 203616 [details] Patch

Comment on attachment 203616 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203616&action=review > Tools/Scripts/webkitpy/common/system/crashlogs.py:83 > + log_directory = port.results_directory() I only needed to pass in 'port' to get access to this directory. Is there some other way for the CrashLog object to get to the port without passing as argument?

Comment on attachment 203616 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203616&action=review >> Tools/Scripts/webkitpy/common/system/crashlogs.py:83 >> + log_directory = port.results_directory() > > I only needed to pass in 'port' to get access to this directory. Is there some other way for the CrashLog object to get to the port without passing as argument? Since log_directory is only used in the call to files_under, why don't we just call it there? > Tools/Scripts/webkitpy/common/system/crashlogs.py:85 > + pid_line_regex = re.compile('\s+Global\s+PID:\s+\[(?P<pid>\d+)\]') Since this doesn't change, why don't we put it as a static value? > Tools/Scripts/webkitpy/common/system/crashlogs.py:86 > + errors = '' Ser errors to None. > Tools/Scripts/webkitpy/common/system/crashlogs.py:98 > + if (int(match.group('pid')) == pid): The outer parenthesis is not needed. > Tools/Scripts/webkitpy/port/win.py:239 > + winpid = self._executive.pid_to_sys_pid.get(pid) Nit: we should call this system_id to be consistent. > Tools/Scripts/webkitpy/port/win.py:268 > + if not crash_log: > + continue > + crash_logs[test_name] = crash_log It's probably cleaner to write it as: if crash_log: crash_logs[test_name] = crash_log > Tools/Scripts/webkitpy/port/win.py:272 > + self.win_pid = int(pid) Why are we setting this as an instance variable? It's not used anywhere else so I don't think this is intended. r- because of this. Also, we should call this system_pid to be consistent.

Comment on attachment 203616 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203616&action=review >> Tools/Scripts/webkitpy/common/system/crashlogs.py:86 >> + errors = '' > > Ser errors to None. I can't -- this is a string that is appended to in the stanzas below (this is duplicated from the mac logic).

Comment on attachment 203616 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203616&action=review >>> Tools/Scripts/webkitpy/common/system/crashlogs.py:83 >>> + log_directory = port.results_directory() >> >> I only needed to pass in 'port' to get access to this directory. Is there some other way for the CrashLog object to get to the port without passing as argument? > > Since log_directory is only used in the call to files_under, why don't we just call it there? Done >> Tools/Scripts/webkitpy/common/system/crashlogs.py:85 >> + pid_line_regex = re.compile('\s+Global\s+PID:\s+\[(?P<pid>\d+)\]') > > Since this doesn't change, why don't we put it as a static value? Done >> Tools/Scripts/webkitpy/common/system/crashlogs.py:98 >> + if (int(match.group('pid')) == pid): > > The outer parenthesis is not needed. Done >> Tools/Scripts/webkitpy/port/win.py:239 >> + winpid = self._executive.pid_to_sys_pid.get(pid) > > Nit: we should call this system_id to be consistent. Done >> Tools/Scripts/webkitpy/port/win.py:268 >> + crash_logs[test_name] = crash_log > > It's probably cleaner to write it as: > if crash_log: > crash_logs[test_name] = crash_log Done >> Tools/Scripts/webkitpy/port/win.py:272 >> + self.win_pid = int(pid) > > Why are we setting this as an instance variable? It's not used anywhere else so I don't think this is intended. > r- because of this. > > Also, we should call this system_pid to be consistent. Corrected.

Created attachment 203618 [details] Patch

Comment on attachment 203618 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203618&action=review > Tools/Scripts/webkitpy/common/system/crashlogs.py:29 > +import codecs We don't need this once you address the comment below. > Tools/Scripts/webkitpy/common/system/crashlogs.py:82 > + def _find_newest_log_win(self, process_name, pid, include_errors, newer_than, port): We can't write any test for this method? > Tools/Scripts/webkitpy/common/system/crashlogs.py:95 > + with codecs.open(path, 'r', 'utf8', 'ignore') as ff: > + f = ff.read() Please use filesystem.read_text_file.

(In reply to comment #8) > (From update of attachment 203618 [details]) > > Tools/Scripts/webkitpy/common/system/crashlogs.py:95 > > + with codecs.open(path, 'r', 'utf8', 'ignore') as ff: > > + f = ff.read() > > Please use filesystem.read_text_file. I can't, because I need to pass the 'ignore' flag to the underlying codecs.open call so that I can handle the malformed Windows log files. I could modify the existing read_text_file to add an optional error-handling argument. Would that be okay?

(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 203618 [details] [details]) > > > Tools/Scripts/webkitpy/common/system/crashlogs.py:95 > > > + with codecs.open(path, 'r', 'utf8', 'ignore') as ff: > > > + f = ff.read() > > > > Please use filesystem.read_text_file. > > I can't, because I need to pass the 'ignore' flag to the underlying codecs.open call so that I can handle the malformed Windows log files. > Ick. Can you read the file as a straight binary file (using read_binary_file)? I'm guessing python might be okay with that. > I could modify the existing read_text_file to add an optional error-handling argument. Would that be okay? I'd prefer not to if there's any way around it. The only reason I'd be open to allowing the flag would be for us to be able to more easily write tests for this, but even then I'd probably rather add a separate _read() call on this object that you could override to mock.

Created attachment 203635 [details] Patch

Attachment 203635 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/common/system/crashlogs.py', u'Tools/Scripts/webkitpy/common/system/crashlogs_unittest.py', u'Tools/Scripts/webkitpy/common/system/executive.py', u'Tools/Scripts/webkitpy/common/system/executive_mock.py', u'Tools/Scripts/webkitpy/common/system/filesystem.py', u'Tools/Scripts/webkitpy/common/system/filesystem_mock.py', u'Tools/Scripts/webkitpy/common/system/filesystem_unittest.py', u'Tools/Scripts/webkitpy/port/base.py', u'Tools/Scripts/webkitpy/port/server_process.py', u'Tools/Scripts/webkitpy/port/win.py', u'Tools/Scripts/webkitpy/port/win_unittest.py']" exit_code: 1 Tools/Scripts/webkitpy/common/system/filesystem_mock.py:324: [MockFileSystem.read_text_file] An attribute affected in webkitpy.common.system.crashlogs_unittest line 271 hide this method [pylint/E0202] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 203635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203635&action=review > Tools/Scripts/webkitpy/common/system/crashlogs.py:35 > + PID_LINE_REGEX = re.compile('\s+Global\s+PID:\s+\[(?P<pid>\d+)\]') Use r'~'. > Tools/Scripts/webkitpy/common/system/crashlogs.py:96 > + f = self._host.filesystem.read_text_file(path, 'ignore') > + match = self.PID_LINE_REGEX.search(f) > + if match is None: > + continue > + if int(match.group('pid')) == pid: > + return errors + f Please don't use one-letter variable like f. I'd call this variable log_file.

Comment on attachment 203635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203635&action=review > Tools/Scripts/webkitpy/common/system/executive.py:279 > + ps_process = self.run_command(['ps', '-e'], error_handler=Executive.ignore_error) Won't this only work under cygwin?

Comment on attachment 203635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203635&action=review >> Tools/Scripts/webkitpy/common/system/crashlogs.py:35 >> + PID_LINE_REGEX = re.compile('\s+Global\s+PID:\s+\[(?P<pid>\d+)\]') > > Use r'~'. Doesn't the "r" string decoration mean that it will not treat escape characters specially? I want to use the escape characters as they are written. Probably I am misunderstanding your comment: Is there some built-in feature for regular expression building you are wanting me to use? >> Tools/Scripts/webkitpy/common/system/crashlogs.py:96 >> + return errors + f > > Please don't use one-letter variable like f. I'd call this variable log_file. Done >> Tools/Scripts/webkitpy/common/system/executive.py:279 >> + ps_process = self.run_command(['ps', '-e'], error_handler=Executive.ignore_error) > > Won't this only work under cygwin? Ah, right -- I'll change that to: if sys.platform == "cygwin":

Created attachment 203648 [details] Patch

Comment on attachment 203648 [details] Patch No longer pass 'errors' argument to {read|write}_text_file. Resolve dpranke/rniwa comments.

(In reply to comment #10) > (In reply to comment #9) > > > Please use filesystem.read_text_file. > > > > I can't, because I need to pass the 'ignore' flag to the underlying codecs.open call so that I can handle the malformed Windows log files. > > > > Ick. Can you read the file as a straight binary file (using read_binary_file)? I'm guessing python might be okay with that. You are right. I used that with a combination of 'decode' to get what I wanted. > > I could modify the existing read_text_file to add an optional error-handling argument. Would that be okay? > > I'd prefer not to if there's any way around it. I no longer need the change; I've backed it out.

Committed r151151: <http://trac.webkit.org/changeset/151151>

Comment on attachment 203648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203648&action=review You missed a couple of things. Can you file a follow-up bug/patch? > Tools/Scripts/webkitpy/common/system/crashlogs.py:40 > + def find_newest_log(self, process_name, pid=None, include_errors=False, newer_than=None, port=None): You didn't address the comment I had about passing in results_directory instead of the port (or moving the port to the __init__ call) > Tools/Scripts/webkitpy/common/system/executive.py:278 > + if sys.platform in ("cygwin"): this makes win32 take the else branch, which is wrong.

Reopening to address dpranke's comments.

Moved corrections to Bug 117212.