-
Notifications
You must be signed in to change notification settings - Fork 635
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
Support for Unix Sockets in Windows #396
base: master
Are you sure you want to change the base?
Conversation
674e249
to
b07152b
Compare
@leso-kn Any chance you can review this PR? |
The changes look fine to me, but I do not own a windows machine either. Out of curiosity I brought up some effort to add a Github Actions definition for windows, but it's running into some – what I believe are – runtime dll open issues (see the pipeline log). I'll be calling it a day for today, feel free to have a look at the log and comment. |
Is it possible to have the build result as artifacts? Runtime DLL errors require further inspection into the DLL directly either by using Dependency Walker or improved version of it. |
@MikuAuahDark It's possible to get, but you have to setup the pipeline ahead of time to post the artifacts you want back to the job, you can't inspect and get files out of an old job if it didn't mark them for posting ahead of time. Also since this is a case of a failed job the asset posting needs to be configured such that the failing step of the job does not block the rest of the job steps from running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a merge from master to get new CI workflows with Windows tests running in this PR. That turns up build failures for the Windows jobs. That will need to be resolved...
I'll see what I can do this weekend. |
b07152b
to
c44d18b
Compare
Alright, I rebased my changes based on the master branch and modified the rockspec file. Hopefully it solves the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for responding to feedback on this. It passes CI now so that's something.
The commit list is a bit chaotic, but I'm happy to fix that up into something more conventional, so don't sweat it. I can split up and combine the existing commits to get something cleaner.
Before I do that though can you explain why there are now two Visual Studio projects here? Is there a reason they can't be combined into a single project? I don't understand how VS tooling works obviously but it doesn't make sense to me why there are two different project files for it.
I found out separating it is the best way to go. The Do note that Visual Studio Solutions ( |
Okay then, I guess that settles that. Thanks for the info. |
This PR enables compilation of
socket.unix
module in Windows. Fixes #328.Note that the Makefile change is untested, only the Visual Studio solutions. One thing I noticed that in Windows,
inet.c
must be compiled in Unix sockets. Rockspec changes is also untested.Resulting binary from Visual Studio solutions is tested and works as expected by connecting to Unix socket that's created from PHP. Note that while
unix.dgram
/unix.udp
function is exposed, Unix datagram socket doesn't work in Windows. This is currently Windows limitation.Feel free to cherry-pick as appropriate if this PR as a whole lacked the quality and standards of the repository.