[WIP] Include IPv6 addresses for getipaddr and getipaddrs #30609
Conversation
|
As mentioned at #30604: this is not the desired solution. There should instead be a single multi-protocol function |
|
Right, sorry, I got busy and didn't have a chance to address the updates in that issue. I'll mark this as WIP. |
jpsamaroo
changed the title
jpsamaroo
changed the title
|
Having issues testing this locally, so I'm gonna rely on CI to test it for me. I'll rebase and clean up old code once we're done here. |
jpsamaroo
changed the title
|
As far as I can tell, the OSX build error is just a network error, unrelated to this PR. |
|
Not sure how to request a review, so @StefanKarpinski do you like the approach I took here? If so, I'll cleanup and squash. |
|
I do like the overall direction. However, I think this:
Should be " Since |
|
I updated that same line to indicate that it returns IPv4 sorted first, hopefully the language is OK. |
stdlib/Sockets/src/addrinfo.jl
Outdated
| Get an IP address of the local machine, preferring IPv4 over IPv6. Throws if no | ||
| addresses are available. | ||
|
|
||
| getipaddr(addr_type::Type{T}) where T<:IPAddr -> IPAddr |
There was a problem hiding this comment.
| getipaddr(addr_type::Type{T}) where T<:IPAddr -> IPAddr | |
| getipaddr(addr_type::Type{T}) where T<:IPAddr -> T |
stdlib/Sockets/src/addrinfo.jl
Outdated
| end | ||
| end | ||
| ccall(:uv_free_interface_addresses, Cvoid, (Ptr{UInt8}, Int32), addr, count) | ||
| return lo_present ? localhost : error("No networking interface available") | ||
| =# |
There was a problem hiding this comment.
This commented out code should just be deleted rather than commented out.
There was a problem hiding this comment.
Agreed, was just keeping it around so that I don't have to fish it out of git again if I found I needed it 😄
|
Circling back to this since I just remembered this existed. Is there anything else I've missed in this PR that should be addressed? |
|
Have marked for triage to discuss if the API is good, if everyone is ok with it we’ll merge. |
|
Triage is in favor. One suggestion (see comments): make |
|
Ok, sounds good. Has someone marked the extraneous extra curly braces that I should remove? Or is that addressed by current reviews? Also, squash to single commit, or is some other split desired? |
|
I don't think there's curlies that need changing. Squash to a single commit, please, although that can be done when this is merged too but since you're rebasing anyway... |
jpsamaroo
force-pushed
the
getipaddrs-ipv6
branch
from
78cceb4 to
95362a0
Compare
|
I'm not sure what's up with the mixed bag of failures, but it seems like this is passing in general. Anyway, let me know if there was anything else I should do! |
|
@staticfloat, are the failures here expected? |
|
Appveyor and Travis I cant speak for, but the Mac buildbot is a failure that’s already fixed on master, so you just need to rebase on top to get that sorted. |
jpsamaroo
force-pushed
the
getipaddrs-ipv6
branch
from
95362a0 to
4e9697e
Compare
|
Everything seems mostly green, but did my changes actually manage to hang MacOS bootstrap? |
|
That timeout looks unrelated to me. @staticfloat, can I get a professional opinion? |
|
Yeah no that's weird. I restarted it just in case. |
|
Looks like a mysterious failure. The rebuild made it through bootstrap, so you've got the 👍 from me |
|
Ok, so I'm glad we got this merged, but after playing around with it for a bit (which I did before too but it didn't hit me until now), I realize that the boolean argument should be a keyword. I would call it |
|
Also, this fails tests locally for me on macOS. |
|
However, it doesn't seem to be any worse than it was before this commit, so it's a separate issue. |
Fixes #30604
I chose the path of least breakage here, so that
getipaddr()isn't changed. I'm happy to take another approach to the API if desired.