cmake: improve detection of threadsafe feature #9312

mback2k · 2022-08-14T19:06:08Z

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

Follow up to #8680 and #8997

MarcelRaad

👍

mback2k · 2022-08-21T20:48:32Z

@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?

MarcelRaad · 2022-08-22T07:46:19Z

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 · 2022-08-23T19:54:18Z

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

MarcelRaad · 2022-08-23T20:17:37Z

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 · 2022-08-23T20:20:42Z

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 · 2022-08-23T20:26:45Z

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 · 2022-08-23T20:30:41Z

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 · 2022-08-23T20:40:30Z

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:

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

MarcelRaad · 2022-08-23T20:45:26Z

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 · 2022-08-24T05:55:17Z

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

MarcelRaad · 2022-08-24T07:33:12Z

yes 👍

mback2k · 2022-08-24T20:32:14Z

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 · 2022-08-24T21:25:11Z

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 · 2022-08-24T21:55:13Z

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 · 2022-08-24T22:08:29Z

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.

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

Suggested-by: Marcel Raad Follow up to curl#9312

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

Assisted-by: Jay Satiro Reviewed-by: Marcel Raad Follow up to #9312 Closes #9375

mback2k added the cmake label Aug 14, 2022

mback2k self-assigned this Aug 14, 2022

mback2k force-pushed the improve-cmake-threadsafe branch 2 times, most recently from 31beaaa to e17de80 Compare August 14, 2022 20:19

MarcelRaad approved these changes Aug 14, 2022

View reviewed changes

mback2k added the Windows Windows-specific label Aug 14, 2022

mback2k force-pushed the improve-cmake-threadsafe branch 2 times, most recently from 45204c0 to a1f8843 Compare August 21, 2022 20:12

mback2k requested a review from MarcelRaad August 21, 2022 20:49

mback2k force-pushed the improve-cmake-threadsafe branch from e6737f4 to c940724 Compare August 23, 2022 18:42

MarcelRaad approved these changes Aug 25, 2022

View reviewed changes

mback2k force-pushed the improve-cmake-threadsafe branch from e961210 to 64c3770 Compare August 25, 2022 21:43

mback2k marked this pull request as ready for review August 25, 2022 21:45

cmake: add detection of threadsafe feature

0f90606

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 force-pushed the improve-cmake-threadsafe branch from 64c3770 to 0f90606 Compare August 26, 2022 09:44

mback2k closed this in 109e973 Aug 26, 2022

mback2k added a commit to mback2k/curl that referenced this pull request Aug 26, 2022

lib: consolidate Win32 setup definitions in setup-win32.h

c3b4090

Suggested-by: Marcel Raad Follow up to curl#9312

mback2k mentioned this pull request Aug 26, 2022

lib: consolidate Win32 setup definitions in setup-win32.h #9375

Closed

bagder mentioned this pull request Aug 31, 2022

CMakeLists.txt : HAVE_WIN32_WINNT value detection and threadsafe #9406

Closed

mback2k added a commit that referenced this pull request Sep 6, 2022

lib: add required Win32 setup definitions in setup-win32.h

0c68e25

Assisted-by: Jay Satiro Reviewed-by: Marcel Raad Follow up to #9312 Closes #9375

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: improve detection of threadsafe feature #9312

cmake: improve detection of threadsafe feature #9312

mback2k commented Aug 14, 2022 •

edited

MarcelRaad left a comment

mback2k commented Aug 21, 2022

MarcelRaad commented Aug 22, 2022

mback2k commented Aug 23, 2022

MarcelRaad commented Aug 23, 2022 •

edited

mback2k commented Aug 23, 2022

MarcelRaad commented Aug 23, 2022

mback2k commented Aug 23, 2022 •

edited

MarcelRaad commented Aug 23, 2022 •

edited

MarcelRaad commented Aug 23, 2022

mback2k commented Aug 24, 2022

MarcelRaad commented Aug 24, 2022

mback2k commented Aug 24, 2022

MarcelRaad commented Aug 24, 2022 •

edited

MarcelRaad commented Aug 24, 2022

mback2k commented Aug 24, 2022

cmake: improve detection of threadsafe feature #9312

cmake: improve detection of threadsafe feature #9312

Conversation

mback2k commented Aug 14, 2022 • edited

MarcelRaad left a comment

Choose a reason for hiding this comment

mback2k commented Aug 21, 2022

MarcelRaad commented Aug 22, 2022

mback2k commented Aug 23, 2022

MarcelRaad commented Aug 23, 2022 • edited

mback2k commented Aug 23, 2022

MarcelRaad commented Aug 23, 2022

mback2k commented Aug 23, 2022 • edited

MarcelRaad commented Aug 23, 2022 • edited

MarcelRaad commented Aug 23, 2022

mback2k commented Aug 24, 2022

MarcelRaad commented Aug 24, 2022

mback2k commented Aug 24, 2022

MarcelRaad commented Aug 24, 2022 • edited

MarcelRaad commented Aug 24, 2022

mback2k commented Aug 24, 2022

mback2k commented Aug 14, 2022 •

edited

MarcelRaad commented Aug 23, 2022 •

edited

mback2k commented Aug 23, 2022 •

edited

MarcelRaad commented Aug 23, 2022 •

edited

MarcelRaad commented Aug 24, 2022 •

edited