| Summary: | Allow run-webkit-archive to launch arbitrary binaries | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sam Sneddon [:gsnedders] <gsnedders> | ||||
| Component: | Tools / Tests | Assignee: | Sam Sneddon [:gsnedders] <gsnedders> | ||||
| Status: | NEW --- | ||||||
| Severity: | Normal | CC: | ap, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=226560 https://bugs.webkit.org/show_bug.cgi?id=189234 https://bugs.webkit.org/show_bug.cgi?id=270507 |
||||||
| Attachments: |
|
||||||
|
Description
Sam Sneddon [:gsnedders]
2022-02-21 05:16:07 PST
Created attachment 452725 [details]
Patch
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. |