-
Notifications
You must be signed in to change notification settings - Fork 127
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
[Feature Request]: Create Windows CI Support #18
Comments
@JiaT75 Could you specify which flavors of msys2 would you like to support? There are different combinations of arch, compiler, c standard library: https://www.msys2.org/docs/environments/ I had recent experience of setting GitHub workflows for Windows builds and msys2, I might be able to help. Update 1: "MinGw and CMake", ok, I see that Update 2: Here is GH Action to setup msys2 https://github.com/msys2/setup-msys2 with pretty detailed README. |
@arixmkii Eventually, I would like to support the MSYS, UCRT64, and MINGW64 flavors in CI runners. Along with MSVC CMake of course. The other ones either don't run on x86-64 (which is what the GitHub-hosted runners support), or are the same but just use clang. The clang build is already tested by the MacOS runner, so its probably not necessary to test it on Windows. I don't develop or use xz or liblzma on Windows, but we have a few users who do. My note about MinGw and CMake was more about testing the autotool and CMake builds on Windows. So, I think using msys2 makes sense since it is likely many of our users do so. To start, I think just picking one environment would be great. I feel the most useful one would be MINGW64 (msvcrt) if we also have CMake testing ucrt. Our README-Windows.txt suggests that we only support msvcrt, but this is likely outdated. Since we already have a lot of runners and configurations, maybe the other environments could be in a separate Workflow that we run manually pre-release or if we are addressing a Windows specific bug. Thanks for offering to help with this! I was planning on working on this soon, but I kept delaying it to work on other things. Your help would be greatly appreciated if you have the time to do it :) |
@JiaT75 I did something. At least it runs full autotools set on Windows host. I don't yet understand CMake part and also VisualStudio parts, but might be able to take another look some time later. Any additional context, what you think could help is highly appreciated. |
I am not sure how experienced you are with CMake, but we shouldn't need anything too complicated. I haven't fully thought through it, but here are a few ideas I have had so far:
I hope this helps! Thanks again for your contributions so far. Let me know what other questions you may have. |
@arixmkii Are you still working on this? If you don't have time to finish it, no need to worry. I can finish up the last few changes and close this out. |
I was trying to compile XZ with cmake(included in Visual Studio 2022) and below was my observation: Step 1: Set build environment
Step 2: Configure build environment:
Setp 3: Start build: Result:
I found liblzma.dll is not getting generated. Then I configured using: And the liblzma.dll was generated. Looks like you need to put following in CMakeLists.txt: |
Hi! This question doesn't exactly belong on this issue. This issue is for discussing changes to our Continuous Integration scripts for improving Windows support (which I still need to improve/finish). Anyways, I will still answer your question here.
We have always had the Thank you for this report, but we will not be changing this. |
Ok, Thanks. |
Hi @JiaT75 Thank you for the ping. Got it out of my focus. I will revisit the topic this weekend and update. I hope to get Windows part done. |
@arixmkii I ended up making the Windows-CI work with MSYS2, but it would be great if things worked with MSVC (which is why I left the Issue open). If you have experience working with MSVC that is great. I imagine this would require writing a Windows PowerShell or Batch script to do things similar to what ci_build.sh does, and then refactoring windows-ci.yml to utilize the script. We need to use CMake to generate Windows Visual Studio files (using CMake's Recently we ported the xz command line tool to work with MSVC so making sure we can continue to build on MSVC as the codebase changes is certianly valuable. |
Hi @JiaT75 I had no problems compiling with MSVC only (cmake and tools shipped by Visual Studio 2022). Thanks a lot for setting the cmake files up for this. Are there any plans adding MSVC CI Builds (including binary releases) to the CI pipeline? I can help out on this, if there is any interest. Best Link to the mentioned CI action: https://github.com/ndu2/xz/actions/runs/8224685611 (see also the files windows/build-with-vs2022.bat, .github/workflows/windows-vs2022-ci.yml) |
Would Visual Studio + Clang-cl be worth trying too? The inline x86-64
assembly code in 5.6.x is compatible with GCC and Clang, so I hope that
compiling with Clang-cl would result in better decompression speed than
compiling with MSVC. (LZMA SDK has MSVC compatible assembly but then
one needs to use LZMA SDK's C code and APIs too.)
|
Good point, I wasn't aware of that assembly code. I did a quick test on a virtual windows 10: MSVC against clang-cl (needed
using xz with all default settings (xz.exe -z raw, xz.exe -d raw.xz) and took the timings: 1: almost identical (differences <2%) I will bench with a "real" windows later and update here update: i see a performance increase of up to 25% on windows 11 AMD Ryzen 7 PRO 7840U. |
Thanks @ndu2 for doing some benchmarking and the demo pipeline! I hadn't forgotten about this but we had higher priority things to work on instead. MSVC support for xz is still fairly recent, so adding it to the CI pipeline would be great. I'll take a look at your demo pipeline hopefully soon :) The existing CI tests likely need a bit of a clean up anyway, some parts were written hastily by me. They have proven to be great for catching bugs though. We don't have plans for using CI for releases. The assembly code is only for decoding so that explains your results. Using larger test files may help be sure that the results aren't due to noise. |
Thank you @JiaT75 and @Larhzu for the quick feedback.
Yes, I think so. I added Clang-cl builds. There is just one include missing (Originally I required more patches due to my inability to properly generate the vsxproj with cmake) Patch: ndu2@5fb2ace
I just updated the demo pipeline to "somewhat complete" state that suffices my own purposes:
Well, I'm neither familiar with github actions nor with windows batch (nor powershell). But I stuck to the "out of the box" tools for better compatibility with VS2022/Windows only setups. You'll find the implementation it those 2 files:
I found 13 test-executables (test_*.exe). I added calls to those in
OK. To be honest, Windows releases for v5.6.x is what brought me here at the first place :-) Please let me know if any of this work is of your interest and how to proceed. I'm more than happy to create PRs, adapt the scripts to the projects needs and clean things up as required. Feel also free to grab what you need. Best |
This will be really helpful to include since for now I just test things locally on a VM with x64 MSVC. Automating this plus extending coverage will save me some effort :)
I haven't worked much with Windows Batch scripting or PowerShell either :/
We currently just use the built-in test harnesses for our Autotools and CMake builds. The way you have it now seems logical, to just loop through the test executables and run them, although the best way to report the errors may need to be looked at. Maybe this is something we could add to
We had another recent request for Windows binaries, so we will more seriously consider this. We need to verify there are no license restrictions preventing us from distributing Windows binaries with the compiler we choose to use (MinGW-w64, MSVC, Clang-cl, etc.). Also, I would probably want to generate the Windows binaries locally instead of relying on GitHub runners. The GitHub CI runners are a common attack surface these days so it could be an extra risk. Currently, we only use CI for testing so if the GitHub runners are compromised then its not a security threat.
I cloned your fork already, so no need to make a PR unless you want to. I suppose it could be helpful to keep the conversation focused on various parts of the code. We usually don't merge PRs directly anyway. Instead we usually take commits we like and adapt the other parts as needed. Don't worry, you'll still be the Author on any commits that are mostly unchanged :) |
Describe the Feature
Both MinGw and CMake Windows builds should be tested.
Expected Complications
Downloading the correct build environment dependencies could be tricky. GitHub might have some reusable components to help with this.
Will I try to implement this new feature?
Yes
The text was updated successfully, but these errors were encountered: