-
-
Notifications
You must be signed in to change notification settings - Fork 107
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
physfs_platform_windows.c: Use newer APIs when permitted by _WIN32_WINNT #7
Conversation
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.) |
From the Microsoft docs (linked above):
The intention is that |
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! |
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:
Bacause they end up with a binary that cannot find the
Would it be possible to add an override for 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. |
Ugh, I didn't realize one of these comments said "Windows 8+" instead of "Windows XP+" I'm reverting this. |
That'd be good, yes |
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.
add_definitions(-D_WIN32_WINNT=0x0601) # Windows 7+
set(VCPKG_C_FLAGS "-DWINVER=0x0601 -D_WIN32_WINNT=0x0601") # set default minimum deployment target of Windows 7 for all ports
set(VCPKG_CXX_FLAGS "-DWINVER=0x0601 -D_WIN32_WINNT=0x0601") # set default minimum deployment target of Windows 7 for all ports This makes your minimum deployment target explicit, and no longer relies on the defaults of your particular combination of environment and toolchain.
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. *
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 |
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.) |
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. |
Benefit from the newer APIs when possible, based on the definition of
_WIN32_WINNT
.