Skip to content
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

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.

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