Bug 182268

Summary: Add a build step to copy resources from WebKitAdditions as bundle resources in WebKit
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebKit Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, commit-queue, joepeck, mitz, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
thorton: review+
Patch
none
Patch for landing none

Description Wenson Hsieh 2018-01-29 15:55:42 PST
<rdar://problem/37003784>
Comment 1 Wenson Hsieh 2018-01-29 16:28:38 PST
Created attachment 332597 [details]
Patch
Comment 2 mitz 2018-01-29 17:45:58 PST
Comment on attachment 332597 [details]
Patch

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

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10022
> +			name = "Copy additional resources";

Why not use the same capitalization style as the other build phases?

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10027
> +			shellScript = "SOURCE=\"$SDKROOT/usr/local/include/WebKitAdditions/WebKit/AdditionalResources\"\nDESTINATION=\"$BUILT_PRODUCTS_DIR/WebKit.framework\"\nif [ -d \"$SOURCE\" ]\nthen\n    rsync -aq --exclude \".svn\" --exclude \".DS_Store\" $SOURCE/* \"$DESTINATION\"\nfi";

Some ideas on how to improve this:
- use $SDK_DIR instead of $SDKROOT
- account for local builds of WebKitAdditions in the built products directories before falling back on the SDK
- use ditto
- don’t behave as if the SDK may contain items named .svn or .DS_Store
- copy into $UNLOCALIZED_RESOURCES_FOLDER_PATH so that it doesn’t depend on the iOS shallow bundle structure
- use modern [[ instead of quirky [
- set -e
Comment 3 Wenson Hsieh 2018-01-29 19:14:43 PST
Comment on attachment 332597 [details]
Patch

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

>> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10022
>> +			name = "Copy additional resources";
> 
> Why not use the same capitalization style as the other build phases?

Good catch! Should definitely be capitalized.

>> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10027
>> +			shellScript = "SOURCE=\"$SDKROOT/usr/local/include/WebKitAdditions/WebKit/AdditionalResources\"\nDESTINATION=\"$BUILT_PRODUCTS_DIR/WebKit.framework\"\nif [ -d \"$SOURCE\" ]\nthen\n    rsync -aq --exclude \".svn\" --exclude \".DS_Store\" $SOURCE/* \"$DESTINATION\"\nfi";
> 
> Some ideas on how to improve this:
> - use $SDK_DIR instead of $SDKROOT
> - account for local builds of WebKitAdditions in the built products directories before falling back on the SDK
> - use ditto
> - don’t behave as if the SDK may contain items named .svn or .DS_Store
> - copy into $UNLOCALIZED_RESOURCES_FOLDER_PATH so that it doesn’t depend on the iOS shallow bundle structure
> - use modern [[ instead of quirky [
> - set -e

- switched to $SDK_DIR
- changed to try and find additional resources in the built products directory first, then fall back to SDK
- changed to use ditto.
- removed unneeded .svn and .DS_Store exclusion
- switched to use $UNLOCALIZED_RESOURCES_FOLDER_PATH
- changed to [[
- added `set -e` at the beginning

Thanks for the suggestions!
Comment 4 Wenson Hsieh 2018-01-29 20:13:57 PST
Created attachment 332624 [details]
Patch
Comment 5 mitz 2018-01-29 20:37:54 PST
Comment on attachment 332624 [details]
Patch

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

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10022
> +			shellScript = "set -e\n\nRELATIVE_SOURCE_PATH=\"usr/local/include/WebKitAdditions/WebKit/AdditionalResources\"\nSOURCE_PATH=\"$BUILT_PRODUCTS_DIR/$RELATIVE_SOURCE_PATH\"\n\nif [[ ! -d \"$SOURCE_PATH\" ]]; then\n    SOURCE_PATH=\"$SDK_DIR/$RELATIVE_SOURCE_PATH\"\nfi\n\nif [[ -d \"$SOURCE_PATH\" ]]; then\n    ditto $SOURCE_PATH/* \"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH\"\nfi";

I think the * there is unnecessary (and harmful if the source directory ever contained subdirectories).
Comment 6 Wenson Hsieh 2018-01-29 20:43:10 PST
(In reply to mitz from comment #5)
> Comment on attachment 332624 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332624&action=review
> 
> > Source/WebKit/WebKit.xcodeproj/project.pbxproj:10022
> > +			shellScript = "set -e\n\nRELATIVE_SOURCE_PATH=\"usr/local/include/WebKitAdditions/WebKit/AdditionalResources\"\nSOURCE_PATH=\"$BUILT_PRODUCTS_DIR/$RELATIVE_SOURCE_PATH\"\n\nif [[ ! -d \"$SOURCE_PATH\" ]]; then\n    SOURCE_PATH=\"$SDK_DIR/$RELATIVE_SOURCE_PATH\"\nfi\n\nif [[ -d \"$SOURCE_PATH\" ]]; then\n    ditto $SOURCE_PATH/* \"$BUILT_PRODUCTS_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH\"\nfi";
> 
> I think the * there is unnecessary (and harmful if the source directory ever
> contained subdirectories).

Ah, I wasn't aware that ditto had different semantics than rsync when the source is a folder. Fixed!
Comment 7 Wenson Hsieh 2018-01-29 21:00:57 PST
Created attachment 332627 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2018-01-29 21:36:42 PST
Comment on attachment 332627 [details]
Patch for landing

Clearing flags on attachment: 332627

Committed r227771: <https://trac.webkit.org/changeset/227771>
Comment 9 Radar WebKit Bug Importer 2018-01-29 21:43:21 PST
<rdar://problem/37015528>