[webkit-reviews] review denied: [Bug 236968] Allow run-webkit-archive to launch arbitrary binaries : [Attachment 452725] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Sam Sneddon [:gsnedders]
<gsnedders at apple.com>'s request for review:
Bug 236968: Allow run-webkit-archive to launch arbitrary binaries
https://bugs.webkit.org/show_bug.cgi?id=236968

Attachment 452725: Patch

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




--- Comment #2 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 452725
  --> https://bugs.webkit.org/attachment.cgi?id=452725
Patch

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

It's a little sad to duplicate run-webkit-app functionality, but this does seem
necessary for inclusion in archives.

It's also somewhat unfortunate that this is going to require Xcode installation
- would be nicer to use shell. Seems acceptable though, this script isn't used
all that much to super-optimize it usability.

Looks good overall, but I would like to take a look at another iteration, so r-
for now.

> Tools/WebKitArchiveSupport/run-webkit-archive:1
> +#!/usr/bin/env xcrun python3

Yay python3!

We use "/usr/bin/env python3" elsewhere, what is the reason to use xcrun? If
you use it, it should be just "/usr/bin/xcrun python3", but I think that
"/usr/bin/env python3" is right.

> Tools/WebKitArchiveSupport/run-webkit-archive:49
> +    battr = attribute.encode("utf-8")  # on macOS, xattr names are always
UTF-8

I'm asking to remove this function altogether, but as a general note, WebKit
style is to use full sentences in comments, with capitalization and periods.

> Tools/WebKitArchiveSupport/run-webkit-archive:107
> +	   if hasxattr(path, "com.apple.quarantine"):

Seems like using xattr CLI tool would be substantially simpler, can you do
that? It can show, set and remove attributes, I didn't see anything needed here
that it doesn't do.

> Tools/WebKitArchiveSupport/run-webkit-archive:112
> +def scary_warning():

I don't understand the purpose of this. The user has already run this script
that can remove quarantine, what is the point of the script warning about
itself?

> Tools/WebKitArchiveSupport/run-webkit-archive:147
> +	   "--override-system-security",

Will the user ever want the script to fail? What it the reason to require an
explicit "please to not fail" option?

> Tools/WebKitArchiveSupport/run-webkit-archive:201
> +    if "Darwin" not in platform.system():
> +	   print("Unsupported OS, exiting.")

This message leaves the user in the dark unnecessarily, could just say
something like "This script only works to launch WebKit archive builds for
macOS".

Note that the script as uploaded wouldn't even get here, for the lack of xcrun.

> Tools/WebKitArchiveSupport/run-webkit-archive:212
> +	   print(
> +	       "No Release or Debug framework directories found in the current
folder, exiting."
> +	   )

Please make this one line.