-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Port Socket::Address
to win32
#10610
Port Socket::Address
to win32
#10610
Conversation
{% if flag?(:win32) %} | ||
Socket.ip?("::1:2:3:4:5:6:7").should be_false | ||
{% else %} | ||
Socket.ip?("::1:2:3:4:5:6:7").should be_true | ||
{% end %} |
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.
This is a bit weird. Do you know why this differs?
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.
No idea, but probably a glitch in the implementation.
There are actually a number of expectations commented out in that spec because they fail with some implementations. I just thought it would be better to test that expectation for POSIX implementations. We could potentially skip the win32 branch because it's faulty. But this at least documents a known failure.
And maybe we could do something about fixing such issues outselves at some point.
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.
This now returns true on Windows CI on master since https://github.com/crystal-lang/crystal/runs/5044032259?check_suite_focus=true, which may be due to windows-latest
upgrading to Windows Server 2022.
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.
Oh that's great. However, now we must check for the windows version 🙈 Or just skip this spec on windows...
@straight-shoota Could you clarify which one I should review? This looks very similar to #10650 . Should one of them be closed? |
Adds bindings for win32 API and integrates them with
Socket::Address
. The winsock2 implementation is pretty close to the BSD socket implementation used on POSIX systems, so there's not much more change necessary.The first commit moves common features from
socket.cr
to a new filecommon.cr
which allows to require it without the main socket code (this is essentially a reboot of #6717).Implements a fraction of #9544