bpo-23451: Fix socket deprecation warnings in socketmodule.c #2318
Conversation
segevfiner
changed the title
segevfiner
changed the title
…`/`WSAStringToAddress`
|
Well I went ahead and used Were now only left with: And those are all from wrapper functions of the "deprecated" functions. So the only thing left to do is to define |
There was a problem hiding this comment.
This looks good, two concerns:
- can you try some of the networking libraries test suites (e.g. Twisted, requests, etc.)? I would hope that the stdlib socket tests are sufficient, but I'm not confident
- rather than adding the preprocessor definition to suppress warnings, can you try
#pragma warningfirst? That way we won't (shouldn't) accidentally introduce new uses of these functions elsewhere.
PCbuild/_socket.vcxproj
Outdated
| @@ -60,6 +60,9 @@ | |||
| <_ProjectFileVersion>10.0.30319.1</_ProjectFileVersion> | |||
| </PropertyGroup> | |||
| <ItemDefinitionGroup> | |||
| <ClCompile> | |||
| <PreprocessorDefinitions>HAVE_INET_PTON;%(PreprocessorDefinitions)</PreprocessorDefinitions> | |||
There was a problem hiding this comment.
This can be defined in PC/pyconfig.h rather than in the project file.
|
@zooba Doing stuff like: #ifdef _MSC_VER
#pragma warning(push)
#pragma warning(disable: 4996)
#endif
h = gethostbyname(name);
#ifdef _MSC_VER
#pragma warning(pop)
#endifIs a bit ugly... Especially in functions already ridden with preprocessor stuff. VS2015 does support There is #define MSVC_WARNING_PUSH __pragma(warning(push))
#define MSVC_WARNING_DISABLE(warns) __pragma(warning(disable: warns))
#define MSVC_WARNING_POP __pragma(warning(pop))
#define MSVC_WARNING_SUPPRESS(warns) __pragma(warning(suppress: warns))Which can make this prettier, but I'm not sure if it's right to add something like this or where to add it to CPython's sources. (pyport.h?) |
|
The requests test suite passed. I failed to run Twisted's because it uses tox/virtualenv which explodes when invoked against a built Python source tree (Can't find Do note that I don't have an IPv6 network (IPv6 DNS does work here though). So hopefully they are not skipping those tests 😛 |
|
You should be able to run As for the warnings, you can probably do something like this at the top of this source file: #if MS_WINDOWS
#define BEGIN_ALLOW_DEPRECATED_CALL __pragma(warning(push))\
__pragma(warning(disable: 4996))
#define END_ALLOW_DEPRECATED_CALL __pragma(warning(pop))
#else
#define BEGIN_ALLOW_DEPRECATED_CALL
#define END_ALLOW_DEPRECATED_CALL
#end
There might even be something useful for the |
|
@zooba Oh my... I got hit by unrelated issues trying to do this... 😣
Had to manually modify the virtualenv created by Tox to workaround these. BTW, I also found I got the following (minus SKIPPED tests): None of them seem related to this change. |
|
@segevfiner Yeouch, those aren't good. The random corruption does concern me a little bit - I don't want to presume what's related to the change here. If they occur without the fix, then let me know and I'd say we're done. Thanks for working through these! It's much appreciated :) |
|
@zooba I get those same failures with a build from master. |
|
Great. The last thing needed is the NEWS entry, and it looks like you'll get to be one of the first to use the new system :) See https://docs.python.org/devguide/committing.html#what-s-new-and-news-entries for info - it's basically just creating a single file in the Windows directory with the title of this bug (and add "Patch by " to ensure you get proper credit). |
|
I fixed the conflict, and if the checks pass then nothing left to do and I'll merge. Thanks! |
|
I'll try, but anyone can feel free to keep an eye on http://buildbot.python.org/all/waterfall?category=3.x.stable to see if any builds have issues as a result of this (or if the warnings still appear - we should have significantly less thanks to this patch). |
We are getting this warnings:
I fixed the
WSASocket,WSADuplicateSocketwarnings by switching to the wide versions. And I added a backwards compatibility note about it. I also fixedWSAAddressToStringA/WSAStringToAddressAby usingwchar_t/Py_UNICODE.inet_ntoa/inet_addris probably okay insocket_inet_ntoa/socket_inet_aton, but I'm not sure about this one: Modules/socketmodule.c:1067, since it does seem like Windows hasinet_ptonsince Windows Vista: InetPton function (Windows). But it wasn't used for implementingsocket_inet_ntop/socket_inet_pton... probably to support older Windows versions. You can also see a Windows specific partialinet_ntop/inet_ptonimplementation in there, which I don't think is used anymore.We probably don't want to fix
gethostbyname/gethostbyaddrand just ignore it by defining_WINSOCK_DEPRECATED_NO_WARNINGS. I haven't defined it yet so that you can see any remaining warnings. But it should probably be added before this is merged.The remaining warnings are:
EDIT: How should the
inet_pton/inet_ntopwarnings be handled? Should we go with killing the legacy stuff and theWSAAddressToString/WSAStringToAddressbasedsocket_inet_ntop/socket_inet_pton, and definingHAVE_INET_PTON? or something else? I haven't tested yet whether that actually works... and I don't know what differences in behavior this might cause if any.cc @zware @zooba