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

Port Socket::Address to win32 #10610

Conversation

straight-shoota
Copy link
Member

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 file common.cr which allows to require it without the main socket code (this is essentially a reboot of #6717).

Implements a fraction of #9544

@straight-shoota straight-shoota added kind:feature kind:refactor platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:networking labels Apr 6, 2021
Comment on lines +279 to +283
{% 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 %}
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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...

@asterite
Copy link
Member

@straight-shoota Could you clarify which one I should review? This looks very similar to #10650 . Should one of them be closed?

@straight-shoota
Copy link
Member Author

straight-shoota commented Apr 23, 2021

#10650 is based on this, it's just the next step. So I'd like to merge this first, then I'll un-draft #10650.

@straight-shoota straight-shoota added this to the 1.1.0 milestone Apr 23, 2021
@straight-shoota straight-shoota merged commit a7544b5 into crystal-lang:master Apr 24, 2021
@straight-shoota straight-shoota deleted the feature/win32-socket-address branch April 24, 2021 17:31
@straight-shoota straight-shoota mentioned this pull request May 10, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature kind:refactor platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:networking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants