[webkit-reviews] review canceled: [Bug 187687] Add --target-path option to dump-class-layout : [Attachment 345065] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has canceled Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 187687: Add --target-path option to dump-class-layout
https://bugs.webkit.org/show_bug.cgi?id=187687

Attachment 345065: Patch

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




--- Comment #4 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 345065
  --> https://bugs.webkit.org/attachment.cgi?id=345065
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.