-
Notifications
You must be signed in to change notification settings - Fork 799
Look for pthreads-win32 when building with MSVC #1412
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: master
Are you sure you want to change the base?
Conversation
Note that MSYS64 identifies itself to Meson as 'cygwin', and has a pthreads-compliant threads implementation. The sources for pthreads-win32 were once included in this project as a requirement for MSVC, but never made it into the Meson config and were removed at some point (I don't know which happened first).
Just to clarify, when you mean MSYS64, do you mean msys2's https://packages.msys2.org/base/meson? |
|
Oh, that's a good question actually. I now have multiple versions of Meson installed, and they are giving different answers, and you are correct that the MSYS2 version is the one identifying itself as 'cygwin'. Using the term MSYS64 is confusing and I wish I could amend my commit message. Because my change tests for both the system and compiler, it should be correct. If I follow the letter of the law from https://github.com/Netflix/vmaf/blob/master/resource/doc/windows.md , then I should be using the MinGW-w64 version, but I already had an older version of meson, ninja, and nasm installed into my MSYS2 base environment. Here's what all the versions of Meson on my system are reporting: Version installed from meson-1.6.1-64.msi, configured to use MSVC with a native.ini file: Old version installed into MSYS2, configured to build using GCC: New version from MinGW-w64: |
|
Okay, I was just a tad confused, since I normally associate the string (case-insensitive) "MSYS64" with |
Yes, I am in the same camp. @sethk If we want to add support for MSVC, is this something that can be verified via CI? |
I'm starting to see a pattern here guys. :)
Theoretically yes, although installing Visual Studio on a headless system is a huge PITA. The last time I tried, it got stuck on some dialog box, despite all the --unattended flags and whatnot. I don't have any experience using it with GitHub Actions, maybe there is some provision for it. Or maybe clang-cl is the answer; I have no experience with that. I wonder how it deals with pthreads... Bringing back MSVC support is not that far off, but it's gonna require 3-4 more patches from me that I need to clean up and test. I presume MSVC worked even after the transition to Meson due to the references to 'msvc' in the build config, and the presence of libvmaf/src/compat/msvc. If you guys would rather drop official support for MSVC/MSVS that would be fine, and you can close #1410, and I can maintain my own fork. The most controversial change I have is that MSVC's C11 implementation lacks variable length arrays, but they added support for enough of stdatomic.h recently that it can be used in place of libvmaf/src/compat/msvc/stdatomic.h with VS2022. |
|
If you're going to add MSVC support, why not just add a win32thread implementation? |
I do think that would be ideal, but for my uses my only goal was to get it to compile. I presume due to the way that the pthreads-w32 sources were added to the project previously, that the work to implement win32 threads would be non-trivial. I also want to stress that I'm not trying to add support for MSVC, I'm just trying to get it to work again the way it did before. And I have succeeded, using some quick-and-dirty hacks that I have been slowly turning into nice patches. The pthreads dependency is just one thing. :) |
|
Okay guys, I have some great news. I put all my MSVC-related changes onto a branch on my fork, and I got libvmaf building with MSVC on GitHub actions. I had to tweak this change to allow for different names pthreads-win32 is packaged as. There are a few other changes that I haven't submitted PRs for yet that are required for this to work, in addition to the changes to the workflows/libvmaf.yml. https://github.com/sethk/vmaf/actions/runs/14507684662/job/40700006474 The Windows version doesn't install correctly, so binaries can't be uploaded yet. I think it's close to working though. The tox tests are also failing for multiple reasons. Example of install failure: https://github.com/sethk/vmaf/actions/runs/14506207926/job/40696005366 During this process I found out that c11threads are a standard that exists, and even if they don't exist under MSVC, there is a minimal wrapper for them here: https://github.com/jtsiomb/c11threads . So I think that could be a great direction to go in instead of porting libvmaf to win32thread, just go from pthreads API to c11threads and include this wrapper, which is in the public domain. |
MSVC 17.8 has native support for C11 Threads. |
@sethk Great, I think having CI for this would give us the confidence to merge your MSVC related changes. |
Note that MSYS64 identifies itself to Meson as 'cygwin', and has a pthreads-compliant threads implementation.
The sources for pthreads-win32 were once included in this project as a requirement for MSVC, but never made it into the Meson config and were removed at some point (I don't know which happened first).