Skip to content

fix: cmake windows xp build#5662

Closed
matthiasng wants to merge 1 commit into
curl:masterfrom
matthiasng:fix/cmake-windows-xp-build
Closed

fix: cmake windows xp build#5662
matthiasng wants to merge 1 commit into
curl:masterfrom
matthiasng:fix/cmake-windows-xp-build

Conversation

@matthiasng

@matthiasng matthiasng commented Jul 8, 2020

Copy link
Copy Markdown

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

matthiasng commented Jul 10, 2020

Copy link
Copy Markdown
Author

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

bagder commented Jul 12, 2020

Copy link
Copy Markdown
Member

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.

4 participants