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

cmake: improve detection of threadsafe feature #9312

Closed
wants to merge 1 commit into from

Conversation

mback2k
Copy link
Member

@mback2k mback2k commented Aug 14, 2022

Avoids failing test 1014 by replicating configure checks
for HAVE_ATOMIC and _WIN32_WINNT with custom CMake tests.

Follow up to #8680 and #8997

@mback2k mback2k added the cmake label Aug 14, 2022
@mback2k mback2k self-assigned this Aug 14, 2022
@mback2k mback2k force-pushed the improve-cmake-threadsafe branch 2 times, most recently from 31beaaa to e17de80 Compare Aug 14, 2022
Copy link
Member

@MarcelRaad MarcelRaad left a comment

👍

@mback2k mback2k added the Windows Windows-specific label Aug 14, 2022
@mback2k mback2k force-pushed the improve-cmake-threadsafe branch 2 times, most recently from 45204c0 to a1f8843 Compare Aug 21, 2022
@mback2k
Copy link
Member Author

mback2k commented Aug 21, 2022

@MarcelRaad with the current state of this PR I almost get all AppVeyor builds to work again regarding test 1014, but one is still failing and quite weird. My new CMake _WIN32_WINNT detection test returns 0x0501 for it, but later during the compilation of easy_lock.h the actual value is 0x0601 and I am not sure where this is coming from. Do you have any idea?

@mback2k mback2k requested a review from MarcelRaad Aug 21, 2022
@MarcelRaad
Copy link
Member

MarcelRaad commented Aug 22, 2022

Hmm, I don't see it on first glance either. This is Visual Studio 2010 with the Windows 7 SDK, which defaults to 0x0601, so that should be correct. I'll take another look later.

@mback2k mback2k force-pushed the improve-cmake-threadsafe branch from e6737f4 to c940724 Compare Aug 23, 2022
@mback2k
Copy link
Member Author

mback2k commented Aug 23, 2022

@MarcelRaad I was missing the include of sdkddkver.h in CurlTests.c. Now the Windows part is working correctly.

@MarcelRaad
Copy link
Member

MarcelRaad commented Aug 23, 2022

But you had the include of windows.h, which should include sdkddkver.h itself, shouldn't it? And even if it didn't, nothing should define _WIN32_WINNT to 0x0501. 😕 Whatever, glad that it works now and thanks for letting me know!

Still/again looks good to me. Only are the includes of curl_setup.h and sdkddkver.h both needed? I thought that curl_setup.h was needed because the include of windows.h, and ultimately sdkddkver.h, was expected to happen through that (in setup-win32.h).

@mback2k
Copy link
Member Author

mback2k commented Aug 23, 2022

sdkddkver.h seems not to be automatically included by windows.h, so I am manually including sdkddkver.h first now to set the defaults based on the SDK environment. Then I include curl_setup.h to have the same behavior as an actual build would hopefully have, with all the includes that you mentioned.

@MarcelRaad
Copy link
Member

MarcelRaad commented Aug 23, 2022

Strange. I checked several Windows SDK versions I have installed and one of the first things all of them do is unconditionally include <sdkddkver.h>, typically right after including <winapifamily.h> for recent versions. 😕

@mback2k
Copy link
Member Author

mback2k commented Aug 23, 2022

And even if it didn't, nothing should define _WIN32_WINNT to 0x0501. 😕

I found the source of that definition:

curl/lib/config-win32.h

Lines 393 to 453 in 3f98eaa

/* Define some minimum and default build targets for Visual Studio */
#if defined(_MSC_VER)
/* Officially, Microsoft's Windows SDK versions 6.X does not support Windows
2000 as a supported build target. VS2008 default installations provides
an embedded Windows SDK v6.0A along with the claim that Windows 2000 is a
valid build target for VS2008. Popular belief is that binaries built with
VS2008 using Windows SDK versions v6.X and Windows 2000 as a build target
are functional. */
# define VS2008_MIN_TARGET 0x0500
/* The minimum build target for VS2012 is Vista unless Update 1 is installed
and the v110_xp toolset is chosen. */
# if defined(_USING_V110_SDK71_)
# define VS2012_MIN_TARGET 0x0501
# else
# define VS2012_MIN_TARGET 0x0600
# endif
/* VS2008 default build target is Windows Vista. We override default target
to be Windows XP. */
# define VS2008_DEF_TARGET 0x0501
/* VS2012 default build target is Windows Vista unless Update 1 is installed
and the v110_xp toolset is chosen. */
# if defined(_USING_V110_SDK71_)
# define VS2012_DEF_TARGET 0x0501
# else
# define VS2012_DEF_TARGET 0x0600
# endif
#endif
/* VS2008 default target settings and minimum build target check. */
#if defined(_MSC_VER) && (_MSC_VER >= 1500) && (_MSC_VER <= 1600)
# ifndef _WIN32_WINNT
# define _WIN32_WINNT VS2008_DEF_TARGET
# endif
# ifndef WINVER
# define WINVER VS2008_DEF_TARGET
# endif
# if (_WIN32_WINNT < VS2008_MIN_TARGET) || (WINVER < VS2008_MIN_TARGET)
# error VS2008 does not support Windows build targets prior to Windows 2000
# endif
#endif
/* VS2012 default target settings and minimum build target check. */
#if defined(_MSC_VER) && (_MSC_VER >= 1700)
# ifndef _WIN32_WINNT
# define _WIN32_WINNT VS2012_DEF_TARGET
# endif
# ifndef WINVER
# define WINVER VS2012_DEF_TARGET
# endif
# if (_WIN32_WINNT < VS2012_MIN_TARGET) || (WINVER < VS2012_MIN_TARGET)
# if defined(_USING_V110_SDK71_)
# error VS2012 does not support Windows build targets prior to Windows XP
# else
# error VS2012 does not support Windows build targets prior to Windows \
Vista
# endif
# endif
#endif

