Skip to content

Migrate ServerDiscovery into the SDK #47

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented May 10, 2025

Summary

Per: jellyfin/Swiftfin#1533

Moves the ServerDiscovery logic into the SDK instead of being in Swiftfin. Introduces a new dependency for the SDK in:

https://github.com/apple/swift-nio-transport-services

Questions

  1. I ran into this NIO issue. There is zero impact from those errors except for the "error" logs. I've resolved the "errors" by manually finding the IPv4/IPv6 with a get func and ifaddr. The downside of manually getting the IP is we only broadcast on one IP instead of binding to all interfaces. I would prefer we take the 0.0.0.0 & :: route instead of using these get funcs since that gives us a more robust approach and reduces the total code for this PR. The current version of this uses the get funcs but I'd be happy to change this if we are fine with those errors getting logged.
  2. When this was in Swiftfin, I was using the logger. I changed this to just print() the same messages to the terminal. I was considering os.log but I saw that required a version bump for MacOS. Whatever approach we want I can make changes. The log prints aren't necessary so worst case I can just delete all the prints.

@JPKribs JPKribs requested a review from LePips May 10, 2025 06:17
@JPKribs
Copy link
Member Author

JPKribs commented May 10, 2025

If you want to test, this branch is all configured ready to go. Currently pointed at my branch until we have our next release:

@JPKribs
Copy link
Member Author

JPKribs commented May 17, 2025

I think it would make sense to knock out #46 before this one. I'm happy where this one stands but I was able to do this same network discovery using Foundation and Network. I only used NIO since I thought I was going to use it for the WebSocket.

If we end up using my current version of the WebSocket where I don't use NIO, it would make more sense to me to redo this one but using non-NIO to avoid adding the dependency unnecessarily.

If we decide we want to use NIO for the WebSocket, we can keep this as-is but I think WebSocket should dictate NIO instead of this.

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

Successfully merging this pull request may close these issues.

1 participant