icculus / physfs

A portable, flexible file i/o abstraction.
https://icculus.org/physfs/
zlib License
560 stars 98 forks source link

physfs_platform_windows.c: Use newer APIs when permitted by _WIN32_WINNT #7

Closed past-due closed 3 years ago

past-due commented 3 years ago

Benefit from the newer APIs when possible, based on the definition of _WIN32_WINNT.

icculus commented 3 years ago

But the preprocessor definition just tells you what SDK you are compiling against, not what is available on the end-user's system.

This works with WinRT because the baseline OS guarantees lots of newer Win32 APIs (and deleted older ones, so sometimes you have no choice).

(I'm pretty sure this thing still works on Windows 95, though, and we can abandon that level of backwards compatibility. But generally if there's a newer API that adds value, we LoadLibrary/GetProcAddress to decide about it at runtime.)

past-due commented 3 years ago

From the Microsoft docs (linked above):

The preprocessor macros WINVER and _WIN32_WINNT specify the minimum operating system version your code supports.

The intention is that _WIN32_WINNT is defined to the minimum targeted version of Windows. As there are build toolchains that default this to, for example, 0x0600 (Windows Vista) or higher - even for Desktop apps - this will enable the new APIs to be used without runtime checks / the LoadLibrary/GetProcAddress dance (where the minimum target is already sufficient). Only the #else branch would benefit from runtime availability checks (which would be a nice additional change).

icculus commented 3 years ago

I guess this can't hurt, in the worst case (someone accidentally builds for a newer platform they don't otherwise require) we're still talking about losing ancient platforms, so I applied it. Thanks!

diath commented 10 months ago

This is an old issue but I'd like to point out that if you use a toolchain that compiles PhysFS on Windows 8 and newer, users can no longer launch the resulting application on Windows 7 (our use case is compiling PhysFS on a Github Runner that runs Windows 11, then linking our game against PhysFS and shipping it to a large audience of players, some of which are still on Windows 7, this is a minor change but alienates a subset of our players), so the initial concern raised was valid:

But the preprocessor definition just tells you what SDK you are compiling against, not what is available on the end-user's system.

Bacause they end up with a binary that cannot find the CreateFile2 function that is not available on older platforms:

The procedure entrypoint CreateFile2 could not be located in the kernel32.dll.

Would it be possible to add an override for _WIN32_WINNT in CMakeLists so that library users can build it without patching the source?

Updated: Actually I'm not sure if that's even a viable option, because if the library is built with a package manger such as vcpkg, we would still need to patch the CMakeFiles.txt file, so either way it requires manual patching to support older platforms, so maybe nevermind the issue.

icculus commented 10 months ago

Ugh, I didn't realize one of these comments said "Windows 8+" instead of "Windows XP+"

I'm reverting this.

sezero commented 10 months ago

I'm reverting this.

That'd be good, yes

past-due commented 10 months ago

This is an old issue but I'd like to point out that if you use a toolchain that compiles PhysFS on Windows 8 and newer, users can no longer launch the resulting application on Windows 7 (our use case is compiling PhysFS on a Github Runner that runs Windows 11, then linking our game against PhysFS and shipping it to a large audience of players, some of which are still on Windows 7, this is a minor change but alienates a subset of our players), so the initial concern raised was valid:

@diath

It sounds like you are using a toolchain or settings that are defaulting to a minimum target Windows version of 8 or greater, and you aren't configuring it yourself.

The fix is to properly - and explicitly - configure your minimum target Windows deployment target, as documented by Microsoft.

This makes your minimum deployment target explicit, and no longer relies on the defaults of your particular combination of environment and toolchain.

Bacause they end up with a binary that cannot find the CreateFile2 function that is not available on older platforms:

The procedure entrypoint CreateFile2 could not be located in the kernel32.dll.

If you explicitly configure your minimum deployment target (instead of relying on toolchain defaults, which get bumped as time goes by), you will not have this issue. (We do so, and the compiled code is correct and can run on whatever the targeted minimum version of Windows is.)

The preprocessor conditions are correct. *

Updated: Actually I'm not sure if that's even a viable option, because if the library is built with a package manger such as vcpkg, we would still need to patch the CMakeFiles.txt file, so either way it requires manual patching to support older platforms, so maybe nevermind the issue.

Please see my suggestions above. Explicitly setting your desired minimum targeted version of Windows will not only fix the issue you are encountering here, but will prevent similar issues with numerous other vcpkg ports and save you many other headaches. (We do this for games that must ship to Windows Vista+ - it works.) If you need a specific minimum deployment target, you really must explicitly define it instead of relying on toolchain / Github Actions runner defaults.


* That all said, I'd not be averse to reverting the #if defined(PHYSFS_PLATFORM_WINRT) || (_WIN32_WINNT >= 0x0602) // Windows 8+ CreateFile2 one for convenience - since (at least for the time being) that's the one someone might hit if they haven't properly configured their minimum targeted Windows version (based on common defaults).

slime73 commented 10 months ago

users can no longer launch the resulting application on Windows 7

My app ran into the same issue and I had to release a hotfix for it that reverted that one line in physfs. I understand that adding explicit configuration to build settings could also fix it, but my app uses a bunch of different libraries and this was the only one that ended up relying on that setup, so in my case at least it made sense to revert the small bit of library code for the hotfix instead of changing the app's build process - which could have had other unintended side effects. That explicit configuration also isn't required for an app to build so I imagine many apps don't have it set up.

past-due commented 10 months ago

users can no longer launch the resulting application on Windows 7

My app ran into the same issue and I had to release a hotfix for it that reverted that one line in physfs. I understand that adding explicit configuration to build settings could also fix it, but my app uses a bunch of different libraries and this was the only one that ended up relying on that setup, so in my case at least it made sense to revert the small bit of library code for the hotfix instead of changing the app's build process - which could have had other unintended side effects. That explicit configuration also isn't required for an app to build so I imagine many apps don't have it set up.

Yeah - I can see the argument for reverting that one line.

But having hit these sorts of issues when numerous libraries started conditionally using Windows Vista+ APIs, and then Windows 7+ APIs (behind the same sorts of conditionals) - and also keep in mind that Windows API structure definitions (and available flags) can expand or change based on the minimum Windows target - I would nonetheless strongly encourage you to properly and explicitly set your targeted minimum Windows version in your build process (as Microsoft documents and suggests) or you will eventually be frustrated by issues elsewhere as well.

(The configuration is required, you are just relying on the defaults of your particular toolchain, Visual Studio project template, (etc) if you don't set / change it yourself.)

icculus commented 10 months ago

Okay, this should be resolved now with the PR that just merged (thank you, @past-due!) This gets us back to XP as the minimum supported platform, regardless of SDK target settings.