Bug 195523 - [Cocoa] Code signing fails because services are copied into XPCServices after the framework is signed
Summary: [Cocoa] Code signing fails because services are copied into XPCServices after...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks: 196317
  Show dependency treegraph
 
Reported: 2019-03-09 13:45 PST by Darin Adler
Modified: 2019-06-16 09:07 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.47 KB, patch)
2019-03-09 13:46 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (377.10 KB, application/zip)
2019-03-09 14:22 PST, EWS Watchlist
no flags Details
Patch (6.31 KB, patch)
2019-03-09 14:55 PST, Darin Adler
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2019-03-09 13:45:50 PST
[Cocoa] Code signing frameworks because services are copied into XPCServices after the framework is signed
Comment 1 Darin Adler 2019-03-09 13:46:24 PST Comment hidden (obsolete)
Comment 2 Darin Adler 2019-03-09 13:47:26 PST
I’m trying what Dan suggested when I asked people at Apple about this problem. This is supposed to solve the problem where we get code signing errors in some builds.
Comment 3 Darin Adler 2019-03-09 13:49:52 PST
Comment on attachment 364140 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        [Cocoa] Code signing frameworks because services are copied into XPCServices after the framework is signed

I meant to say "code signing fails", not "code signing frameworks".
Comment 4 mitz 2019-03-09 13:54:43 PST
Comment on attachment 364140 [details]
Patch

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

Looks good overall. Should the new script build phase’s output paths be populated with the paths of the symlinks? Maybe there’s no harm in having the script run even if they exist.

Another concern is whether the script needs to check if there exists a directory where it wants to create the symlink, and delete it, so that the transition to this new way of building doesn’t require people to manually delete some of the built product.

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10675
> +			shellScript = "if [[ \"${CONFIGURATION}\" == \"Production\" ]]; then\n    exit\nfi\n\nif [[ ${WK_PLATFORM_NAME} != \"macosx\" ]]; then\n    XPC_SERVICES_PATH=\"${BUILT_PRODUCTS_DIR}/WebKit.framework/XPCServices\"\n    BUILT_PRODUCTS_DIR_RELATIVE_PATH_FROM_XPC_SERVICES=\"../..\"\nelse\n    XPC_SERVICES_PATH=\"${BUILT_PRODUCTS_DIR}/WebKit.framework/Versions/A/XPCServices\"\n    BUILT_PRODUCTS_DIR_RELATIVE_PATH_FROM_XPC_SERVICES=\"../../../..\"\nfi\n\nmkdir -p \"${XPC_SERVICES_PATH}\"\nln -sfh \"${BUILT_PRODUCTS_DIR_RELATIVE_PATH_FROM_XPC_SERVICES}/com.apple.WebKit.WebContent.xpc\" \"${XPC_SERVICES_PATH}/com.apple.WebKit.WebContent.xpc\"\nln -sfh \"${BUILT_PRODUCTS_DIR_RELATIVE_PATH_FROM_XPC_SERVICES}/com.apple.WebKit.Networking.xpc\" \"${XPC_SERVICES_PATH}/com.apple.WebKit.Networking.xpc\"\nln -sfh \"${BUILT_PRODUCTS_DIR_RELATIVE_PATH_FROM_XPC_SERVICES}/${WK_STORAGE_SERVICE_PRODUCT_NAME}.xpc\" \"${XPC_SERVICES_PATH}/${WK_STORAGE_SERVICE_PRODUCT_NAME}.xpc\"\n\nif [[ ${WK_PLATFORM_NAME} == macosx ]]; then\n    ln -sfh \"${BUILT_PRODUCTS_DIR_RELATIVE_PATH_FROM_XPC_SERVICES}/com.apple.WebKit.Plugin.32.xpc\" \"${XPC_SERVICES_PATH}/com.apple.WebKit.Plugin.xpc\"\n    ln -sfh \"${BUILT_PRODUCTS_DIR_RELATIVE_PATH_FROM_XPC_SERVICES}/com.apple.WebKit.Plugin.64.xpc\" \"${XPC_SERVICES_PATH}/com.apple.WebKit.Plugin.64.xpc\"\nfi\n";

We can remove references to the 32-bit plug-in service, which no longer exists.
Comment 5 EWS Watchlist 2019-03-09 14:22:49 PST Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-03-09 14:22:51 PST Comment hidden (obsolete)
Comment 7 Darin Adler 2019-03-09 14:32:03 PST
(In reply to mitz from comment #4)
> Should the new script build phase’s output paths be
> populated with the paths of the symlinks? Maybe there’s no harm in having
> the script run even if they exist.

I like the idea of having the script not run if it doesn’t need to. I don’t know how to make sure the build system does run the script whenever it does need to. I think the times it needs to run are:

1) When the symlinks, and perhaps the entire XPCServices directory, haven’t yet been created.

2) When the directory is there and populated with frameworks rather than symlinks to frameworks. I think that I can switch from the "-f" option in the ln command to the "-F" option so that it will delete even entire framework directories when writing the symlinks. I have two ideas about how to have the build system run the script in such cases: a) Do not list the output files so that the script runs every time, b) list one additional output file that the old script never created, a "dummy" one if necessary.

3) Hypothetically in the future, when symlinks are there, but have incorrect paths or are otherwise incorrect because they were created by an earlier version of the script. I believe the script will overwrite such links because I used the "-f" option in the ln command. Again, I need to get the build system to run the script. One idea is to keep changing the name of the dummy file each time the script needs to be re-run.

> Another concern is whether the script needs to check if there exists a
> directory where it wants to create the symlink, and delete it, so that the
> transition to this new way of building doesn’t require people to manually
> delete some of the built product.

I think passing the "-F" option to the ln command should take care of it, but I’m not sure how to test.

> We can remove references to the 32-bit plug-in service, which no longer
> exists.

Thanks. I will do that.
Comment 8 Darin Adler 2019-03-09 14:32:51 PST
Looks like all the tests crashed on the bots. I don’t know why.
Comment 9 Darin Adler 2019-03-09 14:45:09 PST
Here are the things that worry me about putting names in outputPaths:

1) I don’t understand why it’s OK to use the same output paths on macOS as on iOS. Perhaps the symlinks already created as part of the bundle structure make the paths work fine on both, even though they are not the same as the paths we use in the script on macOS. But if so then I don’t understand why we need the XPC_SERVICES_PATH environment variable inside the build phase script itself.

2) I don’t understand why it’s OK to list macOS-only paths that will never be created on iOS. It seems that would lead to the script being executed every time on iOS.

3) I wonder if I will actually need to add a dummy as described above to help people successfully compile with their existing trees, since all the frameworks will be there, they just will be old versions. It’s quite important that those get replaced with symlinks!

Because of those three, I didn’t add outputPaths yet in this upcoming patch.
Comment 10 Darin Adler 2019-03-09 14:55:02 PST
Created attachment 364143 [details]
Patch
Comment 11 Darin Adler 2019-03-10 10:13:57 PDT
Committed r242686: <https://trac.webkit.org/changeset/242686>
Comment 12 Radar WebKit Bug Importer 2019-03-10 10:40:26 PDT
<rdar://problem/48750841>
Comment 13 Alexey Proskuryakov 2019-04-02 17:50:34 PDT
This caused rdar://problem/49542244.