Bug 188601 - Add script to generate WebContent service resource files and change XPC service main SPI to have it's own header
Summary: Add script to generate WebContent service resource files and change XPC servi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ben Richards
URL:
Keywords: InRadar
Depends on: 188683
Blocks:
  Show dependency treegraph
 
Reported: 2018-08-15 07:53 PDT by Ben Richards
Modified: 2018-09-18 13:54 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.15 KB, patch)
2018-08-15 07:59 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (5.66 KB, patch)
2018-08-15 09:24 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (5.93 KB, patch)
2018-08-15 11:05 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (4.43 KB, patch)
2018-08-15 17:15 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (6.35 KB, patch)
2018-08-16 12:23 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch for landing (6.50 KB, patch)
2018-08-16 13:33 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (17.58 KB, patch)
2018-08-17 11:25 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (15.43 KB, patch)
2018-08-17 16:02 PDT, Ben Richards
no flags Details | Formatted Diff | Diff
Patch (16.06 KB, patch)
2018-08-17 17:35 PDT, Ben Richards
rniwa: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch (6.69 KB, patch)
2018-09-17 15:50 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
patch (6.84 KB, patch)
2018-09-18 12:33 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
patch (8.73 KB, patch)
2018-09-18 13:52 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Richards 2018-08-15 07:53:30 PDT
Add script to generate WebContent service resource files
Comment 1 Ben Richards 2018-08-15 07:59:04 PDT
Created attachment 347162 [details]
Patch
Comment 2 Sam Weinig 2018-08-15 09:20:29 PDT
Comment on attachment 347162 [details]
Patch

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

> Tools/Scripts/generate-webcontent-resources:1
> +#!/usr/bin/env python

Please add a license to this file.
Comment 3 Ben Richards 2018-08-15 09:24:15 PDT
Created attachment 347166 [details]
Patch
Comment 4 Ben Richards 2018-08-15 11:05:49 PDT
Created attachment 347178 [details]
Patch
Comment 5 mitz 2018-08-15 13:12:10 PDT
Comment on attachment 347178 [details]
Patch

Some of the things the script does try to duplicate things that the build system does when building the WebContent service—and those things would need to be kept in sync between the script and the other parts of the project whenever they change.

I imagine that the next step after this patch is to have this script run as part of building the WebContent target (rather than keep it around for people to run it manually).

If that’s the case, I think you should consider a different approach to producing these resources, which would require far less duplication and keeping-things-in-sync: leverage the existing INFOPLIST_FILE and RUNLOOP_TYPE build settings as resolved for the platform being built, and get the final entitlements as written by the process-webcontent-or-plugin-entitlements.sh script into $(TEMP_FILE_DIR)/$(FULL_PRODUCT_NAME).xcent.

