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

fix: cmake windows xp build #5662

Closed

Conversation

matthiasng
Copy link

@matthiasng matthiasng commented Jul 8, 2020

The following line always evaluates to true if you compile cmake on a system with inet_pton. This lead to no way to build curl on a newer system for Windows XP.

check_symbol_exists(inet_pton      "${CURL_INCLUDES}" HAVE_INET_PTON)

inet_pton is in WS2tcpip.h enclosed by #if (_WIN32_WINNT >= 0x0600). So we have to set CMAKE_REQUIRED_DEFINITIONS that check_symbol_exists(inet_pton ...) uses the right define.

@jay jay added the cmake label Jul 9, 2020
@bagder bagder requested a review from MarcelRaad July 9, 2020 16:32
@MarcelRaad
Copy link
Member

Thanks, makes sense!

I'm now trying to find out why it worked for me when testing be578ee. I assumed that check_symbol_exists respects COMPILE_DEFINITIONS. I'm pretty sure I tested both post and pre Windows Vista cases with Visual Studio as well as MinGW. 😕

@matthiasng
Copy link
Author

matthiasng commented Jul 10, 2020

Maybe you stumbled over another little bug i encountered yesterday.

CMake only runs check_symbol_exists at configuration time.
This results in the following behaviour:

  1. First run (without variables)
    • check_symbol_exists will set HAVE_INET_PTON to 1
  2. Change ENABLE_INET_PTON or CURL_TARGET_WINDOWS_VERSION and rerun cmake
    • HAVE_INET_PTON stays 1 (cached)

Maybe this can be solve with using a temporary variable for check_symbol_exists and an additional if to set HAVE_INET_PTON depending on the result of check_symbol_exists, ENABLE_INET_PTON and CURL_TARGET_WINDOWS_VERSION.

@bagder bagder closed this in 4ef16f1 Jul 12, 2020
@bagder
Copy link
Member

bagder commented Jul 12, 2020

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

4 participants