So, if curl_setup.h is included before windows.h or sdkddkver.h, then _WIN32_WINNT is set based on _MSC_VER.

Now I am wondering why this behavior is not always affecting us since curl_setup.h should always be the first include, right?

@MarcelRaad
Copy link
Member

MarcelRaad commented Aug 23, 2022

Wow. config-win32.h is for non-CMake builds, but HAVE_CONFIG_H was not set yet for that CMake check, right? That absolutely makes sense then as setup-win32.h, which includes windows.h & friends, is only included later.

Edit: so what happens is:

  1. curl_setup.h is included
  2. as HAVE_CONFIG_H is not set during CMake configure time, unlike the actual build later, config-win32.h gets included
  3. config-win32.h sets _WIN32_WINNT to 0x0501
  4. only then setup-win32.h gets included, which includes the Windows SDK headers

@MarcelRaad
Copy link
Member

MarcelRaad commented Aug 23, 2022

Maybe we should include setup-win32.h directly then to avoid pulling in config-win32.h? Then we should have the same behavior as libcurl itself in regards to Windows SDK includes and definitions and don't need any additional includes.

@mback2k
Copy link
Member Author

mback2k commented Aug 24, 2022

Ok, so include setup-win32.h instead of windows.h or sdkddkver.h and curl_setup.h, right?

@MarcelRaad
Copy link
Member

MarcelRaad commented Aug 24, 2022

yes 👍

@mback2k
Copy link
Member Author

mback2k commented Aug 24, 2022

AppVeyor looks good now, thanks @MarcelRaad. Unfortunately I had to include some boilerplate in the test before including setup-win32.h to avoid compiler warnings and errors. Also I am not sure regarding the relevance of the other CI failures yet.

@MarcelRaad
Copy link
Member

MarcelRaad commented Aug 24, 2022

Ah, right, that stuff is still in curl_setup.h for the winapifamily.h include. I actually wanted to move that whole block to setup-win32.h, but that's not possible because of a circular dependency: config-win32.h needs the definition of CURL_WINDOWS_APP. That check needs _WIN32_WINNT, which is only reliably defined after the windows.h include in setup-win32.h. Which needs curl_config.h/config-win32.h again for the header availability macros. So probably the order is best as it is right now (first including winapifamily.h for CURL_WINDOWS_APP, then including curl_config.h/config-win32.h for the header availability macros, then including setup-win32.h for the other Windows SDK includes), as the user should define _WIN32_WINNT anyway when building for Windows.

But I think we could move the definition of WIN32_LEAN_AND_MEAN and NOGDI to setup-win32.h. AFAICS, the winapifamily.h needed for CURL_WINDOWS_APP is independent of those two macros.

@MarcelRaad
Copy link
Member

MarcelRaad commented Aug 24, 2022

Also I am not sure regarding the relevance of the other CI failures yet.

The macOS HTTP2 test failures show up in other PRs too.

@mback2k
Copy link
Member Author

mback2k commented Aug 24, 2022

But I think we could move the definition of WIN32_LEAN_AND_MEAN and NOGDI to setup-win32.h. AFAICS, the winapifamily.h needed for CURL_WINDOWS_APP is independent of those two macros.

Okay, but let us save that for after fixing AppVeyor for now to see if that would break other things. Then the test can be updated accordingly.

mback2k added a commit to mback2k/curl that referenced this issue Aug 25, 2022
Avoids failing test 1014 by replicating configure checks
for HAVE_ATOMIC and _WIN32_WINNT with custom CMake tests.

Reviewed-by: Marcel Raad

Follow up to curl#8680
Closes curl#9312
@mback2k mback2k force-pushed the improve-cmake-threadsafe branch from e961210 to 64c3770 Compare Aug 25, 2022
@mback2k mback2k marked this pull request as ready for review Aug 25, 2022
Avoids failing test 1014 by replicating configure checks
for HAVE_ATOMIC and _WIN32_WINNT with custom CMake tests.

Reviewed-by: Marcel Raad

Follow up to curl#8680
Closes curl#9312
@mback2k mback2k force-pushed the improve-cmake-threadsafe branch from 64c3770 to 0f90606 Compare Aug 26, 2022
@mback2k mback2k closed this in 109e973 Aug 26, 2022
mback2k added a commit to mback2k/curl that referenced this issue Aug 26, 2022
mback2k added a commit that referenced this issue Sep 5, 2022
CMake seems to be able to compare two hex values just fine.
Also make sure CURL_TARGET_WINDOWS_VERSION is respected.

Assisted-by: Marcel Raad
Reviewed-by: Viktor Szakats
Reported-by: Keitagit-kun on github

Follow up to #9312
Fixes #9406
Closes #9411
mback2k added a commit that referenced this issue Sep 6, 2022
Assisted-by: Jay Satiro
Reviewed-by: Marcel Raad

Follow up to #9312
Closes #9375
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Windows Windows-specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants