Skip to content

Explicitly default the expected Windows API to 0x0601 (Windows 7) #8680

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

Merged
merged 4 commits into from
Aug 5, 2025

Conversation

mrotteveel
Copy link
Member

@mrotteveel mrotteveel commented Aug 2, 2025

Explicitly default the expected Windows API to 0x0601 (Windows 7)

Also:

  • Reject versions before Windows 7 (0x0601)
  • Ensure firebird.h is included first to avoid translation units unexpectedly using a different version or other config than defined in firebird.h
  • Also raise libcds expected Windows API to Windows 7 (0x601) instead of Windows 2000 (0x0500)/Windows XP (0x501)
  • Removes some inappropriate defines to very old Windows API versions

@mrotteveel mrotteveel requested a review from asfernandes August 2, 2025 12:07
@mrotteveel mrotteveel self-assigned this Aug 2, 2025
@mrotteveel
Copy link
Member Author

Note that defaulting to Windows 10 (0x0A00) just formalizes and makes explicit what already happens inside the Windows 10 API headers when _WIN32_WINNT is not defined, and gives us a point to control it for the future.

I have considered setting the yvalve project (fbclient) to explicitly use a lower _WIN32_WINNT value (e.g. 0x0601 for Windows 7), but did not do so at this time.

@hvlad
Copy link
Member

hvlad commented Aug 2, 2025

Note that defaulting to Windows 10 (0x0A00) just formalizes and makes explicit what already happens inside the Windows 10 API headers when _WIN32_WINNT is not defined, and gives us a point to control it for the future.

I'm not sure it should be done this way. Ideally, _WIN32_WINNT should be set to the minimal possible value that allows code to compile. Or even not set, but asserted. This allow anyone to use different (older) Platform SDK to try build Firebird for own purposes (for ex. to ensure compatibility with pre-Win10).

I have considered setting the yvalve project (fbclient) to explicitly use a lower _WIN32_WINNT value (e.g. 0x0601 for Windows 7), but did not do so at this time.

This define will not make anything compatible with Win7. It doesn't affect CRT code, for example. Also, fbclient is not build from the single project.

I.e. I support removal of current define's (3 or 4 places, iirc) but don't think we must explicitly set _WIN32_WINNT.
In some places it could be check'ed for the required minimal value, if really necessary.

@hvlad
Copy link
Member

hvlad commented Aug 2, 2025

Also, I see no need to change LibCDS as any other external library. At least until it builds.

@mrotteveel
Copy link
Member Author

mrotteveel commented Aug 2, 2025

Note that defaulting to Windows 10 (0x0A00) just formalizes and makes explicit what already happens inside the Windows 10 API headers when _WIN32_WINNT is not defined, and gives us a point to control it for the future.

I'm not sure it should be done this way. Ideally, _WIN32_WINNT should be set to the minimal possible value that allows code to compile. Or even not set, but asserted. This allow anyone to use different (older) Platform SDK to try build Firebird for own purposes (for ex. to ensure compatibility with pre-Win10).

I agree it should be set to the minimum value that we expect it to compile, and I think that is the version that we want to support, or want to be able to support.

I asked for input on firebird-devel, and the only response I got was from Paul, so I simply decided to put in the default that we're currently already using by way of how the Windows API decides if you do not explicitly specify it! If that should be a lower value, then so be it, I can change it to a lower value.

I have considered setting the yvalve project (fbclient) to explicitly use a lower _WIN32_WINNT value (e.g. 0x0601 for Windows 7), but did not do so at this time.

This define will not make anything compatible with Win7. It doesn't affect CRT code, for example. Also, fbclient is not build from the single project.

As I understand it, yvalve depends on the projects it needs but not on their build outputs, so I expect that will cause those projects to be built as needed using the configuration for the yvalve, but maybe I'm misunderstanding how msbuild works there.

I.e. I support removal of current define's (3 or 4 places, iirc) but don't think we must explicitly set _WIN32_WINNT. In some places it could be check'ed for the required minimal value, if really necessary.

