win: fix Universal Windows Platform build

[MarcelRaad]

As reported by @joycepg here: #820 (comment)

This fixes a merge error in commit 7f3df80 caused by commit 332e8d6.

Additionally, this disables Curl_verify_windows_version for Windows App
builds. There seems to be no way to determine the Windows version from
a UWP app. Neither GetVersion(Ex), nor VerifyVersionInfo, nor the
Version Helper functions are supported.

[jay]

We use Curl_verify_windows_version in more than one place, is there a point in having to guard it each time? If there is no way to get the OS version anymore (which seems ridiculous but true) can we just change that function to behave as if it's dealing with Windows 10 in the case of UWP, like

Curl_verify_windows_version()
{
#if defined(CURL_WINDOWS_APP)
assume windows 10 and make the comparison against that
#else
normal stuff
#endif
}

[MarcelRaad]

Unfortunately CURL_WINDOWS_APP can also mean Windows 8/8.1 "modern UI" apps. I guess the best we could do is always assume to be running on Windows version _WIN32_WINNT. But that wouldn't be entirely correct either as you can also run a Windows 8 app on Windows 10.

[bagder]

Can't we then just pretend that CURL_WINDOWS_APP is Windows 8 (or later) ? We don't have any code that checks for anything that recent anyway so 8 or 10 shouldn't make much difference in curl (right now), isn't that so?

[MarcelRaad]

@bagder You're right, everything starting from Vista is currently treated the same way (there is code in schannel.c that checks for at least Windows 8.1, but that code cannot be compiled for CURL_WINDOWS_APP anyway).

Thanks for your comments, @jay and @bagder ! I've updated the commit to let Curl_verify_windows_version assume we're always running on the target Windows version for CURL_WINDOWS_APP. This is the minimum version supported by the app, so for Windows 10 universal apps it's Windows 10 and for Windows 8/8.1 apps it's Windows 8/8.1.

I've tested the code (the only call to this function for Windows "modern" apps is currently from connect.c) in two test apps targeting Windows 8.0 and 10, respectively.

[joycepg]

should this pull request include a change to the MakefileBuild.vc to pass the correct value of _WIN32_WINNT? Currently it ends up defined but empty. That will make this line fail:
https://github.com/curl/curl/pull/1048/files#diff-de29f75999c1a69b9e75eccdfdc246c7R90

I notice the automated build uses CMake, and although you can't see the command for compiling files of the library, it certainly defines this explicitly for the tests:
https://ci.appveyor.com/project/curlorg/curl/build/7.50.0.1840/job/cr95qsvtky4lvp2y#L1524

Or is CMake the only way the Windows 10 build should be done? If so a note in the winbuild directory?

[jay]

Marcel thanks for your changes. I need to catch up with you here though because I'm unfamiliar with this. I can't find anything in MS documentation that says UWP works on less than Windows 10, but they say this:

A Universal Windows Platform (UWP) app is a Windows experience that is built upon the Universal Windows Platform (UWP), which was first introduced in Windows 8 as the Windows Runtime.

... and then in the CURL_WINDOWS_APP code it's only set if _WIN32_WINNT >= 0x0602. So what you are getting at is CURL_WINDOWS_APP could be RT instead of UWP, is that correct?

To Paul's point:

should this pull request include a change to the MakefileBuild.vc to pass the correct value of _WIN32_WINNT? Currently it ends up defined but empty.

CURL_WINDOWS_APP is only supposed to be defined if >= 0x602. However I think this raises an interesting point, if someone wants to build a libcurl DLL that targets Windows RT/UWP then logically they'd have to define CURL_WINDOWS_APP? How exactly does this work? Can that DLL still be used on desktops?

[MarcelRaad]

So what you are getting at is CURL_WINDOWS_APP could be RT instead of UWP, is that correct?

Yes, exactly. CURL_WINDOWS_APP is defined for all non-Desktop apps on Windows 8+, so it applies to WinRT apps in addition to Windows 10 UWP apps. It's also defined for Windows Phone 8 apps, which have roughly the same Windows API limitations, but I've never tried building for that.

However I think this raises an interesting point, if someone wants to build a libcurl DLL that targets Windows RT/UWP then logically they'd have to define CURL_WINDOWS_APP? How exactly does this work? Can that DLL still be used on desktops?

You mean a DLL that works for classic desktop applications as well as WinRT/UWP apps? I've never tried that, I've always compiled all external libraries for my target platform.

How I build libcurl for UWP/WinRT is add an empty UWP/WinRT project to the MSVC14 libcurl solution file, move all files over from the existing project file, and adjust the preprocessor definitions. I don't define CURL_WINDOWS_APP myself but let curl_setup.h detect it.

[MarcelRaad]

@joycepg _WIN32_WINNT should never be defined to empty, it should either be defined to a valid 2-byte integer value or not be defined at all, in which case libcurl's config_win32.h will pick 0x0600 (Windows Vista) for you. I've never looked at MakefileBuild.vc, I hope I can look at it tonight.

[joycepg]

My understanding of the WIN32 API availability:

• desktop apps (Windows 2000->Windows 10): no restrictions
• Windows Phone (7.x, 8.x): a small subset
  
• Windows 8.x "metro" apps, aka Windows Runtime: a very small subset. For example, even winsock is not allowed
• Windows 10 UWP, a subset

The point is that Windows 10 (UWP) has more WIN32 than Windows 8.x (Windows Runtime), and libcurl will never run on Windows Runtime because socket() is not allowed. So I think CURL_WINDOWS_APP should effectively mean "Windows 10"