What do you think?
Comment 6 Ben Richards 2018-08-15 13:57:18 PDT
(In reply to mitz from comment #5)
> Comment on attachment 347178 [details]
> Patch
> 
> Some of the things the script does try to duplicate things that the build
> system does when building the WebContent service—and those things would need
> to be kept in sync between the script and the other parts of the project
> whenever they change.
> 
> I imagine that the next step after this patch is to have this script run as
> part of building the WebContent target (rather than keep it around for
> people to run it manually).
> 
> If that’s the case, I think you should consider a different approach to
> producing these resources, which would require far less duplication and
> keeping-things-in-sync: leverage the existing INFOPLIST_FILE and
> RUNLOOP_TYPE build settings as resolved for the platform being built, and
> get the final entitlements as written by the
> process-webcontent-or-plugin-entitlements.sh script into
> $(TEMP_FILE_DIR)/$(FULL_PRODUCT_NAME).xcent.
> 
> What do you think?

My current idea for next steps would be to use this script in a build phase of any client application that would like to create a custom WebContent service. I liked this idea because it would allow the client application to specify where to place these resource files at build time (such as a derived sources directory).

Regarding your suggestion,

I see what you mean about RUNLOOP_TYPE. That should be moved to a build setting in the client's custom WebContent service and be resolved for the platform being built. But overall I think this script should remain in sync as long as paths to these specific files aren't changed (and my guess is that they don't change often).

I guess I may be slightly confused as to your proposed alternative approach. Is the suggestion that we should have the WebContent service place the Info.plist, WebContentProcess.xib and final .xcent file in a well known location at build time so that they can be used by a client application?
Comment 7 mitz 2018-08-15 14:05:17 PDT
(In reply to Ben Richards from comment #6)
> (In reply to mitz from comment #5)
> > Comment on attachment 347178 [details]
> > Patch
> > 
> > Some of the things the script does try to duplicate things that the build
> > system does when building the WebContent service—and those things would need
> > to be kept in sync between the script and the other parts of the project
> > whenever they change.
> > 
> > I imagine that the next step after this patch is to have this script run as
> > part of building the WebContent target (rather than keep it around for
> > people to run it manually).
> > 
> > If that’s the case, I think you should consider a different approach to
> > producing these resources, which would require far less duplication and
> > keeping-things-in-sync: leverage the existing INFOPLIST_FILE and
> > RUNLOOP_TYPE build settings as resolved for the platform being built, and
> > get the final entitlements as written by the
> > process-webcontent-or-plugin-entitlements.sh script into
> > $(TEMP_FILE_DIR)/$(FULL_PRODUCT_NAME).xcent.
> > 
> > What do you think?
> 
> My current idea for next steps would be to use this script in a build phase
> of any client application that would like to create a custom WebContent
> service. I liked this idea because it would allow the client application to
> specify where to place these resource files at build time (such as a derived
> sources directory).

This idea is not compatible with how software gets built in Apple’s production build system: when one of those client application gets built, it doesn’t have access to the WebKit source tree, which contains the script and the various files it references.


> 
> Regarding your suggestion,
> 
> I see what you mean about RUNLOOP_TYPE. That should be moved to a build
> setting in the client's custom WebContent service and be resolved for the
> platform being built. But overall I think this script should remain in sync
> as long as paths to these specific files aren't changed (and my guess is
> that they don't change often).
> 
> I guess I may be slightly confused as to your proposed alternative approach.
> Is the suggestion that we should have the WebContent service place the
> Info.plist, WebContentProcess.xib and final .xcent file in a well known
> location at build time so that they can be used by a client application?

Yes, exactly.
Comment 8 Ben Richards 2018-08-15 15:18:42 PDT
Understood. Do you have a recommendation for a good well known location?
Comment 9 mitz 2018-08-15 15:26:11 PDT
(In reply to Ben Richards from comment #8)
> Understood. Do you have a recommendation for a good well known location?

A new subdirectory of the existing WebKit.framework/PrivateHeaders directory is a good place for this because:

1. It is part of WebKit, and already contains content used when building other projects (Safari uses surf from the Scripts subdirectory in there)
2. Header content is included in the SDK, and is therefore available to other projects building against the SDK, but is not included in the OS, so it doesn’t take up space on non-developers’ computers
3. This content is only usable in conjunction with some private WebKit API, so it belongs in PrivateHeaders rather than the public Headers location
Comment 10 mitz 2018-08-15 15:26:48 PDT
s/surf/stuff/ :|
Comment 11 Ben Richards 2018-08-15 17:15:48 PDT
Created attachment 347228 [details]
Patch
Comment 12 mitz 2018-08-15 19:20:18 PDT
Comment on attachment 347228 [details]
Patch

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

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10515
> +			shellScript = "DST_DIR=\"${BUILT_PRODUCTS_DIR}/WebKit.framework/PrivateHeaders/WebContentResources\"\nmkdir -p \"$DST_DIR\"\n\necho \"${DST_DIR}\" > ~/Desktop/dir.txt\n\nfor ((i = 0; i < ${SCRIPT_INPUT_FILE_COUNT}; ++i)); do\n    eval WEBCONTENT_RESOURCE=\\${SCRIPT_INPUT_FILE_${i}}\n    ditto \"${WEBCONTENT_RESOURCE}\" \"${DST_DIR}/${WEBCONTENT_RESOURCE##*/}\"\ndone\n\n";

Probably not supposed to log to a file on the Desktop :)

But this still copies, e.g., the macOS resources regardless of the build target, and still hardcodes the source path (instead of using INFOPLIST_PATH), and doesn’t make any substitutions in the Info.plist. In fact, as currently written, I think what this script does is equivalent to what a Copy Files build phase could have done. But I think we don’t want that: we want the names of the destination files to be consistent, and we want to replace at least some of the variables in the plist.
Comment 13 Ben Richards 2018-08-16 12:23:51 PDT
Created attachment 347285 [details]
Patch
Comment 14 Ben Richards 2018-08-16 12:25:13 PDT
This one should be a bit better!
Comment 15 mitz 2018-08-16 12:48:26 PDT
Comment on attachment 347285 [details]
Patch

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

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10147
> +				413E67622125E6E800C0F09E /* Copy Resources to Framework Private Headers */,

Feels like some qualifier should come before “resources”.

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10506
> +				"$(TEMP_FILE_DIR)/$(FULL_PRODUCT_NAME).xcent",

Can also list $(BUILT_PRODUCTS_DIR)/$(INFOPLIST_PATH) here.
Comment 16 Ben Richards 2018-08-16 13:33:08 PDT
Created attachment 347297 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2018-08-16 14:59:12 PDT
Comment on attachment 347297 [details]
Patch for landing

Clearing flags on attachment: 347297

Committed r234958: <https://trac.webkit.org/changeset/234958>
Comment 18 WebKit Commit Bot 2018-08-16 14:59:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2018-08-16 15:00:49 PDT
<rdar://problem/43397768>
Comment 20 WebKit Commit Bot 2018-08-16 16:52:25 PDT
Re-opened since this is blocked by bug 188683
Comment 21 Ben Richards 2018-08-17 11:25:26 PDT
Created attachment 347371 [details]
Patch
Comment 22 Ben Richards 2018-08-17 16:02:06 PDT
Created attachment 347405 [details]
Patch
Comment 23 Ben Richards 2018-08-17 17:35:28 PDT
Created attachment 347418 [details]
Patch
Comment 24 WebKit Commit Bot 2018-08-17 18:59:34 PDT
Comment on attachment 347418 [details]
Patch

Rejecting attachment 347418 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 347418, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=347418&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=188601&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 347418 from bug 188601.
Fetching: https://bugs.webkit.org/attachment.cgi?id=347418
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 7 diffs from patch file(s).
patching file Source/WebKit/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/Scripts/copy-webcontent-resources-to-private-headers.sh
patching file Source/WebKit/Shared/API/c/WKXPCServiceMain.cpp
patching file Source/WebKit/Shared/API/c/WKXPCServiceMain.h
patching file Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm
patching file Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h
patching file Source/WebKit/WebKit.xcodeproj/project.pbxproj
Hunk #1 succeeded at 1038 with fuzz 2 (offset 82 lines).
Hunk #2 succeeded at 3576 (offset 124 lines).
Hunk #3 succeeded at 8735 (offset 274 lines).
Hunk #4 succeeded at 8954 (offset 275 lines).
Hunk #5 succeeded at 10312 (offset 275 lines).
Hunk #6 succeeded at 10447 (offset 276 lines).
Hunk #7 succeeded at 10820 (offset 295 lines).
Hunk #8 succeeded at 11000 (offset 295 lines).
Hunk #9 FAILED at 12155.
1 out of 9 hunks FAILED -- saving rejects to file Source/WebKit/WebKit.xcodeproj/project.pbxproj.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/8897700
Comment 25 Alex Christensen 2018-09-17 15:50:29 PDT
Created attachment 349958 [details]
patch
Comment 26 Alex Christensen 2018-09-17 16:01:16 PDT
(In reply to Sam Weinig from comment #2)
> Comment on attachment 347162 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347162&action=review
> 
> > Tools/Scripts/generate-webcontent-resources:1
> > +#!/usr/bin/env python
> 
> Please add a license to this file.

Added a license comment and committed to http://trac.webkit.org/r236092
Comment 27 Ryan Haddad 2018-09-17 18:02:15 PDT
Rolled out change in https://trac.webkit.org/r236097 because it broke internal builds.
Comment 28 Alex Christensen 2018-09-18 11:53:50 PDT
The internal build "failure" was a verifier not liking ${PRODUCT_BUNDLE_IDENTIFIER} in the Info.plist.  I'll copy the processed one instead.
Comment 29 mitz 2018-09-18 12:02:25 PDT
(In reply to Alex Christensen from comment #28)
> The internal build "failure" was a verifier not liking
> ${PRODUCT_BUNDLE_IDENTIFIER} in the Info.plist.  I'll copy the processed one
> instead.

That might make it harder for clients to use their own value for that key. Could you just name the file something different than Info.plist?
Comment 30 Alex Christensen 2018-09-18 12:33:53 PDT
Created attachment 350038 [details]
patch
Comment 31 Alex Christensen 2018-09-18 13:52:23 PDT
Created attachment 350047 [details]
patch
Comment 32 Alex Christensen 2018-09-18 13:54:46 PDT
http://trac.webkit.org/r236152