Bug 187687 - Add --target-path option to dump-class-layout
Summary: Add --target-path option to dump-class-layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-15 14:17 PDT by Yusuke Suzuki
Modified: 2019-05-02 16:25 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.46 KB, patch)
2018-07-15 14:19 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (2.48 KB, patch)
2018-07-15 14:21 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (2.58 KB, patch)
2018-07-15 18:05 PDT, Yusuke Suzuki
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-07-15 14:17:22 PDT
Add --target option to dump-class-layout
Comment 1 Yusuke Suzuki 2018-07-15 14:19:14 PDT
Created attachment 345064 [details]
Patch
Comment 2 Yusuke Suzuki 2018-07-15 14:21:14 PDT
Created attachment 345065 [details]
Patch
Comment 3 Daniel Bates 2018-07-15 16:21:48 PDT
Comment on attachment 345065 [details]
Patch

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

> Tools/Scripts/dump-class-layout:78
> +    if not args.target_path == None:

I do not know much about the JSCOnly port. Would it make sense (and solve the problem your are having) to expose a —root option like we do for run-webkit-tests to provide a path to an arbitrary directory for the built products? If so, I suggest we do that for consistency.

If we choose to go with the approach proposed in this patch then please change “== None”  to “is None” (per PEP8)  and I am assuming that argparse will emit and error if target-path is the empty string. Otherwise, we should handle that. Again I would prefer we add —root over —target-path.
Comment 4 Yusuke Suzuki 2018-07-15 18:04:05 PDT
Comment on attachment 345065 [details]
Patch

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

>> Tools/Scripts/dump-class-layout:78
>> +    if not args.target_path == None:
> 
> I do not know much about the JSCOnly port. Would it make sense (and solve the problem your are having) to expose a —root option like we do for run-webkit-tests to provide a path to an arbitrary directory for the built products? If so, I suggest we do that for consistency.
> 
> If we choose to go with the approach proposed in this patch then please change “== None”  to “is None” (per PEP8)  and I am assuming that argparse will emit and error if target-path is the empty string. Otherwise, we should handle that. Again I would prefer we add —root over —target-path.

I don't think `--root` solves the problem. While macOS ports build JSC and WebCore as `framework`, JSCOnly, GTK, and WPE ports are not. They build JSC and WebCore as shared library `.so`.
Furthermore, the names of shared libraries are slightly different in each port. JSCOnly port's shared library is `libJavaScriptCore.so`, GTK ports one is `libjavascriptcoregtk-4.0.so`.
To support these things with `--root` option, we need to handle these differences in python code, which complicates dump-class-layout.

Instead of doing that, just passing a library with `--target-path` is simple. So I would like to choose this approach.

I'll fix `is None` thing and upload the patch.
Comment 5 Yusuke Suzuki 2018-07-15 18:05:08 PDT
Created attachment 345071 [details]
Patch
Comment 7 Simon Fraser (smfr) 2018-07-16 08:39:23 PDT
Comment on attachment 345071 [details]
Patch

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

> Tools/ChangeLog:8
> +        We add a hatch to dump-class-layout for specifying target path directly.

A hatch? Maybe "argument"?
Comment 8 Yusuke Suzuki 2018-07-16 11:51:47 PDT
Comment on attachment 345071 [details]
Patch

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

>> Tools/ChangeLog:8
>> +        We add a hatch to dump-class-layout for specifying target path directly.
> 
> A hatch? Maybe "argument"?

Ah, that means `an escape hatch`. Fixed.
Comment 9 Yusuke Suzuki 2018-07-16 11:52:27 PDT
Committed r233856: <https://trac.webkit.org/changeset/233856>
Comment 10 Radar WebKit Bug Importer 2018-07-16 11:53:19 PDT
<rdar://problem/42249696>