Bug 236968 – Allow run-webkit-archive to launch arbitrary binaries

• Component: Tools / Tests
• Status: NEW

[Sam Sneddon [:gsnedders]]

With Safari often not working, we should make it possible for run-webkit-archive to run other binaries, most obviously MiniBrowser.app (which is included in the archive).

[Sam Sneddon [:gsnedders]]

Created attachment 452725 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 452725 [details]
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.

[Radar WebKit Bug Importer]

<rdar://problem/89553179>