Bug 172033 - webkitpy: Pass directory with crash logs into CrashLogs
Summary: webkitpy: Pass directory with crash logs into CrashLogs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks: 171976
  Show dependency treegraph
 
Reported: 2017-05-12 09:56 PDT by Jonathan Bedard
Modified: 2017-05-12 13:39 PDT (History)
6 users (show)

See Also:


Attachments
Patch (19.18 KB, patch)
2017-05-12 10:14 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (19.24 KB, patch)
2017-05-12 12:10 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-05-12 09:56:47 PDT
Currently, CrashLogs calculates the path to the crash-logs itself.  This path should be generated by the port.
Comment 1 Radar WebKit Bug Importer 2017-05-12 09:57:38 PDT
<rdar://problem/32157616>
Comment 2 Jonathan Bedard 2017-05-12 10:14:37 PDT
Created attachment 309907 [details]
Patch
Comment 3 Daniel Bates 2017-05-12 11:37:04 PDT
Comment on attachment 309907 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/crashlogs_unittest.py:239
> +    def crash_log_path(self):
> +        return '/Users/mock/Library/Logs/DiagnosticReports'

It seems sufficient to define a class constant, maybe DARWIN_MOCK_CRASH_DIRECTORY, to be "/Users/mock/Library/Logs/DiagnosticReports" instead of defining a function. Notice that I explicitly mentioned "DARWIN" in the name of the constant since the path it represents is only meaningful on Darwin-based OSes.

> Tools/Scripts/webkitpy/port/base.py:1351
> +        return self.results_directory()

I take it we consider the location of the crash log on Windows typical for most ports. That is, the crash log/core dump will be next to the executable.

> Tools/Scripts/webkitpy/port/darwin.py:93
> +        if self.host.filesystem.exists(self.host.filesystem.join(log_directory, 'DiagnosticReports')):
> +            return self.host.filesystem.join(log_directory, 'DiagnosticReports')

We may compute the path to the DiagnosticReports directory twice - once to check its existence and once more if it exists. I suggest that we compute it once and cache it in a local variable.

> Tools/Scripts/webkitpy/port/darwin.py:95
> +        else:
> +            return self.host.filesystem.join(log_directory, 'CrashReporter')

We tend to follow the WebKit Code Style Guidelines such that this return statement should be outside of the else scope since the preceding if concludes with a return statement [1].

[1] <https://webkit.org/code-style-guidelines/#linebreaking-else-if>

> Tools/Scripts/webkitpy/port/ios_device_unittest.py:42
> +    def test_crashlog_path(self):
> +        pass

What is the purpose of this test method?
Comment 4 Jonathan Bedard 2017-05-12 12:03:59 PDT
Comment on attachment 309907 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/base.py:1351
>> +        return self.results_directory()
> 
> I take it we consider the location of the crash log on Windows typical for most ports. That is, the crash log/core dump will be next to the executable.

Correct. Additionally, even if this assumption is incorrect, it makes generalizing crash logic easier because this directory will definitely exist.

>> Tools/Scripts/webkitpy/port/ios_device_unittest.py:42
>> +        pass
> 
> What is the purpose of this test method?

The IOSDeviceTest inherits from DarwinTest (through IOSTest).  Until we have implemented the crash log path for iOS, we need this to keep tests passing.
Comment 5 Jonathan Bedard 2017-05-12 12:10:18 PDT
Created attachment 309924 [details]
Patch
Comment 6 Daniel Bates 2017-05-12 12:14:08 PDT
(In reply to Jonathan Bedard from comment #4)
> Comment on attachment 309907 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309907&action=review
> 
> >> Tools/Scripts/webkitpy/port/base.py:1351
> >> +        return self.results_directory()
> > 
> > I take it we consider the location of the crash log on Windows typical for most ports. That is, the crash log/core dump will be next to the executable.
> 
> Correct.

OK.

> Additionally, even if this assumption is incorrect, it makes
> generalizing crash logic easier because this directory will definitely exist.
> 

I'm unclear how the existence of a directory "makes generalizing crash logic easier". It seems that if crash logs/core dumps are not placed in self.results_directory() for some port/OS P and CrashLog was taught to work for P then it would not find any crash logs. This would likely surprise the implementer that taught CrashLog to work for P and though discoverable when testing ones code would be more easily discoverable (and less surprising) if the default implementation of Port.path_to_crash_logs raised a NotImplementedError. The exception is more discoverable because it is not subtle and it would immediately force the implementer to implement this method for P.

> >> Tools/Scripts/webkitpy/port/ios_device_unittest.py:42
> >> +        pass
> > 
> > What is the purpose of this test method?
> 
> The IOSDeviceTest inherits from DarwinTest (through IOSTest).  Until we have
> implemented the crash log path for iOS, we need this to keep tests passing.

:(
Comment 7 WebKit Commit Bot 2017-05-12 12:52:21 PDT
Comment on attachment 309924 [details]
Patch

Clearing flags on attachment: 309924

Committed r216776: <http://trac.webkit.org/changeset/216776>
Comment 8 WebKit Commit Bot 2017-05-12 12:52:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Jonathan Bedard 2017-05-12 13:39:13 PDT
(In reply to Daniel Bates from comment #6)
> 
> ...
>
> I'm unclear how the existence of a directory "makes generalizing crash logic
> easier". It seems that if crash logs/core dumps are not placed in
> self.results_directory() for some port/OS P and CrashLog was taught to work
> for P then it would not find any crash logs. This would likely surprise the
> implementer that taught CrashLog to work for P and though discoverable when
> testing ones code would be more easily discoverable (and less surprising) if
> the default implementation of Port.path_to_crash_logs raised a
> NotImplementedError. The exception is more discoverable because it is not
> subtle and it would immediately force the implementer to implement this
> method for P.
> 
> ...
>

That's a good point.  <https://trac.webkit.org/r216788> changes it so that it will throw an exception by default.