As far as having a single .lib or .dll that works on both desktop and UWP apps, this is fine, as long as the library only calls the allowed subset of WIN32 for where it is linked to. For example, if I build a "desktop" version of libcurl using nmake with all defaults, then try to link that library into a UWP, it will fail. Conversely, I could build a library using the UWP subset that would also work in a desktop app, but I couldn't use WinSSL or SSPI for example.

There is also a new thing in Windows 10 called a Windows Runtime Component, which is also a DLL, but it is a COM object + some metadata. Not relevant to libcurl.

[MarcelRaad]

The point is that Windows 10 (UWP) has more WIN32 than Windows 8.x (Windows Runtime), and libcurl will never run on Windows Runtime because socket() is not allowed. So I think CURL_WINDOWS_APP should effectively mean "Windows 10"

WinSock is available for both Windows RT 8.1 and Windows Phone 8.1 apps when using the updated Windows 8.1 SDK from Visual Studio 2013 Update 3, and libcurl compiles fine with it:
https://blogs.windows.com/buildingapps/2014/10/13/winsock-and-more-open-source-for-your-windows-store-apps
https://msdn.microsoft.com/en-us/library/windows/desktop/ms740642(v=vs.85).aspx

[joycepg]

bother. I missed that sdk update.
you are correct, my apologies.

[MarcelRaad]

@joycepg

should this pull request include a change to the MakefileBuild.vc to pass the correct value of _WIN32_WINNT? Currently it ends up defined but empty. That will make this line fail:
https://github.com/curl/curl/pull/1048/files#diff-de29f75999c1a69b9e75eccdfdc246c7R90

Unfortunately I haven't managed to reproduce that. With your build instructions from #820 (comment), everything compiles fine for me when run from the VS2015 x64 x86 Cross Tools Command Prompt.

If I add

#if _WIN32_WINNT == 0x0600
#error  Vista
#else
#error not Vista
#endif

to Curl_verify_windows_version, then I get the expected error:
..\lib\system_win32.c(88): fatal error C1189: #error: Vista
So _WIN32_WINNT is just not defined at all until it gets defined in config_win32.h.

Perhaps there should be an option added to the nmake build to specify the target Windows version, but unfortunately I'm not familiar with nmake. The only thing I've ever built with nmake is OpenSSL.

[bagder]

I like how this moves the windows app check to the general version check so I'm all for this patch. I would however appreciate getting someone with more windows knowledge to +1 before we merge.

[jay]

I think there are potential problems if a DLL built for Windows apps is then used in Windows desktops. As far as I can tell that is possible, and there we wouldn't want to guess about the version.

[MarcelRaad]

@jay Sorry, I think I don't quite understand. Are you suggesting to determine at runtime if we're running in a Windows app or desktop application?
Unfortunately there's no way to get the actual Windows version in a Windows app DLL as you cannot even use GetProcAddress to get access to VerifyVersionInfo. It just doesn't compile.

[jay]

Unfortunately there's no way to get the actual Windows version in a Windows app DLL as you cannot even use GetProcAddress to get access to VerifyVersionInfo. It just doesn't compile.

Thanks for your patience, this is what I'm trying to figure out. What I want to handle is a scenario where someone builds a DLL for a Windows app and then runs that DLL on the desktop. In other words can you run a UWP libcurl DLL in Windows 7, even if you can't run your app in Windows 7?. Is that a non-issue? I'm unclear whether or not that could happen since it's only using a subset of the API. I just read 'How to: Use Existing C++ Code in a Universal Windows Platform App' and maybe I missed it but I don't see it said either way.

[MarcelRaad]

@jay Ah, I see, thanks. I don't think that's a big issue as _WIN32_WINNT (the minimum supported Windows version) gets automatically set to 8.0/8.1/10 when you create an app project.

Of course one could manually override it, but that's asking for trouble, especially as many replacements for Windows API functions forbidden in store/phone/universal apps are only available in Windows 8.0 and higher. So the code will most likely either not compile (when setting it too low) or not run (when setting it to Windows 8+ and running it on Windows 7).

I'll try if using a UWP DLL/lib in a desktop application even works and report back.

[MarcelRaad]

@jay The Windows 7 desktop application compiles without warnings, but doesn't even start on Windows 10:

The application was unable to start correctly (0xc000a200). Click OK to close the application.

0xc000a200 means STATUS_NOT_APPCONTAINER. So it seems like DLLs compiled for WinRT/UWP cannot be loaded from desktop applications.

[jay]

Please review push-access-guidelines, you can now commit this on your own. As seen in 'How to work with an issue' basically you will get the latest from upstream/master and then rebase your branch on it, before checking out your local copy of upstream/master (usually named 'master' branch) and then merging in your branch's changes fast-forward only.

Also you will need to edit (git commit --amend) your commit message to associate the PR. Since the request can be closed you can add a line that says 'Closes' or just a ref line and close it manually. Here is what I mean

...

Bug: https://github.com/curl/curl/pull/820#issuecomment-250889878
Reported-by: Paul Joyce

Closes https://github.com/curl/curl/pull/1048

or

...

Bug: https://github.com/curl/curl/pull/820#issuecomment-250889878
Reported-by: Paul Joyce

Ref: https://github.com/curl/curl/pull/1048

[MarcelRaad]

@jay Thank you very much for the review and those great guidelines! I hope I did it correctly.