Switch is_directory to exists in assertPathExists

[GranPC]

is_directory returns false for symbolic links and reparse points. This, combined with the fact that USVFS walks up the directory chain when adding an entry to the redirection tree, causes overlays to fail for directories where the path contains a symbolic link or a reparse point.

While that's fine for most users, people who use symbolic links to manage their game collection, or people who mount another disk in a NTFS directory, would be unable to overlay anything inside this game directory. It turns out that with the way Wine works internally, most Linux users actually have such a setup by default, and as of wine commit wine-mirror/wine@6b498d9, which exposes this internal detail to Win32 software, USVFS no longer works.

Changing the function from is_directory to exists seems to have fixed this issue with no adverse side-effects.

[LostDragonist]

Initial testing with Skyrim SE seems fine. Tested the game, SKSE, SkyUI, CBBE, Bodyslide, FNIS, xEdit, and Wrye Bash.

[isanae]

I can't test this right now, but what happens if you do VirtualLinkFile() with an invalid path c:\dir\file\bleh, where dir is a directory and file is a file? That's something plugins can do.

[GranPC]

I can't test that myself either since I don't actually own a copy of the game and the test cases in this repository seem to be broken, but I could loop in the friend I developed this patch with.

My first attempt was changing the is_directory call to is_directory || is_symlink but that still failed for non-symlink reparse points; I'm not sure the Boost API has a function to figure out whether a path corresponds to a traversable reparse point.

If the scenario @isanae propose turns out to be troublesome, we could try and adjust the code to either use the Win32 APIs directly instead of Boost, or possibly refactor the code so it doesn't have to walk up the entire directory chain. I'm not 100% sure I understand why VirtualLinkFile() with c:\games\mods\fnv\overlays needs to check the validity of c:\, c:\games, c:\games\mods and c:\games\mods\fnv - as I understand it, as long as the last element in the path is valid, there shouldn't be any issue.

[isanae]

What about something like is_directory() || (exists() && !is_regular_file()) to be on the safe side?

[GranPC]

Yeah, that might actually be a great approach! We'll give it a shot and update the patch if that turns out to work fine. Thanks!

[isanae]

Make sure to add a comment before the if to explain what's going on and why the second test is needed.

[GranPC]

About to test this!

I've noticed that it might also be possible to do something like is_directory() || is_symlink() || status(targetPath).type() == file_type::reparse_file - I'm about to check and see if this solution could work. Do you reckon it would be better than just checking against directory or "non-regular file"?

[isanae]

I think this is mostly equivalent to what I wrote. I'm not sure what else exists() && !is_regular_file() could match except for symlinks and reparse points. It can end up accepting things that are not directories, but since the check is never done against the original path, only parent_path(), I don't think it's much of an issue. If that's the case, I'd prefer the simpler code.

[GranPC]

Both solutions worked, but the one I committed feels like a more solid and easier to read solution, since it's more explicit.

I saw your comment right after committing - if we prefer the exists() && !is_regular_file() solution, I can adjust my PR to use that instead. From reading the Boost docs, it looks like the only case where it might make a difference is if we don't have enough permissions to determine the type, in which case it'd be type_unknown, but I'm not sure if that applies to Win32.

[isanae]

It's fine. Instead of referring to the pull request in the comment, I'd include the relevant information. Something like "the path should be a directory, but is_directory() returns false for symlinks and reparse points; the condition can technically be true for symlinks to files, but since it's checked against parent_path(), it's unlikely to happen".

[GranPC]

Oh cool, I'll change it to say that instead.

I've been doing these changes from the GitHub web UI since I don't actually have a functioning copy of Windows at all - do you know if it's possible to squash commits from there, or should I create a newer branch to keep the commit history clean, or does it not matter?

Also, it looks like the build broke with this last commit, although it built fine locally and I don't see any errors in the appveyor log. Any idea what's up with that?

Thank you!

[isanae]

No need to squash, it's fine. The failure is because we're exceeding disk space on appveyor, a perennial issue that we can't fix, except by begging them to give us more space. We can't even delete the artifacts ourselves.

[GranPC]

That sounds like a really frustrating situation. Sorry if I exacerbated the issue with my additional commits!

Hopefully this will be good enough to be merged in usvfs - as soon as this issue gets solved, Linux users running the latest Wine version will be able to play with mods again :)

[isanae]

We didn't get any other report that MO doesn't work at all with Wine right now. Is it a new problem?

[GranPC]

The problem was introduced with a Wine update a while ago. As far as I know, Wine 5.0 was the first version to include a patch that allowed USVFS to work, and then Wine 5.4 or 5.5 broke it again with a different change - the one I referenced earlier in this thread. Many people use an older version of Wine or Proton for certain games, so they might not be affected by the issue yet, until they update to the latest version.

While Wine support is my main motivation behind this patch, I believe Windows users could also hit this edge case with certain exotic configurations - so either way, the patch just makes the code more correct.

[Tea23]

I'll chime in cos I've been working with @GranPC on this. So yes, it is a new problem. A regression test of Wine shows that MO (specifically USVFS) stopped working at Wine commit wine-mirror/wine@6b498d9 because it exposes reparse points.

That commit was from almost a year ago and apparently community folk have been aware of this, but haven't looked into it or said anything. I've been bored and really wanting to play New Vegas (in Wine because I'm insane) so I started bothering GranPC!

The outstanding issue I have in Wine with MO now is that NCC will not run due to relying on dotnet46 which currently doesn't work in Wine at all, so I am compromising by setting up my mods in Windows and then just transferring the folder / profiles over. USVFS works great with the the patch otherwise.

[isanae]

This patch will be included in the next devbuild, which should be out on discord within the next few days. Thanks to both of you.