We should explicitly set _WIN32_WINNT, so that 1) all builds conform to what we want to use and be able to use and 2) compilation fails if you try to use a Windows API method that does not exist in the Windows versions we expect to support, or try to build using a too old version of the Windows API, and 3) it makes explicit what we want to support instead of the current situation that no one actually seems to know what Windows versions we intend to support (or at least allow Firebird to run, even if we don't support them).

Sure, setting it to Windows 10 might be "too high", but then suggest which version we should target.

@mrotteveel
Copy link
Member Author

Also, I see no need to change LibCDS as any other external library. At least until it builds.

As I mentioned on firebird-devel, LibCDS has some conditional code that does some detection or initialization differently for Windows Vista and higher, and for Windows 7 and higher, so raising it from some targets Windows 2000, some targets Windows XP to all targets Windows 7 seems rational to me.

Let me know if you want me to revert it.

@hvlad
Copy link
Member

hvlad commented Aug 4, 2025

I have considered setting the yvalve project (fbclient) to explicitly use a lower _WIN32_WINNT value (e.g. 0x0601 for Windows 7), but did not do so at this time.

This define will not make anything compatible with Win7. It doesn't affect CRT code, for example. Also, fbclient is not build from the single project.

As I understand it, yvalve depends on the projects it needs but not on their build outputs, so I expect that will cause those projects to be built as needed using the configuration for the yvalve, but maybe I'm misunderstanding how msbuild works there.

Note value of Link Library Dependencies setting (true).
yvalve depends on common and remote and linked with them.

I.e. I support removal of current define's (3 or 4 places, iirc) but don't think we must explicitly set _WIN32_WINNT. In some places it could be check'ed for the required minimal value, if really necessary.

We should explicitly set _WIN32_WINNT, so that 1) all builds conform to what we want to use and be able to use and 2) compilation fails if you try to use a Windows API method that does not exist in the Windows versions we expect to support, or try to build using a too old version of the Windows API, and 3) it makes explicit what we want to support instead of the current situation that no one actually seems to know what Windows versions we intend to support (or at least allow Firebird to run, even if we don't support them).

Sure, setting it to Windows 10 might be "too high", but then suggest which version we should target.

0x0601 (Win7), I think.

@hvlad
Copy link
Member

hvlad commented Aug 4, 2025

Also, I see no need to change LibCDS as any other external library. At least until it builds.

As I mentioned on firebird-devel, LibCDS has some conditional code that does some detection or initialization differently for Windows Vista and higher, and for Windows 7 and higher, so raising it from some targets Windows 2000, some targets Windows XP to all targets Windows 7 seems rational to me.

Let me know if you want me to revert it.

After more close look at the LibCDS code and tend to agree with you here.

@mrotteveel mrotteveel force-pushed the fix-ancient-windows-use branch from bb34496 to 1074640 Compare August 4, 2025 08:07
@mrotteveel mrotteveel changed the title Explicitly default the expected Windows API to 0x0A00 (Windows 10) Explicitly default the expected Windows API to 0x0601 (Windows 7) Aug 4, 2025
@mrotteveel mrotteveel force-pushed the fix-ancient-windows-use branch from b46c3ae to 6c78c50 Compare August 5, 2025 08:03
@mrotteveel
Copy link
Member Author

Can this be merged, @asfernandes @hvlad ?

* Reject versions before Windows 7 (0x0601)
* Ensure firebird.h is included first to avoid translation units unexpectedly using a different version or other config than defined in firebird.h
* Also raise libcds expected Windows API to Windows 7 (0x601) instead of Windows 2000 (0x0500)/Windows XP (0x501)
@mrotteveel mrotteveel force-pushed the fix-ancient-windows-use branch from 6c78c50 to 64469d4 Compare August 5, 2025 08:32
@mrotteveel mrotteveel merged commit e554b27 into FirebirdSQL:master Aug 5, 2025
23 checks passed
@mrotteveel mrotteveel deleted the fix-ancient-windows-use branch August 5, 2025 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants