_socket.py: fix for systems where AI_NUMERICSERV is not defined #3135
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3135 +/- ##
===============================================
Coverage 100.00000% 100.00000%
===============================================
Files 124 124
Lines 18779 18780 +1
Branches 1269 1269
===============================================
+ Hits 18779 18780 +1
🚀 New features to boost your workflow:
|
|
@CoolCat467 That one CI which fails is unrelated, right? |
Docs are failing because your newsfragment is using markdown not rST. Try doing It's a bit confusing because the logs show two warnings, but the one you should care about is: I don't think the other one makes it fail, though I may be wrong. |
barracuda156
force-pushed
the
AI_NUMERICSERV
branch
from
641c0df to
36a3de8
Compare
barracuda156
force-pushed
the
AI_NUMERICSERV
branch
from
36a3de8 to
5260922
Compare
src/trio/_socket.py
Outdated
| # AI_NUMERICSERV may be missing on some older platforms, so use it when available. | ||
| _NUMERIC_ONLY = _stdlib_socket.AI_NUMERICHOST | ||
| _NUMERIC_ONLY |= getattr(_stdlib_socket, "AI_NUMERICSERV", 0) |
There was a problem hiding this comment.
I think you need to be a bit more specific here, otherwise future people that stumble on this comment will have to do a bunch of research to figure out why/when it might be missing. And as I said in the issue, perhaps even a note on why it's needed despite being deprecated since a decade+
There was a problem hiding this comment.
Maybe just linking the issue will work? That has info.
There was a problem hiding this comment.
@barracuda156 could you fix this? I think this is the only problem before merging.
(well, barring rejecting this whole change on basis of it being something we cannot test for a system we could not support. IMO this change is fine but @jakkdl didn't respond to the response you left on the issue.)
There was a problem hiding this comment.
Sorry, this got missed.
I will return to this tomorrow.
There was a problem hiding this comment.
Added a link now to the issue into the comment.
|
Bump @barracuda156 |
barracuda156
force-pushed
the
AI_NUMERICSERV
branch
from
5260922 to
3f7b30e
Compare
| @@ -0,0 +1,2 @@ | |||
| Fixed socket module for some older systems which lack `socket.AI_NUMERICSERV`. | |||
There was a problem hiding this comment.
Oops CI is still complaining about being unable to find socket.AI_NUMERICSERV. Try this instead:
| Fixed socket module for some older systems which lack `socket.AI_NUMERICSERV`. | |
| Fixed socket module for some older systems which lack ``socket.AI_NUMERICSERV``. |
Rationale: on some platforms
AI_NUMERICSERVmay not be defined. In particular, old versions of macOS, possibly some other. While affected users are probably few, it is a trivial fix, which makestriousable (for example,yewtubeapp works then).Credits to @CoolCat467
Closes: #3133