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.

Sign up for GitHub

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

Jump to bottom

cmake: do not force Windows target versions #9046

Closed
wants to merge 1 commit into from
Closed

cmake: do not force Windows target versions #9046

wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jun 24, 2022 •
edited

The goal of this patch is to avoid CMake forcing specific Windows
versions and rely on toolchain defaults or manual selection instead.
This gives back control to the user. This also brings CMake closer to
how autotools and Makefile.m32 behaves in this regard.

  • CMake had a setting ENABLE_INET_PTON defaulting to ON, which did
    nothing else than fixing the Windows build target to Vista. This also
    happened when the toolchain did not have Vista support (e.g. original
    MinGW), breaking such builds.

    In other environments it did not make a user-facing difference,
    because libcurl has its own pton() implementation, so it works well
    with or without Vista's inet_pton().

    This patch drops this setting. inet_pton() is now used whenever
    building for Vista or newer, either when requested manually or by
    default with modern toolchains (e.g. mingw-w64). Older envs will fall
    back to curl's pton().

    Ref: windows: improve random source #9027 (comment)
    Ref: lib/easy_lock.h: include synchapi.h on Windows #8997 (comment)

  • When the user did no select a Windows target version manually, stop
    explicitly targeting Windows XP, and instead use the toolchain default.

    This may pose an issue with old toolchains defaulting to pre-XP
    targets. In such case you must manually target Windows XP via:
    -DCURL_TARGET_WINDOWS_VERSION=0x0501
    or
    -DCMAKE_C_FLAGS=-D_WIN32_WINNT=0x0501

Closes #xxxx

@vszakats vszakats added cmake Windows Windows-specific labels Jun 24, 2022
@vszakats vszakats changed the title cmake: do not force bump build target to Vista cmake: improve control over the targeted Windows version Jun 24, 2022
@vszakats
Copy link
Member Author

vszakats commented Jun 24, 2022 •
edited

This PR deprecates CURL_TARGET_WINDOWS_VERSION. It was introduced in January 2021. It's a CMake- and curl-specific setting for something that seems relatively straightforward to specify in a universal way by passing -D_WIN32_WINNT=. As a CMake-noob, it is possible I'm missing certain merits/aspects of the dedicated-setting approach. It's also not a required change to reach the goals of the PR (in its current form.)

I'm happy to discuss what to do with this option. Keep it as-is, leave it deprecated, delete it?

@vszakats vszakats changed the title cmake: improve control over the targeted Windows version cmake: do not force Windows target versions Jun 24, 2022
@vszakats vszakats force-pushed the cmakenovista branch 2 times, most recently from 093fae5 to 644b784 Compare June 24, 2022 14:04
@MarcelRaad
Copy link
Member

MarcelRaad commented Jun 24, 2022 •
edited

I'm not very familiar with this aspect of CMake, but several other projects I use provide a way to add compiler or preprocessor options without overriding the defaults via -DCMAKE_C(XX)_FLAGS. IIUC, that's required for respecting what's set via environment variables and toolchain files.

If that assumption is wrong or there's a different way to do that, I'm all for deleting the option. Then I'm probably the only one using it anyway 🙂

@vszakats
Copy link
Member Author

vszakats commented Jun 24, 2022 •
edited

@MarcelRaad Thanks for your feedback. If this brings things closer to the CMake-way, we can certainly keep it.

I haven't tried it, but CMake says to also support CFLAGS envvar, so CFLAGS=-D_WIN32_WINNT=... might also work. [UPDATE: Could not make it work, export CFLAGS= appears to be ignored.]

@jzakrzewski
Copy link
Contributor

Projects tend to provide dedicated options for aspects that they deem "frequently used" or "overly complicated to setup otherwise". So the question would be if this setting falls into one of those categories, I guess. I for sure don't know ;)

@vszakats
cmake: do not force Windows target versions
7b4d1cd 
The goal of this patch is to avoid CMake forcing specific Windows
versions and rely on toolchain defaults or manual selection instead.
This gives back control to the user. This also brings CMake closer to
how autotools and `Makefile.m32` behaves in this regard.

- CMake had a setting `ENABLE_INET_PTON` defaulting to `ON`, which did
  nothing else than fixing the Windows build target to Vista. This also
  happened when the toolchain did not have Vista support (e.g. original
  MinGW), breaking such builds.

  In other environments it did not make a user-facing difference,
  because libcurl has its own pton() implementation, so it works well
  with or without Vista's inet_pton().

  This patch drops this setting. inet_pton() is now used whenever
  building for Vista or newer, either when requested manually or by
  default with modern toolchains (e.g. mingw-w64). Older envs will fall
  back to curl's pton().

  Ref: curl#9027 (comment)
  Ref: curl#8997 (comment)

- When the user did no select a Windows target version manually, stop
  explicitly targeting Windows XP, and instead use the toolchain default.

  This may pose an issue with old toolchains defaulting to pre-XP
  targets. In such case you must manually target Windows XP via:
    `-DCURL_TARGET_WINDOWS_VERSION=0x0501`
  or
    `-DCMAKE_C_FLAGS=-D_WIN32_WINNT=0x0501`

Closes #xxxx
@vszakats
Copy link
Member Author

Thanks for the inputs. I deleted the deprecation part. It's nice to make passing this setting a little bit friendlier.

jay
jay approved these changes Jun 27, 2022
View reviewed changes
Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

I would save this for the next feature window which is in several days barring any issues worthy of a patch release

@vszakats vszakats added the feature-window A merge of this requires an open feature window label Jun 27, 2022
vszakats added a commit to curl/curl-for-win that referenced this pull request Jun 27, 2022
@vszakats
curl-cmake.sh: apply upstream PR and adapt build config [ci skip]
53cba6c 
Sync up OS target selection settings with autotools and m32.

Ref: curl/curl#9046
@vszakats vszakats closed this in 8ba22ff Jul 4, 2022
@vszakats vszakats deleted the cmakenovista branch July 4, 2022 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jay jay jay approved these changes

@MarcelRaad MarcelRaad MarcelRaad approved these changes

Assignees
No one assigned
Labels
cmake feature-window A merge of this requires an open feature window Windows Windows-specific
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vszakats @MarcelRaad @jzakrzewski @jay