win32: support long file paths #13134
Conversation
|
For the record, there was already this PR: #12119 Although this does not propose any changes to the binary manifest on Win32. |
|
I didn't expect things to be complicated like that. I have this manifest option enabled in mpv.net, and it works well, if I remember correctly. |
|
Yep, enabling it in the manifest should be enough - that's what I gathered from the docs as well. The other PR was some kind of workaround, I believe. |
|
Download the artifacts for this pull request: |
|
confirming this fixes the referenced bug |
|
this one: #10025 |
|
Worth mentioning that you have to manually enable this in the registry: I remember this being discussed a while ago and someone discovering that certain popular applications silently enable this feature system-wide. |
|
I couldn't comment before, but this PR is incomplete. You have to go through all affected API calls and change the hard coded buffer sizes to dynamically allocated ones, as there is no limit on the path lengths anymore. Last time I checked there were at least a few places to change. Buffer overflow on arbitrary user input is "not ideal"(tm).
Indeed the Python installer does that, it is opt-out, but is enabled by default IIRC. |
|
This suggests that an mpv installer should have this option (i don't know
if there is one), or that information should be added to the command line
help that mentions that while on windows long file paths are supported,
they are not going to be enabled without a registry setting.
…On Thu, Dec 21, 2023 at 6:34 PM Kacper Michajłow ***@***.***> wrote:
I couldn't comment before, but this PR is incomplete. You have to go
through all affected API calls and change the hard coded buffer sizes to
dynamically allocated ones, as there is no limit on the path lengths
anymore. Last time I checked there were at least a few places to change.
Buffer overflow on arbitrary user input is "not ideal"(tm).
I remember this being discussed a while ago and someone discovering that
certain popular applications silently enable this feature system-wide.
Indeed the Python installer does that, it is opt-out, but is enabled by
default IIRC.
—
Reply to this email directly, view it on GitHub
<#13134 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABPWPU275OL2N6PS4SQBDTYKRXKLAVCNFSM6AAAAABA2U2KSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRWGY4TCNZWGA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
mpv uses
Surely the win32 API takes buffer sizes and does not blindly write as much as it wants. |
|
Yes, when the path is truncated, mpv should MP_WARN with instructions. We could even add a specific command to do it for the user. |
|
@sfan5 Doesn't change the fact that long path option is opt-in for a reason and applying it blindy is not ideal. But if you checked that's fine, it's ok. I cannot look up the code right now. But this PR looks incomplete to say the least. (There is no documentation update even...) It would have been done by now if it were so easy. EDIT: sorry for the double post |
|
the warn-on-truncation thing is a good idea.
i thought of suggesting that, but i thought maybe it's a little much. if you think that's a good idea, i'll defer to you. |
The real question is. Does mpv suppose to support long paths, or just pretend to do it? Whole Lines 553 to 575 in c09245c Some of the things will work, others will not work. That's true Microsoft takes care about API safety, so it is hard to overflow buffers, but supporting long paths sometimes can have it's own problems. If you open a file and truncated path is passed around and someone do My point was only that those |
Yes, but even if I somewhat repeat myself here, I don't see this as of any concern for mpv. Not at all. Like, the hypothetical end-user™ comes asking that question, and mpv project can just say: "we support it, our job is done". This feature, or option, exists in Windows, even if - for now - it is toggled off by default. So, the burden is on the end-user, to voluntarily change this option. Or to pester MSFT to change the defaults. |
This is a hard limit of the win32 directory enumeration API. Enabling long path also doesn't change the filename length limit of the underlying filesystem, which is 255 characters for NTFS, FAT32, exFAT, and ReFS. For all practical purposes, this isn't a case that's affected by this change. |
I agree, but I forgot to check prior to merging this. |
i have to disagree, it's a bad point of view for a project to take in my opinion. if i can install mpv on a windows system and a feature doesn't work because some arcane setting in a database that's supposed to be modified by programs running on that system such as mpv, then that feature is broken and needs to be fixed. "our job is done" is the epitome of anti-user design. user-friendly design starts with what a user wants to do ("the user wants to play files on very long paths") and implements everything so that the user can do it without having to manipulate any other things. where some things are not possible for the program to do, the program should instruct the user what to do. you ought to deliver "the whole package" and not just some slapdash that no one knows how to make magically work while refusing to elaborate why it doesn't magically work. |
This is simply not true. Since Windows 10, version 1607, the The maximum total path length post Windows 10 v1607 change is 32 767 characters. |
|
@Hrxn "file name" is usually understood as what you call "path component".
"file name" is usually not understood to mean "file path", especially not
"full absolute file path".
hope that puts everyone on the same page now.
…On Fri, Dec 22, 2023 at 5:22 PM HRXN ***@***.***> wrote:
[..] Enabling long path also doesn't change the filename length limit of
the underlying filesystem, which is 255 characters for NTFS, FAT32, exFAT,
and ReFS. For all practical purposes, this isn't a case that's affected by
this change.
This is simply *not true*.
Since Windows 10, version 1607, the MAX_PATH limitations has been removed
from the Win32 API. That is the whole point of the LongPathsEnabled /
longPathAware exercise.
The *maximum* total path length post Windows 10 v1607 change is 32 767
characters.
You mention the filename length limit of the underlying filesystem. What
you write here is misleading. There is no "filename" length limit that is
part of the filesystem itself. The filesystem has a *path component*
limitation, which is 255 characters for all common/modern filesystems, but
again, this is a limit *per* path component, not a limit of a full file
name.
—
Reply to this email directly, view it on GitHub
<#13134 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABPWPXABNWW3U2M2QYP6TDYKWXUVAVCNFSM6AAAAABA2U2KSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRXHA3DKNJRGE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Look, I'm not going to write a treatise here on what does or does not constitute "user-friendly" design, not because I don't hold any qualified opinions on this subject matter, no, I simply do not have the time to do so, so I'm going straight to the significant omissions of what you have written here. The assumption you made here is entirely wrong. You cannot "install" mpv on a Windows system, you cannot do the same on any other operating system, for that matter. Simply because mpv is what you can see on here ( You cannot simply have the same expectation here as of the official Windows installers for Python, as provided by the official Python project / The Python Software Foundation, which do indeed check for Once there is an official installer for mpv on Windows provided by this project, we can have this discussion again (hint: never). Also, it is definitely not some "arcane" setting. It's quick and easy to make this change. Asking the end-user to fetch a registry ( Considering this as a broken feature is something you would have to take up with Microsoft. They don't made the step (yet) to make this change as a default, whatever the reasoning. Probably just not that high up their or their customer's priority list. But I would not be surprised if this will indeed become the new default in a new version of Windows. |
Completely irrelevant distraction to the issue at hand here. Any program (hint: this includes mpv) that opens a file needs to be able to uniquely address the file it is directed to open. And what makes a file uniquely addressable? It's what is colloquially called the "full name", in other words, the full path name, or, to be even more exact: the Fully qualified path name. It does not work with a file name alone. Also, if I may remind everyone, you, in your very own comment, confirmed this as fixed for you. |
|
@Hrxn your recent comments are adversarial to everyone trying to just get this stuff done and irrelevant to the work. You have exactly one commit in the mpv repository, which is "OSC: fix indentation and stray whitepace". It removes two spaces from code indentation. You are tied for place 408 in the list of contributors with regards to amount of commits. Yet you act like you own the project and everyone in this PR. You don't know how to work collaboratively. Please leave and let us do our work. |
|
Completely irrelevant distraction to the issue at hand here.
The "issue at hand here" is the win32 directory enumeration function. The `cFileName` in the `WIN32_FIND_DATA` structure used to return the enumerated filename is limited to `MAX_PATH` characters, regardless of the long path setting.
The term "filename", in the context of this `cFileName` member and the underlying implementation of the filesystems, refers to the name of the file without any path information.
|
|
This one?
https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-findfirstfilea
|
lmao, there's a place in the list of contributors? like a ranking? first time i've heard of such a thing. |
|
By default, the name is limited to MAX_PATH characters. To extend this limit to 32,767 wide characters, prepend "\?" to the path. For more information, see Naming Files, Paths, and Namespaces.
This refers to the `lpFileName` parameter of `FindFirstFile` function, which supports long path. The `cFileName` member in the `WIN32_FIND_DATA` structure does not.
|
|
Please limit personal remarks/attacks to minimum. This is not a good way of communicating. There are more important things to discuss, no need to include unnecessary remarks. |
|
@kasper93 hrxn shows up out of nowhere and starts bossing people around and wastes people's time by being snarky about docs he doesn't understand. this needed to be nipped in the bud, hopefully it'll stick. |
#include <windows.h>
#include <tchar.h>
#include <stdio.h>
void _tmain(int argc, TCHAR *argv[])
{
WIN32_FIND_DATA FindFileData;
HANDLE hFind;
if( argc != 2 )
{
_tprintf(TEXT("Usage: %s [target_file]\n"), argv[0]);
return;
}
_tprintf (TEXT("Target file is %s\n"), argv[1]);
hFind = FindFirstFileEx(argv[1], FindExInfoStandard, &FindFileData,
FindExSearchNameMatch, NULL, 0);
if (hFind == INVALID_HANDLE_VALUE)
{
printf ("FindFirstFileEx failed (%d)\n", GetLastError());
return;
}
else
{
_tprintf (TEXT("The first file found is %s\n"),
FindFileData.cFileName);
FindClose(hFind);
}
}
MSFT docs say this API function supports LongFilePaths. Who is wrong? |
|
shinchiro/mpv-packaging@1c534ce The built-in installer for mpv-winbuild-cmake now auto enable windows long path support. Hopefully this will help solve the problem here. |
win32: support long file paths