-
Notifications
You must be signed in to change notification settings - Fork 54
macos port: use native interfaces to find default network interface #6925
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
base: master
Are you sure you want to change the base?
Conversation
9a18911
to
524ca5e
Compare
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 seems like a nice improvement to me!
It turns out this is not reliable in some cases, where no interface with a blank netmask exists but a default still does, working on a fix. |
problem: MacOS/BSD lack a linux-compatible proc (usually) so we can't read routes from proc solution: On linux we'll continue to use /proc/net/route, while on macos it turns out you can have a default interface *that does not have a netmask of zero*. To get it reliably, I've added a version of `getprimary_interface4` that uses the CoreFoundation and SystemConfiguration frameworks to request the global IPv4 default interface name from macos directly. This is more macos specific than using a PF_ROUTE socket, but much _much_ simpler. If we want this to work on other BSDs (aside from FreeBSD with linux compat turned on) we'll want to do that but NETLINK/PF_ROUTE are really arcane and rather high overhead for something this simple. Anyway, this is using the classic C interface to CoreFoundation rather than any of the add-ons so there's no need to compile code as Objective-C or anything. As to why I switched to this from the getifaddrs approach, that seemed to work fine on linux with a small tweak, but not on macos, and if it doesn't work reliably cross-platform it seems better to keep what we had battle tested and add the more specific mac version like this.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6925 +/- ##
==========================================
- Coverage 83.91% 83.91% -0.01%
==========================================
Files 540 540
Lines 90526 90526
==========================================
- Hits 75967 75963 -4
- Misses 14559 14563 +4
🚀 New features to boost your workflow:
|
There's a failure happening on both PRs related to this from the fileref test. I haven't been able to reproduce it on a local mac, and it doesn't seem to have cropped up before. @garlick have you seen this? It looks like a symlink test failure, but there's no related output from GHA to see what actually failed other than that. |
Does the tmate action work on the macos builder? I've used that in the past to debug issues that annoyingly only happen in CI. I just usually temporarily add the action on failure. (I Ikeep wondering if there's a way to permanently add that action and only trigger it via a "debug" rebuild) |
That is an excellent thought. I don't know, but I will find out. |
Apologies, I made a comment on this stuff in the other PR that is based on top of it. Just that I'm traveling today and wanted to have a closer look at this before it goes in. Great work @trws. |
I decided to try out the MacOS build at the end of last week, and noticed one bit of low-hanging fruit. The
getprimary_iface4
function was failing, and I guessed that could cause a whole lot of downstream failures.The commit message has much more detail, but I ended up completely reworking the approach with roughly the same result.