[webkit-reviews] review granted: [Bug 227333] Build Default Metal library offline, and install to WebCore Resources : [Attachment 433631] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Kyle Piddington
<kpiddington at apple.com>'s request for review:
Bug 227333: Build Default Metal library offline, and install to WebCore
Resources
https://bugs.webkit.org/show_bug.cgi?id=227333

Attachment 433631: Patch

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




--- Comment #18 from Kenneth Russell <kbr at google.com> ---
Comment on attachment 433631
  --> https://bugs.webkit.org/attachment.cgi?id=433631
Patch

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

Alexey's review is more thorough than I can provide - I don't know Xcode well
enough to review those changes - but conceptually this sounds fine. Happy to
look at further iterations as well. r+

>> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:3702
>> +			shellScript = "echo python
\"${SRCROOT}/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py
\" \"$SCRIPT_INPUT_FILE_0\" \"$SCRIPT_OUTPUT_FILE_0\"\npython
\"${SRCROOT}/src/libANGLE/renderer/metal/shaders/create_mtl_internal_shaders.py
\" \"$SCRIPT_INPUT_FILE_0\" \"$SCRIPT_OUTPUT_FILE_0\"\nrm
\"${SCRIPT_INPUT_FILE_0}\"\n";
> 
> Why does this rm SCRIPT_INPUT_FILE_0 in the end? This means that incremental
builds will always have to redo this work, and that's harmful to build speed.
> 
> If the problem is that the file goes into $(BUILT_PRODUCTS_DIR), perhaps
that's what needs to change.

Agreed - this should avoid overwriting the output file if nothing's changed.
Take a look at Source/ThirdParty/ANGLE/adjust-angle-include-paths.py and the
Xcode build rule which invokes it to see how it avoids redundant work. (It
looks like it caches the file's contents in memory so it can compare with
what's currently on disk.) I don't remember who most recently improved the
incremental build support (krollin@ maybe?) and have been looking through the
revision history of Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj to
see.