-
Notifications
You must be signed in to change notification settings - Fork 36
Update WindowsPort.md as per #129 and other advisories #130
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
base: main
Are you sure you want to change the base?
Conversation
- Make users aware that shallow cloning of the WebKit repo will cause failure - Make users aware that "C++ Clang Tools for Windows" will cause build failure and offer workaround in the Webkit Command Prompt script. - Add option to set ccmake for pch in the Webkit Command Prompt
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.
Overall looks good just some follow ups for you and we can squash and land
docs/Ports/WindowsPort.md
Outdated
|
||
Install CMake, Perl, Python, Ruby, gperf \([GnuWin32 Gperf](https://gnuwin32.sourceforge.net/packages/gperf.htm)\), LLVM, and Ninja. | ||
Python 3.12 has [a problem for WebKit at the moment](https://webkit.org/b/261113). Use Python 3.11. | ||
- Python 3.12 has [a problem for WebKit at the moment](https://webkit.org/b/261113). Use Python 3.11. |
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’m not 100% sure this is still an issue and I’m not sure if Python 3.13 is good to use. Might be worth verifying this.
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.
Python 3.13 (latest choco) compiles WebKit for me with the following caveates:
- I did not flush
ccache
or__pycache__
- I am not building or running any tests
- I am only building Release
While choco install python311
works, it is no longer on the Chocolately package list. I don't really understand the underlying issue at a low level to make a call on this, but maybe the documentation should advise to do choco install python --version=3.11.x
only if problems are experienced?
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’m not sure if that works because you can have multiple python versions installed
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.
@iangrunert any thoughts on this?
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'm using Python 3.12 and haven't ran into issues as of yet.
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.
From CMake configure: -- Found Python: C:/Python313/python.exe (found version "3.13.6") found components: Interpreter
I think this is hard-coded somewhere obfuscated to PATH. I tried setting the PYTHON
env variable to something else from the WebKit shell script, and it got overwritten. Chocolately should however fix PATH to whatever version is installed last, even though multiple version remain on disk.
Let me know if we are good with 3.13.x. If so, we can just document python
for choco install.
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.
@iangrunert @donny-dont
Have you reached any conclusion on how to document Python?
docs/Ports/WindowsPort.md
Outdated
|
||
You can use [Chocolatey](https://community.chocolatey.org/) to install the tools. | ||
[ActivePerl chocolatey package](https://community.chocolatey.org/packages/ActivePerl) has a problem and no package maintainer now. | ||
XAMPP includes Perl, and running layout tests needs XAMPP. Install XAMPP instead. | ||
|
||
``` | ||
choco install -y xampp-81 python311 ruby git cmake gperf llvm ninja | ||
choco install -y xampp-81 python311 ruby git gperf llvm ninja | ||
choco install -y cmake --version=3.31.8 |
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 think we need a choco pin command with this so when someone upgrades they don’t end up with CMake 4. Ideally the version could be <4.0.0 so if there is another 3.x release it’ll upgrade.
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.
See revised
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.
@iangrunert any other suggestions for this?
docs/Ports/WindowsPort.md
Outdated
# Use the latest 3.x version of cmake available | ||
choco search -e cmake -a | ||
choco install -y cmake --version=3.x.x | ||
choco pin add -n=cmake |
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’m not chocolatey expert would this work if a new 3.x.x is released and update is called?
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 think that pin means pin, but we can be more helpful with the message (see update)
Add patch fix for CMake complications with woff2
docs/Ports/WindowsPort.md
Outdated
|
||
rem Vcpkg cannot install woff2 with CMake 4+ | ||
rem Apply the patch below to force vcpkg to the VS toolchain bundled version of CMake 3 | ||
rem The WebKit build will still use the CMake on your PATH | ||
rem ( | ||
rem echo diff --git a/WebKitLibraries/triplets/x64-windows-webkit.cmake b/WebKitLibraries/triplets/x64-windows-webkit.cmake | ||
rem echo index 502577b9fe..b06fdcdbda 100644 | ||
rem echo --- a/WebKitLibraries/triplets/x64-windows-webkit.cmake | ||
rem echo +++ b/WebKitLibraries/triplets/x64-windows-webkit.cmake | ||
rem echo @@ -1,3 +1,5 @@ | ||
rem echo +set^(VCPKG_ENV_PASSTHROUGH DevEnvDir^) | ||
rem echo +set^(CMAKE_COMMAND "$ENV{DevEnvDir}CommonExtensions/Microsoft/CMake/CMake/bin/cmake.exe"^) | ||
rem echo set^(VCPKG_TARGET_ARCHITECTURE x64^) | ||
rem echo set^(VCPKG_CRT_LINKAGE dynamic^) | ||
rem echo set^(VCPKG_LIBRARY_LINKAGE dynamic^) | ||
rem ) > triplet.patch | ||
rem git apply --whitespace=nowarn triplet.patch 2>nul | ||
rem del triplet.patch | ||
|
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.
This patches the triplet file to force vcpkg to use the VS toolchain version of CMake 3.
It is potentially fragile, but I think it beats all the messing with the devs build environment outside of the project scope.
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.
Apologies for the flip flop...
This mixed mode CMake build failed on me overnight. See the comment below for a more robust approach.
Mixed CMake with patch build fails. Force PATH to bundled CMake 3.x instead.
rem Set PATH to use the VS toolchain bundled CMake 3.x. This ensures that downstream vcpkg builds will succeed. | ||
path %DevEnvDir%CommonExtensions\\Microsoft\\CMake\\CMake\\bin;%path% | ||
|
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.
Instead of forcing CMake 3.x in the developers global ENV, use the VS 2022 bundled CMake 3.x for the project build.
@@ -1,35 +1,42 @@ | |||
# Windows port | |||
|
|||
It is using [cairo](https://www.cairographics.org/) for the graphics backend, [libcurl](https://curl.se/libcurl/) for the network backend. | |||
It is using [skia](https://skia.org/) for the graphics backend, [libcurl](https://curl.se/libcurl/) for the network backend. |
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.
Am I right that skia is now the default?
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.
Yep, Skia is the default.
Housekeeping, formatting, semantics and update to consider VCPKG build in recent WebKit versions.
docs/Ports/WindowsPort.md
Outdated
Recent WebKit versions will use [VCPKG](https://vcpkg.io) to build the required libraries. | ||
|
||
Older versions will automatically download required libraries from [WebKitRequirements](https://github.com/WebKitForWindows/WebKitRequirements) when you perform a `build-webkit`. | ||
It checks the latest WebKitRequirements every time, so it is recommended to use `--skip-library-update` for incremental builds to speed up the next time. | ||
``` | ||
python Tools\Scripts\update-webkit-win-libs.py | ||
perl Tools\Scripts\build-webkit --release --skip-library-update | ||
``` |
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.
@iangrunert @donny-dont
Are you OK with how this documents the change to VCPKG builds?
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'd probably link the commit where vcpkg was introduced https://commits.webkit.org/295042@main
Over time it'll get less likely someone is trying to build WebKit from a hash before that landed in May.
rem Set these for older versions that do not use VCPKG for library building | ||
rem set WEBKIT_LIBRARIES=%~dp0WebKitLibraries\win | ||
path %~dp0WebKitLibraries\win\bin;%path% | ||
rem path %~dp0WebKitLibraries\win\bin;%path% |
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.
Am I correct that these are no longer required for VCPKG build scenarios?
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.
Correct - WebKitLibraries was used for the old library downloading mechanism.
A successful build using the strategy in "WebKit command prompt" to prefix @iangrunert @donny-dont |
Add link to commit that switches to VCPKG for dependency libs.
Note from the build log: |
Uh oh!
There was an error while loading. Please reload this page.