Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Xcode] Teach "Create symlinks to XPC services" build phase to also symlink libWebKitSwift.dylib #24276

Merged
merged 1 commit into from
Feb 13, 2024
Merged

[Xcode] Teach "Create symlinks to XPC services" build phase to also symlink libWebKitSwift.dylib #24276

merged 1 commit into from
Feb 13, 2024

Conversation

aestes
Copy link
Contributor

@aestes aestes commented Feb 12, 2024 •
edited by webkit-early-warning-system
Loading

58b61c2

[Xcode] Teach "Create symlinks to XPC services" build phase to also symlink libWebKitSwift.dylib
https://bugs.webkit.org/show_bug.cgi?id=269229
rdar://122825232

Reviewed by Elliott Williams.

libWebKitSwift.dylib expects to be installed in WebKit.framework/Frameworks, but nothing in the
engineering build system places it in that location. While WebKitSwiftSoftLink.mm will find the
dylib in $BUILT_PRODUCTS_DIR on macOS, tools for creating roots from engineering built products
(e.g., `package-root`) do not know to include the dylib without at least a symlink to it in the
expected location. To fix this, extended WebKit's "Create symlinks to XPC services" build phase
to create a symlink to libWebKitSwift.dylib in the expected install location.

* Source/WebKit/WebKit.xcodeproj/project.pbxproj:

Canonical link: https://commits.webkit.org/274514@main

e939c7b

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
  🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 🧪 merge   🛠 watch
  🛠 watch-sim

@aestes aestes self-assigned this Feb 12, 2024
@aestes aestes added the Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases label Feb 12, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 12, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 224e4ac)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios   🛠 mac ✅ 🛠 wpe   🛠 wincairo
✅ 🛠 ios-sim ❌ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2   🧪 api-mac ✅ 🧪 api-wpe
⏳ 🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 gtk
✅ 🧪 api-ios   🧪 mac-wk2   🧪 gtk-wk2
  🛠 tv ❌ 🧪 mac-AS-debug-wk2   🧪 api-gtk
✅ 🛠 tv-sim   🧪 mac-wk2-stress
  🛠 watch
  🛠 watch-sim

emw-apple
emw-apple reviewed Feb 12, 2024
View reviewed changes
Copy link
Contributor

@emw-apple emw-apple left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libWebKitSwift.dylib expects to be installed in WebKit.framework/Frameworks

Is it always a top-level Frameworks/ directory in the bundle, or does it follow the normal macOS convention of being in a Versions/A/ directory? If so, you probably want to take my suggestion below:

Source/WebKit/WebKit.xcodeproj/project.pbxproj Show resolved Hide resolved
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 12, 2024
@aestes
Copy link
Contributor Author

aestes commented Feb 12, 2024

libWebKitSwift.dylib expects to be installed in WebKit.framework/Frameworks

Is it always a top-level Frameworks/ directory in the bundle, or does it follow the normal macOS convention of being in a Versions/A/ directory? If so, you probably want to take my suggestion below:

WebKitSwiftSoftLink.mm looks for the dylib directly in WebKit.framework/Frameworks; it's relying on the symlink created by the "Make Frameworks Symbolic Link" build phase.

@emw-apple
Copy link
Contributor

WebKitSwiftSoftLink.mm looks for the dylib directly in WebKit.framework/Frameworks; it's relying on the symlink created by the "Make Frameworks Symbolic Link" build phase.

Got it… I think we still need to use the non-symlinked path in the pbxproj. Without it, the build system could in theory schedule this task to run before it creates the symlink. And once we turn on build script sandboxing, we'd need the build system to know that the script writes to the versioned directory.

emw-apple
emw-apple approved these changes Feb 12, 2024
View reviewed changes
Copy link
Contributor

@emw-apple emw-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+ with fix mentioned above

@aestes aestes removed the merging-blocked Applied to prevent a change from being merged label Feb 12, 2024
@aestes aestes force-pushed the eng/Xcode-Teach-Create-symlinks-to-XPC-services-build-phase-to-also-symlink-libWebKitSwift-dylib branch from 224e4ac to e939c7b Compare February 12, 2024 22:22
@aestes
Copy link
Contributor Author

aestes commented Feb 12, 2024

WebKitSwiftSoftLink.mm looks for the dylib directly in WebKit.framework/Frameworks; it's relying on the symlink created by the "Make Frameworks Symbolic Link" build phase.

Got it… I think we still need to use the non-symlinked path in the pbxproj. Without it, the build system could in theory schedule this task to run before it creates the symlink. And once we turn on build script sandboxing, we'd need the build system to know that the script writes to the versioned directory.

Sounds good. I went with your suggestion of $(WK_FRAMEWORK_VERSION_PREFIX). Thanks for the review!

@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 12, 2024 •
edited
Loading

EWS run on current version of this PR (hash e939c7b)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
  🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 🧪 merge   🛠 watch
  🛠 watch-sim

@aestes aestes added the merge-queue Applied to send a pull request to merge-queue label Feb 13, 2024
@aestes
[Xcode] Teach "Create symlinks to XPC services" build phase to also s…
58b61c2 
…ymlink libWebKitSwift.dylib

https://bugs.webkit.org/show_bug.cgi?id=269229
rdar://122825232

Reviewed by Elliott Williams.

libWebKitSwift.dylib expects to be installed in WebKit.framework/Frameworks, but nothing in the
engineering build system places it in that location. While WebKitSwiftSoftLink.mm will find the
dylib in $BUILT_PRODUCTS_DIR on macOS, tools for creating roots from engineering built products
(e.g., `package-root`) do not know to include the dylib without at least a symlink to it in the
expected location. To fix this, extended WebKit's "Create symlinks to XPC services" build phase
to create a symlink to libWebKitSwift.dylib in the expected install location.

* Source/WebKit/WebKit.xcodeproj/project.pbxproj:

Canonical link: https://commits.webkit.org/274514@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Xcode-Teach-Create-symlinks-to-XPC-services-build-phase-to-also-symlink-libWebKitSwift-dylib branch from e939c7b to 58b61c2 Compare February 13, 2024 02:32
@webkit-commit-queue
Copy link
Collaborator

Committed 274514@main (58b61c2): https://commits.webkit.org/274514@main

Reviewed commits have been landed. Closing PR #24276 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 58b61c2 into WebKit:main Feb 13, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emw-apple emw-apple emw-apple approved these changes

Assignees

@aestes aestes

Labels
Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases
Projects
None yet
Milestone
No milestone
5 participants
@aestes @webkit-early-warning-system @emw-apple @webkit-commit-queue @webkit-ews-buildbot