Skip to content
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

Windows support #53

Open
promag opened this issue Apr 6, 2021 · 7 comments
Open

Windows support #53

promag opened this issue Apr 6, 2021 · 7 comments

Comments

@promag
Copy link

promag commented Apr 6, 2021

I think this should be referenced here:

Supporting windows, not just unix systems. The existing socket code is already cross-platform, so the only windows-specific code that needs to be written is code spawning a process and passing a socket descriptor. This can be implemented with CreateProcess and WSADuplicateSocket. Example: https://memset.wordpress.com/2010/10/13/win32-api-passing-socket-with-ipc-method/.

From https://github.com/bitcoin/bitcoin/blob/master/doc/multiprocess.md

Is there anything else worth mentioning? I could give it a try.

@ryanofsky
Copy link
Collaborator

This is interesting since that I think comment was written before libmultiprocess existed as a separate library.

I think basically the comment is still applicable and can be used to implement windows versions of mp::SpawnProcess and mp::WaitProcess (with the latter calling WaitForSingleObject and GetExitCodeProcess).

Function signatures in mp/util.h should also change slightly to take generic socket and process handles instead of using ints. Maybe should look something like:

#ifdef WIN32
using SocketId = HANDLE;
using ProcessId = HANDLE;
#else 
using SocketId = int;
using ProcessId = int;
#endif

using FdToArgsFn = std::function<std::vector<std::string>(SocketId fd)>;
int SpawnProcess(ProcessId& pid, FdToArgsFn&& fd_to_args);
int WaitProcess(ProcessId pid);

@ryanofsky
Copy link
Collaborator

Another part of this is that signatures of mp::ConnectStream and mp::ServeStream should change to accept windows handles:

template <typename InitInterface>
std::unique_ptr<ProxyClient<InitInterface>> ConnectStream(EventLoop& loop, SocketId socket);

template <typename InitInterface, typename InitImpl>
void ServeStream(EventLoop& loop, SocketId socket, InitImpl& init);

I think this should only require replacing the int type with the generic SocketId though.

@ryanofsky
Copy link
Collaborator

ryanofsky commented Apr 6, 2021

Also finally some KJ_SYSCALL uses in mp/proxy.cpp might have to be replaced:

int fds[2];
KJ_SYSCALL(socketpair(AF_UNIX, SOCK_STREAM, 0, fds));
m_wait_fd = fds[0];
m_post_fd = fds[1];

KJ_SYSCALL(::close(m_post_fd));

char buffer = 0;
KJ_SYSCALL(write(m_post_fd, &buffer, 1));

char buffer = 0;
KJ_SYSCALL(write(m_post_fd, &buffer, 1));

I think there are straightforward replacements for all of these. Replace socketpair with maybe 2
CreatePipe calls, replace close with CloseHandle write with WriteFile.

EDIT: Should only need to replace socketpair with one CreatePipe call not two. I was thinking 2 because socketpair is bidirectional, but this code is only using the socket for signalling just writing to one end, so even on unix I think this could just use plain pipe.

@promag
Copy link
Author

promag commented Apr 6, 2021

I'm having some issues trying to build on windows.

I downloaded and built capnproto-c++-0.8.0 with msvc2017. Then I set CapnProto_DIR and ran cmake. Then building the solution I have the following error:


C:\Users\joao\Projects\libmultiprocess\build>msbuild Libmultiprocess.sln  /property:Configuration=Release /property:Platform=Win32

[...]

CustomBuild:
  Compiling Cap'n Proto schema include/mp/proxy.capnp
  C:\Users\joao\Projects\capnproto-c++-0.8.0\build\src\capnp\Release\capnp.exe compile: -I C:/Users/joao/Projects/include: no such directory
  Try 'C:\Users\joao\Projects\capnproto-c++-0.8.0\build\src\capnp\Release\capnp.exe compile --help' for more information.

Looks like it's using C:/Users/joao/Projects to search for include/mp/proxy.capnp?

@ryanofsky
Copy link
Collaborator

Hmm, that custom build rule comes from this line in CMakeLists:

capnp_generate_cpp(MP_PROXY_SRCS MP_PROXY_HDRS include/mp/proxy.capnp)

It's possible prefixing include/mp/proxy.capnp as ${CMAKE_CURRENT_SOURCE_DIR}/include/mp/proxy.capnp could help, but this is a guess, and it would probably be good to see if there's way to get more verbose build output (complete command line) to see what could be happening.

In longer run, I assume the best way to get these various packages working together on windows will be with vcpkg, but I don't have any experience using vcpkg yet. I'm expecting there are more build issues waiting after this one. My previous comments above were about porting library code, and I think porting build code should be simpler and require fewer changes than porting library code, but the problems are probably less predictable and may take more time to debug.

@promag
Copy link
Author

promag commented Apr 6, 2021

Same here, not used to code on windows, I'll try your suggestion just to have it build so I can work on the actual code change.

@promag
Copy link
Author

promag commented Apr 6, 2021

Issue is related to CAPNP_INCLUDE_DIRECTORY , not with include/mp/proxy.capnp. Tomorrow I'll try to see why it's getting that value, otherwise I'll set it manually to the correct one just to move forward.

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

No branches or pull requests

2 participants