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

inet_pton: fix include on windows to get prototype #1639

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@bagder
Member

bagder commented Jul 4, 2017

inet_pton() exists on Windows and gets used by our cmake builds. Make
sure the correct header file is included to avoid compiler warnings.

@bagder bagder added the build label Jul 4, 2017

@mention-bot

This comment has been minimized.

mention-bot commented Jul 4, 2017

@bagder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yangtse to be a potential reviewer.

@@ -29,6 +29,9 @@ int Curl_inet_pton(int, const char *, void *);
#ifdef HAVE_INET_PTON
#ifdef HAVE_ARPA_INET_H
#include <arpa/inet.h>
#elif defined(HAVE_WINSOCK2_H)
/* inet_pton() exists in Windows XP or later */
#include <winsock2.h>
#endif

This comment has been minimized.

@Jan-E

Jan-E Jul 5, 2017

Contributor

@badger inet_pton() is declared in ws2tcpip.h

@bagder bagder force-pushed the bagder/inet_pton-windows branch from c2f7973 to a2b4878 Jul 5, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jul 5, 2017

@Jan-E: thanks! Pushed update just now.

@bagder

This comment has been minimized.

Member

bagder commented Jul 5, 2017

Simply including ws2tcpip.h instead of winsock2.h didn't work either. I gave up and now provide an inet_pton() prototype directly in lib/inet_pton.h for windows builds using it. It finally fixes those warnings.

@coveralls

This comment has been minimized.

coveralls commented Jul 5, 2017

Coverage Status

Coverage decreased (-0.04%) to 74.4% when pulling be36f62 on bagder/inet_pton-windows into fa289ea on master.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 5, 2017

In my Windows 10 SDK and MinGW-w64's w32api, it's in ws2tcpip.h. I couldn't find it at all in the original MinGW's w32api. But it's only defined when targeting Windows Vista and later. Are you maybe using MinGW? It targets Windows 2000 (MinGW) / XP (MinGW-w64) by default.

Defining the prototype unconditionally might be dangerous as the program won't start on Windows XP and lower anymore even if compiled with the correct target Windows version.

@bagder

This comment has been minimized.

Member

bagder commented Jul 5, 2017

Well, the prototype isn't what will make curl use it, but it removes the warning here. In this case, cmake detects the function and sets HAVE_INET_PTON and then we'll use it in the code. Of course the output binary won't work on old windows.

Are you maybe using MinGW

No, look at the appveyor builds. They're all built with three different versions of MSVC.

As @snikulov pointed out on the mailing list , the warnings I couldn't get rid of is probably because -D_WIN32_WINNT=0x0501 is set unconditionally by the cmake scripts.

I'm not sure what the correct fix for that is...

@curl curl deleted a comment from coveralls Jul 5, 2017

@curl curl deleted a comment from coveralls Jul 5, 2017

bagder added some commits Jul 5, 2017

cmake: if inet_pton is used, bump _WIN32_WINNT
... and make sure inet_pton is always checked for when *not* using Windows,
which is a regression from 4fc6ebe.

Idea-by: Sergei Nikulov
inet_pton: fix include on windows to get prototype
inet_pton() exists on Windows and gets used by our cmake builds. Make
sure the correct header file is included to avoid compiler warnings.

@bagder bagder force-pushed the bagder/inet_pton-windows branch from be36f62 to a63d761 Jul 5, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jul 5, 2017

Pushed a new version now that bumps the lowest windows version required if ENABLE_INET_PTON is set.

@MarcelRaad

Ah, I just wanted to propose something like that too :-)

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 5, 2017

ENABLE_INET_PTON hasn't been released yet, has it? Not being familiar with CMake, as it's Windows-specific, maybe we should go the other way round and make the Windows target version configurable? Otherwise, it may get complicated if more functions only available in even later Windows versions are introduced.

@bagder

This comment has been minimized.

Member

bagder commented Jul 5, 2017

It's not been released, and even if it had, we're not that strict on keeping the build scripts "compatible".

But sure, I'm certainly not against that - it sounds like a more sensible approach. Windows is a platform where I'm not working myself so I limit my amount of fiddling there, relying on others... Feel like having a go at it?

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 5, 2017

Yes, if noone familiar with CMake volunteers, I'll try to do that. Might take a while though because I'm currently moving and have limited internet access.

@bagder

This comment has been minimized.

Member

bagder commented Jul 5, 2017

Okay, no worries. I think I'll still merge this PR once it looks okay so at least the current approach works better, and then we can move to a better approach in a second step.

@coveralls

This comment has been minimized.

coveralls commented Jul 5, 2017

Coverage Status

Coverage decreased (-0.06%) to 74.407% when pulling a63d761 on bagder/inet_pton-windows into da08c86 on master.

select.h: avoid macro redefinition harder
... by checking the POLLIN define, as the header file checks don't work
on Windows.
@coveralls

This comment has been minimized.

coveralls commented Jul 5, 2017

Coverage Status

Coverage decreased (-0.05%) to 74.411% when pulling 70f42c7 on bagder/inet_pton-windows into da08c86 on master.

@bagder bagder closed this in 21e0705 Jul 5, 2017

@bagder bagder deleted the bagder/inet_pton-windows branch Jul 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment