-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[CMake] Integrate vcpkg into Windows build #36366
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.
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
[CMake] Integrate vcpkg into Windows build #36366
Conversation
|
EWS run on previous version of this PR (hash 596b342) Details |
|
Needs to be rebased after #36365 lands |
|
To run create a directory to do the build in and invoke cmake directly replacing the path to your webkit checkout |
596b342 to
efba267
Compare
|
EWS run on previous version of this PR (hash efba267) Details |
efba267 to
88b6f96
Compare
|
EWS run on previous version of this PR (hash 88b6f96) Details |
|
Ok so this works locally but will require some changes to the bots to support it. |
88b6f96 to
085b077
Compare
|
EWS run on previous version of this PR (hash 085b077) Details |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this approach.
- Buildbot workers have to compile all required libraries. But, we have no enough VM resources.
- It increases the built artifact size if all compiled DLL are copies to
bindirectory. (layout tests bots need built DLLs)
I think current WebKitRequirements64.zip approach is better.
|
I tried |
|
085b077 to
89c7498
Compare
|
EWS run on previous version of this PR (hash 89c7498) Details |
This is resolved now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you use GitHub packages binary cache?
https://learn.microsoft.com/en-us/vcpkg/reference/binarycaching
8d7ae1b to
616c811
Compare
|
EWS run on previous version of this PR (hash 616c811) Details |
|
Added the vcpkg binary caching as requested to WebKitForWindows/docker-webkit-dev#408 I ran it locally with the updated requirements and 👇 was pushed. The latest change updated ICU to 77.1. |
616c811 to
5e5416a
Compare
|
EWS run on previous version of this PR (hash 5e5416a) Details |
|
Using GitHub Packages as a NuGet feed for The cache was populated in https://ews-build.webkit.org/#/builders/59/builds/49007 . As an example here's icu 👇 The hash from ☝️ is represented in the package listing 👇 . The next build then restored from cache on a different bot because the build environment hash was the same. (this is why the counter is 1 for ☝️). See https://ews-build.webkit.org/#/builders/59/builds/49018 for the complete build. So a restore only took 13s. This seems more efficient than the Overall this is ready to go. The request for binary caching has been done for all bots using Docker. A bot provided by Playwright folks is not using Docker, ews-004, and does not have vcpkg installed. I believe this is due to Visual Studio 2022 build tools which does not package vcpkg. Using the Docker images there or adding in vcpkg like the Docker images is the only thing that needs to be done to land this patch. |
|
We as Playwright have tried this patch and it works well for us (builds successfully and works). It simplifies rolling of dependencies, seems to solve pinning and allows vcpkg caching strategies to get used. Thanks Don for the work and we're excited to get this landed and remove our local vcpkg hacks. |
|
debug builds still have a problem https://bugs.webkit.org/show_bug.cgi?id=287815 |
|
Given that the error in the linked bug literally says (It's surprising for this to be raised as an issue prior to landing, given that Windows tests are not run via EWS.) |
|
I know it's not a problem for you. You don't maintain Windows WebKit. You don't fix problems for Windows WebKit. |
|
Given the needs of the community shipping the Windows port I am confident this solves all the issues with WebKitRequirements.zip. Their concerns with the status quo are valid and need to be prioritized. This patch has already identified issues in the codebase that weren’t caught because the release crt was used. Using fully debug libraries might identify more issues. Hypotheticals aren’t a reason to block this patch and not landing this patch is making the windows port less secure. |
5e5416a to
9853c43
Compare
|
EWS run on previous version of this PR (hash 9853c43) Details |
9853c43 to
419c15e
Compare
|
EWS run on previous version of this PR (hash 419c15e) Details |
419c15e to
9237033
Compare
|
EWS run on current version of this PR (hash 9237033) Details |
https://bugs.webkit.org/show_bug.cgi?id=282800 Reviewed by Ross Kirsling. Use `vcpkg` to handle the third party requirements of the Windows WebKit build. The requirements will be built locally within the build and no longer distributed from WebKitForWindows/WebKitRequirements. While all port definitions are in that repository a migration to the canonical vcpkg registry at Microsoft/vcpkg will be prioritized. Removes the code that would download prebuilt binaries to `WebKitLibraries/win`. This change will be used to support additional libraries in the future. * .gitignore: * Source/cmake/OptionsJSCOnly.cmake: * Source/cmake/OptionsWin.cmake: * Tools/Scripts/build-webkit: * Tools/Scripts/download-github-release.py: Removed. * Tools/Scripts/update-webkit-win-libs.py: * Tools/Scripts/webkitdirs.pm: (shouldUseVcpkg): (generateBuildSystemFromCMakeProject): (vcpkgArgsFromFeatures): (windowsLibrariesDir): Deleted. * Tools/Scripts/webkitperl/BuildSubproject.pm: * WebKitLibraries/triplets/x64-windows-webkit.cmake: Added. * vcpkg-configuration.json: Added. * vcpkg.json: Added. Canonical link: https://commits.webkit.org/295042@main
9237033 to
32342c7
Compare
|
Committed 295042@main (32342c7): https://commits.webkit.org/295042@main Reviewed commits have been landed. Closing PR #36366 and removing active labels. |
🛠 wpe-cairo
🛠 🧪 jsc

32342c7
9237033