Fix windows libp2p #29
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nd server.go to allow Multicast transmit
| var wcm ipv4.ControlMessage | ||
| for ifi := range c.ifaces { | ||
| wcm.IfIndex = c.ifaces[ifi].Index | ||
| switch runtime.GOOS { | ||
| case "darwin", "ios", "linux": |
There was a problem hiding this comment.
Why is this special-casing darwin, ios and linux, and not windows?
There was a problem hiding this comment.
As I was looking at all the examples of using SetMulticastInterface I saw this example on Google's own Golang docs
https://pkg.go.dev/golang.org/x/net/ipv4#example-RawConn-AdvertisingOSPFHello
I believe it comes down to ControlMessage only being implemented on linux/iOS/darwin mac based platforms and none others. It didn't make sense to specifically call out windows. See https://pkg.go.dev/golang.org/x/net/ipv4#PacketConn.WriteTo
marten-seemann
approved these changes
Aug 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding to @davidflowerday's PR I've made this platform specific so windows/others uses SetMulticastInterface and linux/darwin uses ControlMessage, I've also updated server.go to include a fix for this problem as well.
I was having issues getting this library to even work on windows, only mDNS services I registered on localhost would show up in my browse/lookup requests, after digging in I found @davidflowerday's PR and a few other comments on other forks.
I've tests this fix on Windows and and linux (WSL) and both seem to be in good shape, I can resolve and register mDNS service both on and off box and see them. I do not have a mac ( Intel or Arm ) to test this on unfortunately.
from @davidflowerday's PR:
I'm attempting to use go-chromecast on Windows which leverages this library. Unfortunately, mDNS wasn't working. I was able to fix it by replacing the code that used ControlMessage in WriteTo() with a call to SetMulticastInterface() instead. I think this may be related to issues grandcat#54 grandcat#69 and grandcat#75. I've tested this change on Windows and Linux and I will test on macOS shortly as well. I only changed client.go but it's possible a similar change is needed in server.go as well.
In the docs for the ipv4 package under Bugs there is this note:
On Windows, the ControlMessage for ReadFrom and WriteTo methods of PacketConn is not implemented.
I've also submitted this as a PR to the upstream grandcat/zerconf though I'm less hopeful it'll be accepted there. Your fork here seems to be more active! which is why I also submitted it here.
Additionally, the ipv4 docs on Multicasting here show an example using SetMulticastInterface() as I've done in this PR which suggests to me that this is an OK way to handle this issue, but if you'd prefer something else I'd be happy to change it.
Thanks for making zeroconf and for considering this PR.