Skip to content

Conversation

ayeteadoe
Copy link
Contributor

@ayeteadoe ayeteadoe commented May 12, 2025

NOTE: This work depends on #4698 being merged into upstream first, but I'm putting it up as a draft for now to get feedback on this approach of gradually getting Windows CI on par with Unix, reducing the chances of Windows bitrot as things are built up.

@ayeteadoe
Copy link
Contributor Author

FYI @ADKaster

@ayeteadoe
Copy link
Contributor Author

Here is a log of what running ctest does with the windows_ci_ninja preset:

windows_ci_ninja.log

@ayeteadoe ayeteadoe force-pushed the windows-ak-testsuite branch from f96181d to 418b2cd Compare May 12, 2025 18:13
@ayeteadoe
Copy link
Contributor Author

Also, MSVC supports ASAN (https://learn.microsoft.com/en-us/cpp/sanitizers/asan?view=msvc-170) via /fsanitize=address so I could add that to the CI configuration preset as well

@ayeteadoe
Copy link
Contributor Author

Ah, looks like there is an issue with Clangs Windows ASAN impl at the moment. We fail to build LibTextCodec as the GenerateEncodingIndexes executable fails at run time with the following:

[55/62] Generating LookupTables.h, LookupTables.cpp
FAILED: Lagom/Libraries/LibTextCodec/LookupTables.h Lagom/Libraries/LibTextCodec/LookupTables.cpp C:/Users/to_co/Documents/Development/ladybird-windows/Build/debug/Lagom/Libraries/LibTextCodec/LookupTables.h C:/Users/to_co/Documents/Development/ladybird-windows/Build/debug/Lagom/Libraries/LibTextCodec/LookupTables.cpp 
C:\WINDOWS\system32\cmd.exe /C "cd /D C:\Users\to_co\Documents\Development\ladybird-windows\Build\debug\Lagom\Libraries\LibTextCodec && C:\Users\to_co\Documents\Development\ladybird-windows\Build\debug\bin\GenerateEncodingIndexes.exe -h LookupTables.h.tmp -c LookupTables.cpp.tmp -j C:/Users/to_co/Documents/Development/ladybird-windows/Libraries/LibTextCodec/indexes.json && "C:\Program Files\JetBrains\CLion 2024.1.1\bin\cmake\win\x64\bin\cmake.exe" -E copy_if_different LookupTables.h.tmp LookupTables.h && "C:\Program Files\JetBrains\CLion 2024.1.1\bin\cmake\win\x64\bin\cmake.exe" -E copy_if_different LookupTables.cpp.tmp LookupTables.cpp && "C:\Program Files\JetBrains\CLion 2024.1.1\bin\cmake\win\x64\bin\cmake.exe" -E remove LookupTables.h.tmp LookupTables.cpp.tmp"
==32936==interception_win: unhandled instruction at 0x7ffc24413000: 44 0f b6 1a 4c 8b d2 48
==32936==interception_win: unhandled instruction at 0x7ffc24413000: 44 0f b6 1a 4c 8b d2 48

It looks like LLVM team addressed this in llvm/llvm-project@8417f6a; however, the release this went into looks to be 20.1.0, but the latest Visual Studio 2022 (I'm on 17.13.6) is only on 19.1.1:

C:\Users\to_co\Documents\Development\ladybird-windows\Build\debug>"C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/Llvm/x64/bin/clang-cl.exe" --version
clang version 19.1.1
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\Llvm\x64\bin

So unfortunately probably need to wait a bit until Visual Studio itself upgrades the version of LLVM it ships with to 20.1.x+ so we can test locally, and then wait for Github Actions to support that Visual Studio version on windows-latest.

It does look like Github Actions is planning to add support for LLVM 20.1.x+ in early June, but the one time I tried building ladybird outside of the Visual Studio-based LLVM toolchain things did not go well. Not sure it's worth the effort to support standalone LLVM clang-cl builds when things should just work:tm: if we wait a bit.

@ADKaster
Copy link
Member

If GHA supports VS Preview, I have no qualms about using that. That's what I've been using the entire time to test changes.

@R-Goc
Copy link
Contributor

R-Goc commented May 13, 2025

Is there an easy way to download llvm from the llvm-project GitHub? They are the same binaries as what VS ships(yes, really).

@ADKaster
Copy link
Member

Anything that's more complicated than "use what's already there" or "Wait two weeks" seems not a good use of time.

@ayeteadoe
Copy link
Contributor Author

ayeteadoe commented May 13, 2025

Is there an easy way to download llvm from the llvm-project GitHub?

Yes, it is very simple to install, I actually did install the latest LLVM when investigating above so I could try and confirm if the ASAN issue was indeed resolved. But I setup my CLion toolchain to be outside of VS toolset (i.e. the Developer Command Prompt/vcvarsall.bat)

They are the same binaries as what VS ships(yes, really).

Actually, that is a good point, I bet I could use the VS toolset environment still, but point to my 20.1.4 clang-cl install and not use Visual Studios. Let me try that out...

And it worked!
image

@ayeteadoe
Copy link
Contributor Author

So that means as of actions/runner-images#12001, we can just setup the Windows CI workflow to use whatever current visual studio is available but set CMAKE_C_COMPILER and CMAKE_CXX_COMPILER to the latest Github Actions LLVM release, and then we can have Windows running with ASAN!

@R-Goc
Copy link
Contributor

R-Goc commented May 13, 2025

Well what I meant is if there is an easy way to do it in CI. I do use the llvm-project version myself.

@ayeteadoe
Copy link
Contributor Author

ayeteadoe commented May 13, 2025

Well what I meant is if there is an easy way to do it in CI. I do use the llvm-project version myself.

Ah sorry misunderstood, but yeah at that point I feel like Windows CI can just be regular until windows-latest updates to a Visual Studio that ships LLVM 20.1.x+, rather than add the custom Github Actions tech that will be redundant not long after

@github-actions github-actions bot added the conflicts Pull request has merge conflicts that need resolution label May 14, 2025
Copy link

Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest master.

@ayeteadoe
Copy link
Contributor Author

Tested on Windows via a clean build (modulo removing vcpkg_installed) with the windows_ci_ninja CMake preset. Going to leave the ASAN stuff out of this for now given GHA doesn't support it yet anyways, but I do have it working locally so once there is support I'll put up another PR at that point.

configure_windows_ci_ninja.txt

build_windows_ci_ninja.txt

test_windows_ci_ninja.txt

@github-actions github-actions bot removed the conflicts Pull request has merge conflicts that need resolution label May 19, 2025
@ayeteadoe ayeteadoe marked this pull request as ready for review May 19, 2025 01:45
@ayeteadoe ayeteadoe force-pushed the windows-ak-testsuite branch 2 times, most recently from c365ccb to 3403033 Compare May 19, 2025 14:20
@ayeteadoe ayeteadoe force-pushed the windows-ak-testsuite branch from 3403033 to 70a5562 Compare May 19, 2025 14:57
@ayeteadoe ayeteadoe requested a review from ADKaster May 19, 2025 16:47
@ayeteadoe
Copy link
Contributor Author

Ah sorry only saw your post in ui-windows about your pre-req PR to cleanup the CMake formatting here after I requested a re-review

@ayeteadoe
Copy link
Contributor Author

Now depends on some clean up in #4821

@github-actions github-actions bot added the conflicts Pull request has merge conflicts that need resolution label May 19, 2025
Copy link

Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest master.

ayeteadoe added 2 commits May 19, 2025 16:15
This will allow us to gradually build up official support for Windows,
as only targets we have explicitly marked as supported on windows will
be built and ran during CI.
We now explicitly enabling support for the minimum libraries needed
to build and run the AK testsuite. 81/82 tests are running and
passing. The exception is LexicalPath, as some path behaviour on
Windows is different than Unix, so the current tests will have lots of
platform specific failures. The implementer of LexicalPathWindows
recommended windows-specific tests here, so I will do that in a
follow up.
@ayeteadoe ayeteadoe force-pushed the windows-ak-testsuite branch from 70a5562 to 636211f Compare May 19, 2025 23:16
@github-actions github-actions bot removed the conflicts Pull request has merge conflicts that need resolution label May 19, 2025
Copy link
Member

@ADKaster ADKaster left a comment

Choose a reason for hiding this comment

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

I think this change is pretty much ready now, thanks for re-working it on top of my CMake changes

@ADKaster ADKaster merged commit 8cf01a2 into LadybirdBrowser:master May 20, 2025
7 checks passed
@ayeteadoe ayeteadoe deleted the windows-ak-testsuite branch May 20, 2025 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants