Add MSVC build scripts & fix for MSVC#4
Conversation
|
Thanks! Clearly, AI was involved. This makes me wonder if the changes could actually be simpler - AI easily writes a lot of code without "thinking" whether that was in fact required. Specifically, is the PowerShell script needed, doesn't MSVC also provide
Why do you call it "OpenMP-only"? |
|
Oh, I see the collision-check was in fact OpenMP only. So it was a bug it wasn't guarded. OK then. |
solardiz
left a comment
There was a problem hiding this comment.
I request some trivial changes (such as the SCHED_RR check) and make comments for discussion (such as whether we need the PowerShell script).
For the changes, please use force-pushes of the existing commits - don't add more commits.
|
@solardiz Hello, thank you for quick review. Of course things would be much easier if MSVC supports
Also, the reason that NMAKE or MSYS2 make isn't being used for MSVC is that basically, MSVC doesn't export it's binaries to PATH so the correct method of using them would be, 1. targeting Those two processes are very painful when you do it manually and that is why powershell script, which is something that comes with Windows installation would be here as a perfect solution to handle all this mess. ( You can also see my powershell script for building Bitcoin with MSVC tools here https://github.com/quadcpu/bitcoin/blob/msvc-31.0/build-win.ps1 https://github.com/quadcpu/bitcoin/blob/msvc-31.0/build-win.md, the official bitcoin docs suggest of the simple use of |
- build.yml: use 'make check' instead of a manual build+diff, and drop the -lrt/-s linker flags on Linux and macOS (only the benchmark needs them). - build.yml: build and run the benchmark on Linux and Windows for compile coverage of benchmark.c (macOS lacks the OpenMP it needs). - benchmark.c: lead with the POSIX block (#ifndef _MSC_VER), wrap the SCHED_RR block in #ifdef SCHED_RR, and drop the now-unneeded geteuid shim. - build.ps1: 'benchmark' target now builds and runs benchmark.exe.
|
I have also made some changes that could possibly address the pointed out issues. I will also apply same changes to openwall/yescrypt#11 when this PR is merged |
| echo PASSED | ||
| make check LDFLAGS="" | tee /dev/stderr | grep -qx PASSED | ||
| make benchmark | ||
| ./benchmark |
There was a problem hiding this comment.
Is the purpose of | tee /dev/stderr | grep -qx PASSED to get the exit code right? If so, we should probably enhance Makefile so that make check gives the right exit code directly, if it doesn't already.
Similarly, doesn't make check already build and run benchmark? Do we end up running it twice here?
There was a problem hiding this comment.
Is the purpose of | tee /dev/stderr | grep -qx PASSED to get the exit code right?
Yes, and I just fixed the makefile to produce correct error code
Similarly, doesn't make check already build and run benchmark?
It doesn't, I just checked and still we need to run make check , make benchmark and ./benchmark ( for powershell script build.ps1 -Target benchmark does run benchmark.exe though
|
Thank you! As I wrote:
Would it be practical for you to roll your changes into the original commit? I don't want a separate "Address PR comments" commit in this project's history, but I do want to include long-term relevant commit info from here. |
Make correct exit code for make check
@solardiz Yes, I left commits without squashing it for better review. ( and since that I would reuse commit changes to apply it on other PR as well ). Would be great if you can sqash merge the PR so that it would end up in a single commit merge. |
| # Then build and run "benchmark" to compile-cover its OpenMP/POSIX path | ||
| # (it keeps the default -fopenmp and -lrt, which gcc on Linux supports). | ||
| # runs the "tests" target, then exits non-zero on a mismatch. The "tests" | ||
| # binary needs neither -lrt nor -s, so LDFLAGS="" drops them. Then build |
There was a problem hiding this comment.
Sure -s -lrt are unnecessary for tests on recent glibc, but why bother dropping them? Let's not introduce unneeded complexity and unnecessary explanations here.
|
OK, this almost looks good. I think we also need your copyright+license statement for the PowerShell script, similar to what I use for other files in here if possible. |
* Removed unnecessary comments on build.yml * Added Copyright Header on PS1
|
@solardiz Done, I have also added appropriate copyright header on .ps1 file since I am not the repo owner here haha |
It's weird to have this copyrighted by me when I didn't contribute to it at all. Are you saying you're assigning me the copyright for your (or your AI's) work? |
|
@solardiz Yeah, if it is required would be adding "Copyright 2026 CPUchain" on top of the file though |
|
@quadcpu Copyright assignment is good, but I suggest adding a clarification that this was "Contributed by CPUchain with copyright assigned to the project's original author". Just this additional line. |
I'll plan on doing squash-and-merge, although I'm mildly unhappy this puts GitHub as the committer. I prefer it when the PR contributor uses force-pushes, so that the commits can then be merged as-is. |
|
@solardiz I just forgot ps1 file was a transpile of Makefile assisted by AI. I have just added appropriate copyright header onto that file thank you.
Sounds good enough, when it is squashed merge I will also update the PR on yescrypt as well Now added the commit |
|
Merged, thank you! Please proceed with yescrypt now. |
|
I've just amended the commit in |
Adding build scripts for Windows MSVC and applied some fixes for MSVC compiler ( which doesn't support many of C lang keywords and unix apis )
This is implemented because Rust and Node.js still requires MSVC targets for c bindings ( maybe they would add support for clang-cl at the future, but I think it would not happen at least on upcoming years ), and since gcc / clang still remains experimental for windows support :(
Source fixes for MSVC (all guarded so GCC behavior is unchanged):
(save/nsave) inside the existing #ifdef _OPENMP.
It builds on Windows like https://github.com/quadcpu/yespower/actions/runs/27429768034/job/81076781413