Bug 237394 - Copy WebKit frameworks and XPC processes to Secondary Path
Summary: Copy WebKit frameworks and XPC processes to Secondary Path
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-02 15:11 PST by Michael Saboff
Modified: 2022-03-03 17:27 PST (History)
15 users (show)

See Also:


Attachments
Patch (83.57 KB, patch)
2022-03-02 17:21 PST, Michael Saboff
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2022-03-02 15:11:36 PST
Copy the various WebKit Frameworks, XPC services and other dylibs to the SYSTEM_SECONDARY_CONTENT_PATH when it is set.

For the copied executables, we also need to update the binaries to use the frameworks copied to SYSTEM_SECONDARY_CONTENT_PATH.
Comment 1 Michael Saboff 2022-03-02 15:12:01 PST
<rdar://89053248>
<rdar://86558002>
Comment 2 Michael Saboff 2022-03-02 17:21:03 PST
Created attachment 453681 [details]
Patch
Comment 3 EWS Watchlist 2022-03-02 17:23:32 PST
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 4 Elliott Williams 2022-03-02 17:50:15 PST
Comment on attachment 453681 [details]
Patch

Some high-level feedback:

At first glance, it looks like you could do this with less code by adding Copy Files phases to the Xcode targets, which would copy their target's product to $(SECONDARY_STAGED_FRAMEWORK_DIRECTORY). These can be configured to be install-only and Mac-only. It might be worth trying this out before we commit to adding a new rsync script.

If you do stick with a script, please put it in WTF's Scripts/ directory. Those scripts get copied during WTF's installhdrs and are available for other projects to use, to save us from checking in duplicate copies :)
Comment 5 Saam Barati 2022-03-02 18:41:24 PST
Comment on attachment 453681 [details]
Patch

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

> Source/WebGPU/Scripts/copy-frameworks-to-secondary-path.sh:1
> +#!/bin/sh

Do we really need all the different versions of this script? There must be a better way.
Comment 6 Alexey Proskuryakov 2022-03-02 19:20:05 PST
Comment on attachment 453681 [details]
Patch

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

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:11931
> +			inputFileListPaths = (

I think that we are going to need inputs for script phases for the modern build system.
Comment 7 Richard Houle 2022-03-02 20:30:07 PST
As the original implementor of mach_o.py, this patch looks good to me.
Comment 8 Michael Saboff 2022-03-02 21:11:11 PST
(In reply to Saam Barati from comment #5)
> Comment on attachment 453681 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453681&action=review
> 
> > Source/WebGPU/Scripts/copy-frameworks-to-secondary-path.sh:1
> > +#!/bin/sh
> 
> Do we really need all the different versions of this script? There must be a
> better way.

Our internal build system builds one project at a time.  Therefore only the files in that projects tree are available.
Comment 9 Michael Saboff 2022-03-02 21:30:19 PST
(In reply to Elliott Williams from comment #4)
> Comment on attachment 453681 [details]
> Patch
> 
> Some high-level feedback:
> 
> At first glance, it looks like you could do this with less code by adding
> Copy Files phases to the Xcode targets, which would copy their target's
> product to $(SECONDARY_STAGED_FRAMEWORK_DIRECTORY). These can be configured
> to be install-only and Mac-only. It might be worth trying this out before we
> commit to adding a new rsync script.

This was patterned after similar scripts in the Safari source tree that do the same types of copying and modifying.

That may work for the copy-frameworks-to-secondary-path.sh script, but for copy-xpc-services-to-secondary-path.sh  we'd still need a script to update DYLD_VERSIONED_FRAMEWORK_PATH for the XPC services.

Also, we read the symlink found at ${BUILT_PRODUCTS_DIR}/${PRODUCT_COMPONENT} to get the real file or directory to copy over.

> If you do stick with a script, please put it in WTF's Scripts/ directory.
> Those scripts get copied during WTF's installhdrs and are available for
> other projects to use, to save us from checking in duplicate copies :)

We don't copy the WTF scripts to the public SDK.  This would probably break Xcode builds for open source contributors.
Comment 10 Elliott Williams 2022-03-03 15:50:33 PST
(In reply to Michael Saboff from comment #9)
> (In reply to Elliott Williams from comment #4)
> > Comment on attachment 453681 [details]
> > Patch
> > 
> > Some high-level feedback:
> > 
> > At first glance, it looks like you could do this with less code by adding
> > Copy Files phases to the Xcode targets, which would copy their target's
> > product to $(SECONDARY_STAGED_FRAMEWORK_DIRECTORY). These can be configured
> > to be install-only and Mac-only. It might be worth trying this out before we
> > commit to adding a new rsync script.
> 
> This was patterned after similar scripts in the Safari source tree that do
> the same types of copying and modifying.
> 
> That may work for the copy-frameworks-to-secondary-path.sh script, but for
> copy-xpc-services-to-secondary-path.sh  we'd still need a script to update
> DYLD_VERSIONED_FRAMEWORK_PATH for the XPC services.
>
> Also, we read the symlink found at
> ${BUILT_PRODUCTS_DIR}/${PRODUCT_COMPONENT} to get the real file or directory
> to copy over.

Thanks for the explanation. I still think it would be worth trying at some point (in both trees), but not so high-value that we should block now. FWIW, I believe Xcode's copy phases resolve symlinks, so that particular need would be met.


> > If you do stick with a script, please put it in WTF's Scripts/ directory.
> > Those scripts get copied during WTF's installhdrs and are available for
> > other projects to use, to save us from checking in duplicate copies :)
> 
> We don't copy the WTF scripts to the public SDK.  This would probably break
> Xcode builds for open source contributors.

WTF scripts don't make it to the public SDK, but they are always available in engineering builds at $(BUILT_PRODUCTS_DIR)/usr/local/include/wtf/Scripts. This is how, for instance, generate-unified-source-bundles.rb works. When WTF builds, Source/WTF/Scripts/generate-unified-source-bundles.rb is copied to /usr/local/include/wtf/Scripts. Build phases in WebCore and JSC execute it from that path to create their respective unified sources.

AFAICT, this script would be used in the same way, so it should be able to live in Source/WTF/Scripts/ too. Is there something I'm missing here?
Comment 11 Elliott Williams 2022-03-03 15:55:00 PST
(In reply to Alexey Proskuryakov from comment #6)
> Comment on attachment 453681 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453681&action=review
> 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:11931
> > +			inputFileListPaths = (
> 
> I think that we are going to need inputs for script phases for the modern
> build system.

IMO it is fine to omit them for now. XCBuild will run the script unconditionally if it has no inputs, and since this script is only run for `install` actions, running it unconditionally does not affect typical incremental builds.

We would need to add them if and when we start sandboxing our scripts, though.
Comment 12 Saam Barati 2022-03-03 17:15:34 PST
Comment on attachment 453681 [details]
Patch

r=me
Comment 13 Saam Barati 2022-03-03 17:16:13 PST
(In reply to Elliott Williams from comment #10)
> (In reply to Michael Saboff from comment #9)
> > (In reply to Elliott Williams from comment #4)
> > > Comment on attachment 453681 [details]
> > > Patch
> > > 
> > > Some high-level feedback:
> > > 
> > > At first glance, it looks like you could do this with less code by adding
> > > Copy Files phases to the Xcode targets, which would copy their target's
> > > product to $(SECONDARY_STAGED_FRAMEWORK_DIRECTORY). These can be configured
> > > to be install-only and Mac-only. It might be worth trying this out before we
> > > commit to adding a new rsync script.
> > 
> > This was patterned after similar scripts in the Safari source tree that do
> > the same types of copying and modifying.
> > 
> > That may work for the copy-frameworks-to-secondary-path.sh script, but for
> > copy-xpc-services-to-secondary-path.sh  we'd still need a script to update
> > DYLD_VERSIONED_FRAMEWORK_PATH for the XPC services.
> >
> > Also, we read the symlink found at
> > ${BUILT_PRODUCTS_DIR}/${PRODUCT_COMPONENT} to get the real file or directory
> > to copy over.
> 
> Thanks for the explanation. I still think it would be worth trying at some
> point (in both trees), but not so high-value that we should block now. FWIW,
> I believe Xcode's copy phases resolve symlinks, so that particular need
> would be met.
> 
> 
> > > If you do stick with a script, please put it in WTF's Scripts/ directory.
> > > Those scripts get copied during WTF's installhdrs and are available for
> > > other projects to use, to save us from checking in duplicate copies :)
> > 
> > We don't copy the WTF scripts to the public SDK.  This would probably break
> > Xcode builds for open source contributors.
> 
> WTF scripts don't make it to the public SDK, but they are always available
> in engineering builds at
> $(BUILT_PRODUCTS_DIR)/usr/local/include/wtf/Scripts. This is how, for
> instance, generate-unified-source-bundles.rb works. When WTF builds,
> Source/WTF/Scripts/generate-unified-source-bundles.rb is copied to
> /usr/local/include/wtf/Scripts. Build phases in WebCore and JSC execute it
> from that path to create their respective unified sources.
> 
> AFAICT, this script would be used in the same way, so it should be able to
> live in Source/WTF/Scripts/ too. Is there something I'm missing here?

Michael, maybe you can file a FIXME for this, and we can improve it in the future? Would be nice to remove the duplication.
Comment 14 Michael Saboff 2022-03-03 17:27:03 PST
Committed r290805 (248044@trunk): <https://commits.webkit.org/248044@trunk>