Bug 117098 – [Windows] NRWT is not putting crash logs in proper place

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Brent Fulgham]

The layout test result page created by NRWT expects crash logs to be placed in particular locations using specific names.  The current implementation of this logic for the Windows port is doing the wrong things, so these end up being dead links.

This patch corrects the Windows logic to move the crash files to the proper place.

[Radar WebKit Bug Importer]

<rdar://problem/14039502>

[Brent Fulgham]

Created attachment 203616 [details]
Patch

[Brent Fulgham]

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?

[Ryosuke Niwa]

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.

[Brent Fulgham]

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).

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 203618 [details]
Patch

[Ryosuke Niwa]

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.

[Brent Fulgham]

(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?

[Dirk Pranke]

(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.

[Brent Fulgham]

Created attachment 203635 [details]
Patch

[WebKit Commit Bot]

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.

[Ryosuke Niwa]

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.

[Dirk Pranke]

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?

[Brent Fulgham]

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":

[Brent Fulgham]

Created attachment 203648 [details]
Patch

[Brent Fulgham]

Comment on attachment 203648 [details]
Patch

No longer pass 'errors' argument to {read|write}_text_file.  Resolve dpranke/rniwa comments.

[Brent Fulgham]

(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.

[Brent Fulgham]

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

[Dirk Pranke]

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.

[Brent Fulgham]

Reopening to address dpranke's comments.

[Brent Fulgham]

Moved corrections to Bug 117212.