Repository navigation
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. LLM Linter (✨ experimental)Possible typos and grammar issues:
Possible places where named args for integral literals may be used (e.g.
2026-10-20 10:08:30 |
|
This will need whitelisting of But I'm thinking of switching to cross-compilation, since that's how most users will use this library anyway. |
|
Ok, similar to Bitcoin Core we now cross-compile and then run the result on a Windows machine. The agent made a few other adjustments to the code that I haven't looked at yet. This needs the following whitelist:
|
|
@ryanofsky can you whitelist the upload / download actions I mentioned above? |
|
I think we will need some CI coverage for windows, and this approach looks ok, but it's also compilicated. I wonder if it would be possible to implement a more minimal approach using nix's mingw compiler and wine ( This would seem helpful for local development and debugging. If CI job fails just run it locally with |
|
I'll first try to get the current attempt to run and then will look at some different approaches. Building with nix makes sense. Running in wine could also make sense, though I suspect a native run will catch other things, so maybe we should do both. |
bc01b38 to
bbe64cf
Compare
|
I switched cross-compilation to nix. The (cold cache) build takes very long on CI, so I'll check again tomorrow and then see if I can clean things up. |
|
Alright, CI green! @ryanofsky I'll wait for you to get #231 green again before rebasing and then I'll look into cleanup. You can cherry-pick things out of here if you think they're correct, but otherwise feel free to ignore. I need to study that PR first to better understand what does and doesn't make sense here. |
|
Thanks! I looked more at the nix/wine changes here than the native windows stuff but it looks like everything makes sense. My plan is to split #231 up and open a new PR that only makes non-windows changes. Then #231 can depend on that and this can depend on #231. Alternately, this PR could be implemented independently of the other PRs by just having the windows &wine jobs build libmultiprocess and run mptest, adding a dummy test that can run on windows like KJ_TEST("Hello world")
{
KJ_EXPECT(true);
}and skipping the other tests that don't compile on windows yet. But this is useful for testing in its current form too. Just suggesting in case you are impatient to get this merged. |
|
I'm happy to stay based on #231 so there's some useful coverage. |
|
Rebased. The agent ended up dropping some commits so that's a good sign. @ryanofsky can you check if 07a7f71 has something useful for #231? I bumped the capnproto version for Windows builds to 1.4.0. That gets rid of one patch: 67e5833#diff-022254a27cb514a71c153b75204b316472ba0a5bdf9e20041d2fc17cc2007806 Although it's a small patch, I don't see a good reason to support older versions when we're adding a new platform like this. Especially because I expect most mining users to use our binaries. The other patch I just upstreamed: capnproto/capnproto#2633 |
Yes it looks like this fixes the same issue @ViniciusCestarii reported #231 (comment). The fix there seems simpler, though, and I think a better fix might be possible by dropping the winsock2.h include header to avoid windows headers being pulled in so many places. The main commit here d97c586 also looks good, though the commit message looks out of date, it says it is testing an msvc build but only actually testing a cross-compiled one |
|
I probably going to hold off rebasing this until after #274 lands, or at least I've had some time to review the windows changes myself. |
Remove recently introduced SpawnConnectInfo and SpawnConnectInfoToArgsFn type aliases since they are the same on all platforms and might obscure the fact that connect info should be treated as an opaque string. Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
This change was written with Claude Sonnet 4.6.
Replace the Unix-only UnixListener test helper with a cross-platform SocketListener backed by a std::variant<sockaddr_un> (TCP arm will be added later for Wine compatibility). - test/socketlistener.h: New header with SocketListener class extracted from the pattern of UnixListener but using SocketId/SocketError types, std::filesystem for temp dir cleanup, and mp::CloseSocket for teardown. Uses std::variant<sockaddr_un> so the TCP arm can be appended later without restructuring the class. - test/unixlistener.h: Deleted. - test/listen_tests.cpp: Switch to socketlistener.h and socketlistener. - test/connect_tests.cpp: Switch to socketlistener.h and socketlistener. This change was written with Claude Sonnet 4.6.
Move POSIX-only headers in util.cpp into a #ifndef WIN32 block so the file compiles on Windows without modification. Guard the Unix-only helpers (MakeArgv, MaxFd, ChildFail, SpawnError*, ReadSpawnResult, WriteSpawnError, KillAndReapChild) with a single #ifndef WIN32 block. Guard the extern "C" environ declaration likewise. Guard Unix-only includes in spawn_tests.cpp and connect_tests.cpp and wrap the Unix-only spawn tests with #ifndef WIN32. No Windows implementations are added here; those come in a later commit. This change was written with Claude Sonnet 4.6.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
Add Windows-specific code to support building and running on Windows: - util.h: Guard ProcessId/SocketId/SocketError type aliases with WIN32 ifdefs so they use SOCKET/uintptr_t on Windows and int on Unix. Add winsock2.h include on Windows. - util.cpp: Guard Unix-specific system headers with WIN32 ifdefs. Add Windows-specific includes (windows.h, winsock2.h). Guard MaxFd() with #ifndef WIN32. Add GetCurrentThreadId() branch in ThreadName(). Add win32Socketpair() forward-declare. Add Windows branch in SocketPair() using win32Socketpair(). Add CommandLineFromArgv() helper needed to construct CreateProcess command lines. Add Windows branch in SpawnProcess() using named pipes and WSADuplicateSocket to pass socket to child. Add Windows branch in StartSpawned() reading socket from named pipe. Add Windows branch in WaitProcess() using WaitForSingleObject/GetExitCodeProcess. - proxy.cpp: Add SocketOutputStream class on Windows (analogous to FdOutputStream but using SOCKET/send()). Add Windows branch in EventLoop constructor to create m_post_writer using SocketOutputStream. Co-Authored-By: ViniciusCestarii <viniciuscestari01@gmail.com>
…g to named pipe SpawnProcess on Windows creates a named pipe with PIPE_WAIT and then calls ConnectNamedPipe synchronously. If the child exits or crashes before connecting, ConnectNamedPipe blocks forever with no recovery path. Fix by opening the pipe with FILE_FLAG_OVERLAPPED and using WaitForMultipleObjects on both the connect event and the child process handle. If the process handle signals first, the child died without connecting and SpawnProcess throws instead of hanging. Since the pipe is now in overlapped mode, WriteFile also requires an OVERLAPPED structure; use GetOverlappedResult with bWait=TRUE to handle both synchronous and asynchronous completion. Add a Windows-only test that spawns a child which exits immediately without opening the named pipe and asserts SpawnProcess does not block. (bitcoin-core#231 (comment)) Co-Authored-By: ViniciusCestarii <viniciuscestari01@gmail.com> This change was written with Claude Sonnet 4.6.
Allow code to compile without pthreads available, as required for MSVC compatibility. Avoid unconditional POSIX calls (fork, posix_spawn, pthread_getname_np) by moving them into #ifndef WIN32 or HAVE_PTHREAD_* guards. When pthreads is available on Windows (detected via cmake HAVE_PTHREAD_* checks), still use it for thread name reporting since it provides useful information at low cost. Also add Threads::Threads as an explicit dependency of the multiprocess library. proxy.cpp directly uses thread_local, std::this_thread, and std::thread, and the dependency was previously satisfied only through transitive linkage from CapnProto::kj-async. This change was written with Claude Sonnet 4.6.
Add TCP (sockaddr_in) support to SocketListener and platform-conditional header guards so the class compiles and works on Windows: - Wrap POSIX headers (arpa/inet.h, netinet/in.h, sys/socket.h, sys/un.h) in #else of #ifdef WIN32, with Windows equivalents (afunix.h, ws2tcpip.h) in the #ifdef WIN32 branch. - Add a WsaInit static initializer on Windows that calls WSAStartup before any socket operations. Tests create sockets directly (not via StartSpawned), so Winsock must be initialized here. - Add Init(sockaddr_in&) and Connect(const sockaddr_in&) overloads for TCP loopback connections. - Use TCP on Windows to work around Wine's lack of the AcceptEx extension required by KJ's AF_UNIX listener; Unix platforms continue using AF_UNIX. - Expand std::variant<sockaddr_un> to std::variant<sockaddr_in, sockaddr_un>. This change was written with Claude Sonnet 4.6.
- shell.nix: add `windows` parameter that selects pkgs.pkgsCross.mingwW64
as the cross target; also change crossPkgs default from import<nixpkgs>{}
to null (cleaner API). When windows=true, add native pkgs.capnproto to
nativeBuildInputs so capnp/capnpc-c++ are in PATH for cmake code generation,
and add wine64Packages.staging so ctest can run mptest.exe via wine.
Change llvmBase to always use pkgs (native) instead of crossPkgs.
- ci/configs/windows.bash: new config that cross-compiles with mingw,
sets CMAKE_SYSTEM_NAME=Windows, CMAKE_FIND_ROOT_PATH_MODE_PROGRAM=NEVER
(so cmake finds native capnp from PATH), CMAKE_CROSSCOMPILING_EMULATOR=wine
(so ctest runs mptest.exe via wine), and sets MPGEN_PRE_BUILD=1.
- ci/scripts/ci.sh: add MPGEN_PRE_BUILD support: when set, build native mpgen
in $CI_DIR-native before the main cross build, then inject
-DMPGEN_EXECUTABLE into CMAKE_ARGS. This is needed because cmake's
add_custom_command does not use CMAKE_CROSSCOMPILING_EMULATOR, so the
cross-compiled mpgen.exe cannot be used as a code generator directly.
This change was written with Claude Sonnet 4.6.
The proxy-server code generated for test/mp/test/foo.capnp now has more sections than MSVC's default object file format allows, failing the Windows native VS CI job with "error C1128: number of sections exceeded object file format limit: compile with /bigobj" (https://github.com/bitcoin/bitcoin/actions/runs/36945085020/job/110645241551). Bitcoin Core already passes /bigobj for its own debug builds. Not reproduced locally; based on the error message. This change was written with Claude Opus 5.5 (1M context).
Move Windows out of the general build matrix while preserving its check name, configuration, and cache keys. Reuse the Nix environment and setup steps with YAML anchors so both jobs keep the same build procedure. This lets follow-up jobs depend on the Windows build without waiting for the other configurations in the matrix.
Package test and example executables from the existing Windows cross-build with their runtime DLL dependencies. Download them on windows-latest and run mptest plus the calculator example on Windows. This complements Wine execution with real Windows runtime testing and a smoke test of spawned IPC processes. Depend directly on the Windows cross-build so native tests can run while the other build configurations are still in progress.
4 participants
Based on #231
CI commits are entirely vibe coded, so will need some polishing if it actually works,