[webkit-reviews] review granted: [Bug 176965] teach build.webkit.org to include run-webkit-archive in the root folder of uploaded macOS archives : [Attachment 320927] v2 patch, this one changes the working folder to the script folder, and will allow double click execution

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Lucas Forschler
<lforschler at apple.com>'s request for review:
Bug 176965: teach build.webkit.org to include run-webkit-archive in the root
folder of uploaded macOS archives
https://bugs.webkit.org/show_bug.cgi?id=176965

Attachment 320927: v2 patch, this one changes the working folder to the script
folder, and will allow double click execution

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




--- Comment #5 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 320927
  --> https://bugs.webkit.org/attachment.cgi?id=320927
v2 patch, this one changes the working folder to the script folder, and will
allow double click execution

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

> Tools/ChangeLog:8
> +	   * BuildSlaveSupport/built-product-archive:

This is not new to the patch, but the name of the script doesn't seem to parse.
Is it "build product archive", or "archive built product"?

> Tools/BuildSlaveSupport/built-product-archive:141
> +    command = ['zip', '-j', archiveFile, PATH_TO_LAUNCHER]

FWIW I like to use full paths in scripts, /usr/bin/zip.

> Tools/BuildSlaveSupport/built-product-archive:161
> +	   return subprocess.call(command) or addLauncherToArchive(archiveFile)
  

trailing spaces in this line

> Tools/Scripts/run-webkit-archive:56
>      os.environ['DYLD_LIBRARY_PATH'] = dyld_path

Looking at this, I'm wondering if we also need __XPC_ variants. Did you confirm
that the downloaded frameworks are actually used in WebContent?

> Tools/Scripts/run-webkit-archive:60
> +    script_path = os.path.abspath(__file__)

These changes are not mentioned in ChangeLog.

Not new to the patch either, but this file should probably be hidden somewhere
(maybe in Tools/BuildSlaveSupport). It's not great to add a new script in a
directory that's in every WebKit developer's path when we are not going to ever
run it from this location.