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

Windows 7 #12330

Closed
harryray33 opened this issue Sep 3, 2023 · 14 comments
Closed

Windows 7 #12330

harryray33 opened this issue Sep 3, 2023 · 14 comments
Labels

Comments

@harryray33
Copy link

Does this work with Win 7?

Thanks

@CounterPillow
Copy link
Contributor

god I hope not

@sfan5
Copy link
Member

sfan5 commented Sep 3, 2023

readme

@sfan5 sfan5 closed this as completed Sep 3, 2023
@jimmy-1000
Copy link

It worked good, but the last version 20230924 doesn't work.

shinchiro/mpv-winbuild-cmake#445

@Dudemanguy
Copy link
Member

GetSystemTimePreciseAsFileTime is not found at Kernel32.dll

We don't actually use this API in mpv directly so not sure where this is from?

@kasper93
Copy link
Contributor

kasper93 commented Sep 30, 2023

We don't actually use this API in mpv directly so not sure where this is from?

We set _WIN32_WINNT to 0x0602 which is Windows 8. In this case headers can pull any function that is available on this platform, even if not used directly.

Anyway, we cannot be half way, either we support fully Windows 7 and set _WIN32_WINNT properly or we don't.

Targeting Windows 7 is just getting harder, because most toolchain like MSVC doesn't support building for it anymore. And apparently clang too, because I'm not sure it is _WIN32_WINNT, might be clang itself.

llvm uses this function for std::chrono and if any of mpv dependencies uses it we require it too. https://github.com/llvm/llvm-project/blob/d222c5ec47a09a3e120000e4a1006f4da7741256/libcxx/src/chrono.cpp#L70-L92

Probably not directly mpv "fault".

@Dudemanguy
Copy link
Member

Dudemanguy commented Sep 30, 2023

Yeah looks like nothing we can do about. Not that we're looking to have Windows 7 compatibility or anything.

@avih
Copy link
Member

avih commented Sep 30, 2023

Targeting Windows 7 is just getting harder, because most toolchain like MSVC doesn't support building for it anymore. And apparently clang too

I don't know about msvc, but that's also irrelevant because mpv doesn't support msvc.

Clang definitely supports win 7 at the latest version, which is 17, and you can even target windows XP with UCRT, by invoking e.g.

clang -DWINVER=0x501 \
      -D_WIN32_WINNT=0x501 \
      foo.c -o foo.exe \
      -Wl,--major-subsystem-version=5

(and installing the appropriate UCRT runtime, which exists for windows XP and later).

I'm quite sure all versions of gcc also support win 7 (and XP, if one insists).

So I don't thinki compiler support can be stated as a reason for dropping windows 7.

@kasper93
Copy link
Contributor

kasper93 commented Sep 30, 2023

So I don't thinki compiler support can be stated as a reason for dropping windows 7.

Never was stated like that, we have a lot more reasons to not support Windows 7. And the main being that there is no developers to work on it. Patches welcome. Feel free to keep supporting it if you are interested in it.

bikesheding Windows 7 support is one of the least interesting things you can do with mpv... in my opinion.

@avih
Copy link
Member

avih commented Sep 30, 2023

I simply pointed as an absolutely incorrect statement and argument which you made.

Nothing less, nothing more.

@kasper93
Copy link
Contributor

kasper93 commented Sep 30, 2023

What statement is incorrect? My statement is backed up by the fact that default installation (that is build by shinchiro) of llvm does not support Windows 7, as pointed by others

GetSystemTimePreciseAsFileTime is not found at Kernel32.dll

The fact that you can patch build to support older version is quite obvious, but that's not the case currently.

I'm saying that, because I believe this comes from std::chrono from libc++. That's why gcc build is not affected and that's why in fact mpv is not at fault, but something in dependency chain.

@DeadSix27
Copy link
Contributor

Various things in w32_common are at minimum supporting updated Windows 10, so in the best case it just noops the functions otherwise it crashes.. I don't know why anyone would use Windows 7 anyway, unless it's running on an outdated industrial platform, which would be crazy for other reasons.

@kasper93
Copy link
Contributor

Various things in w32_common are at minimum supporting updated Windows 10, so in the best case it just noops the functions otherwise it crashes.

All of them are either dispatched with getprocaddr or would just fail silently if not supported. Don't make the issue bigger than it really is ;)

@DeadSix27
Copy link
Contributor

DeadSix27 commented Sep 30, 2023

All of them are either dispatched with getprocaddr or would just fail silently if not supported. Don't make the issue bigger than it really is ;)

Fine, my point still stands though 🙃. A little skepticism is never bad.. and upon re-checking I think the newest function we use is vista+.

EDIT:
And as you said, if someone wants to patch for win7 just do it.

@kasper93
Copy link
Contributor

Yes, I recently removed some Windows XP dispatch, but we try to keep it sane. Not really breaking anything Win 7+ intentionally.

I simply pointed as an absolutely incorrect statement and argument which you made.

Just for the visibility, since we are arguing over this one thing.

mingw-w64-headers since this commit mingw-w64/mingw-w64@f3c53a5 uses Windows 10 as default _WIN32_WINNT value.

mpv's itself sets Windows 8, but the offending function does not come from mpv. It comes from std::chrono which does dispatch only for < Win8. https://github.com/llvm/llvm-project/blob/e39727d41fcdcdae2906ae0c982a6ebd19f442ad/libcxx/src/chrono.cpp#L70

And since libc++ is also build by this toolchain it will be build for Win10.

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

No branches or pull requests

8 participants