[CMake] Integrate vcpkg into Windows build#36366
Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom May 16, 2025
Merged
[CMake] Integrate vcpkg into Windows build#36366webkit-commit-queue merged 1 commit intoWebKit:mainfrom
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
Conversation
Collaborator
|
EWS run on previous version of this PR (hash 596b342) Details |
Contributor
Author
|
Needs to be rebased after #36365 lands |
Contributor
Author
|
To run create a directory to do the build in and invoke cmake directly replacing the path to your webkit checkout |
webkit-ews-buildbot
added
the
merging-blocked
donny-dont
removed
the
merging-blocked
donny-dont
force-pushed
the
eng/CMake-Integrate-vcpkg-into-Windows-build
branch
from
596b342 to
efba267
Compare
Collaborator
|
EWS run on previous version of this PR (hash efba267) Details |
🛠 wpe-cairo
🛠 🧪 jsc
donny-dont
force-pushed
the
eng/CMake-Integrate-vcpkg-into-Windows-build
branch
from
efba267 to
88b6f96
Compare
Collaborator
|
EWS run on previous version of this PR (hash 88b6f96) Details |
Contributor
Author
|
Ok so this works locally but will require some changes to the bots to support it. |
webkit-ews-buildbot
added
the
merging-blocked
donny-dont
removed
the
merging-blocked
donny-dont
force-pushed
the
eng/CMake-Integrate-vcpkg-into-Windows-build
branch
from
88b6f96 to
085b077
Compare
Collaborator
|
EWS run on previous version of this PR (hash 085b077) Details |
webkit-ews-buildbot
added
the
merging-blocked
fujii
requested changes
Nov 11, 2024
Contributor
There was a problem hiding this comment.
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.
Contributor
|
I tried |
Contributor
|
donny-dont
removed
the
merging-blocked
donny-dont
force-pushed
the
eng/CMake-Integrate-vcpkg-into-Windows-build
branch
from
085b077 to
89c7498
Compare
Collaborator
|
EWS run on previous version of this PR (hash 89c7498) Details |
Contributor
Author
This is resolved now |
webkit-ews-buildbot
added
the
merging-blocked
fujii
reviewed
Nov 14, 2024
fujii
requested changes
Nov 14, 2024
Contributor
There was a problem hiding this comment.
Why don't you use GitHub packages binary cache?
https://learn.microsoft.com/en-us/vcpkg/reference/binarycaching
donny-dont
removed
the
merging-blocked
donny-dont
removed
the
merging-blocked
donny-dont
force-pushed
the
eng/CMake-Integrate-vcpkg-into-Windows-build
branch
from
7da4d15 to
5639869
Compare
Collaborator
|
EWS run on previous version of this PR (hash 5639869) Details |
Contributor
|
Any update on this @fujii ? I don't think the USE_SKIA build works on main at the moment because the latest WebKitRequirements doesn't include harfbuzz. I think this patch would fix that and make it easier to update the dependencies over time. |
donny-dont
force-pushed
the
eng/CMake-Integrate-vcpkg-into-Windows-build
branch
from
5639869 to
8d7ae1b
Compare
Collaborator
|
EWS run on previous version of this PR (hash 8d7ae1b) Details |
Contributor
Author
I updated the Docker images in docker-webkit-dev and did a build of this patch locally. The first one put artifacts at WebKitForWindows (note that it may not be visible to those without permissions). The second one restored them. See the output 👇 The line from ☝️ to note is this one By using Docker images the build environment between bots will be the same so they will hash to the same version. After the first build is done then all bots will get those packages from the configured NuGet feed. See 👇 for the packages which shows the download count.
|
Contributor
Author
|
Updated config to include ICU 77.1 |
donny-dont
force-pushed
the
eng/CMake-Integrate-vcpkg-into-Windows-build
branch
from
8d7ae1b to
616c811
Compare
Collaborator
|
EWS run on previous version of this PR (hash 616c811) Details |
Contributor
Author
|
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.
|
webkit-ews-buildbot
added
the
merging-blocked
donny-dont
removed
the
merging-blocked
donny-dont
force-pushed
the
eng/CMake-Integrate-vcpkg-into-Windows-build
branch
from
616c811 to
5e5416a
Compare
Collaborator
|
EWS run on previous version of this PR (hash 5e5416a) Details |
webkit-ews-buildbot
added
the
merging-blocked
Contributor
Author
|
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. |
Contributor
|
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. |
Contributor
|
debug builds still have a problem https://bugs.webkit.org/show_bug.cgi?id=287815 |
Member
|
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.) |
Contributor
|
I know it's not a problem for you. You don't maintain Windows WebKit. You don't fix problems for Windows WebKit. |
Contributor
Author
|
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. |
Collaborator
|
EWS run on previous version of this PR (hash 9853c43) Details |
rkirsling
approved these changes
May 9, 2025
Collaborator
|
EWS run on previous version of this PR (hash 419c15e) Details |
Collaborator
|
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
Collaborator
|
Committed 295042@main (32342c7): https://commits.webkit.org/295042@main Reviewed commits have been landed. Closing PR #36366 and removing active labels. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
32342c7
9237033