Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

CMake: Set _WIN32_WINNT=0x0601 #900

Merged
merged 3 commits into from
Feb 10, 2025
Merged

Conversation

past-due
Copy link
Contributor

@past-due past-due commented Feb 8, 2025

This seems to have been added back in #327, and set to 0x0602 (Windows 8) as the target minimum Windows version.

However, all used Windows APIs are compatible with Windows 7 (and some folks still need to target Windows 7), so this PR updates _WIN32_WINNT to 0x0601 (Windows 7).

@saghul
Copy link
Contributor

saghul commented Feb 8, 2025

The problem is windows 7 is rol and there is no easy way to test in a ci to make sure we don't break it.

@past-due
Copy link
Contributor Author

past-due commented Feb 8, 2025

The problem is windows 7 is rol and there is no easy way to test in a ci to make sure we don't break it.

That’s part of the benefit: By setting _WIN32_WINNT=0x0601, there will be a compile error if someone tries to use an API (or new Windows API flag, etc) that isn't available on Windows 7. (Caught by the existing CI runs.) This helps provide a lot of assurance that it doesn't get broken.

(Hence why the MS recommendation is to set it to the minimum operating system version that the code is intended to support.)

@saghul
Copy link
Contributor

saghul commented Feb 8, 2025

I understand but I don't know if we want to be beholden to an operating system that is EOL already.

I'd be ok landing this today, with a big warning: the moment it becomes problematic we take it out.

@bnoordhuis thoughts?

@bnoordhuis
Copy link
Contributor

The number of people that still seem to use Windows 7 in this day and age never ceases to surprise me... but anyway.

If we set that in CMakeLists.txt, readers will reasonably assume we support Windows 7 when we don't. Even Windows 8 is already stretching it.

If @saghul is okay with landing this, then I won't block it, but by myself I would've probably rejected it.

@saghul saghul merged commit 5a949ca into quickjs-ng:master Feb 10, 2025
59 checks passed
@saghul
Copy link
Contributor

saghul commented Feb 10, 2025

Let's give it a try.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants