| Summary: | Copy WebKit frameworks and XPC processes to Secondary Path | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||
| Component: | Platform | Assignee: | Michael Saboff <msaboff> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | ap, dino, emw, eric.carlson, ews-watchlist, hi, kbr, keith_miller, kondapallykalyan, mark.lam, pangle, rhoule, saam, tzagallo, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Michael Saboff
2022-03-02 15:11:36 PST
Created attachment 453681 [details]
Patch
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE 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 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 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. As the original implementor of mach_o.py, this patch looks good to me. (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. (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. (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? (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 on attachment 453681 [details]
Patch
r=me
(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. Committed r290805 (248044@trunk): <https://commits.webkit.org/248044@trunk> |