Skip to content
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 from

Conversation

bagder
Copy link
Member

@bagder 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
Copy link

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

lib/inet_pton.h Outdated
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Badger inet_pton() is declared in ws2tcpip.h

@bagder
Copy link
Member Author

bagder commented Jul 5, 2017

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

@bagder
Copy link
Member Author

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
Copy link

Coverage Status

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

@MarcelRaad
Copy link
Member

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
Copy link
Member Author

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
inet_pton() exists on Windows and gets used by our cmake builds. Make
sure the correct header file is included to avoid compiler warnings.
... and make sure inet_pton is always checked for when *not* using Windows,
which is a regression from 4fc6ebe.

Idea-by: Sergei Nikulov
@bagder
Copy link
Member Author

bagder commented Jul 5, 2017

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

Copy link
Member

@MarcelRaad MarcelRaad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MarcelRaad
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link

Coverage Status

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

... by checking the POLLIN define, as the header file checks don't work
on Windows.
@coveralls
Copy link

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 July 5, 2017 11:29
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants