Support Unicode file paths for non-VC++

[AnonymousRandomPerson]

Description

Use filesystem::path instead of C-strings when opening/saving files.

Motivation and Context

On non-Visual-C++ builds (e.g., macOS builds), the fallback code for opening files in RawFile doesn't handle non-ASCII characters properly, and fails to open files named with these characters. Seen with the "é" character when attempting to import a Pokémon ROM.

How Has This Been Tested?

Tested by importing a Pokémon Pearl ROM with the "é" character in the file name. Music files load correctly from the ROM with this change.

Tested on macOS 11.5.2 with Apple Clang++ ARM64 compiler (version 12.0.5, clang-1205.0.22.11).

Screenshots (if appropriate):

N/A

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• I have read the CONTRIBUTING document.

[sykhro]

Thank you for the contribution, using std::filesystem is on the to-do list. This is fine but please drop the preprocessor block, VC++ supports it too.

[sykhro]

Both files are missing #include <filesystem>. I guess that AppleClang pulls it in via some other header

[AnonymousRandomPerson]

Interesting, I'll troubleshoot these builds more.

[sykhro]

Why is bumping to Catalina required for std::filesystem? I'd prefer to avoid that given that we already limit ourselves to 10.14 for basically no reason (everything works all the way back to 10.13)

[AnonymousRandomPerson]

The Mac build gave this error after removing the _MSC_VER check:

/Users/runner/work/vgmtrans/vgmtrans/src/main/RawFile.cpp:47:25: error: 'path' is unavailable: introduced in macOS 10.15
file.open(filesystem::path(theFileName), ios::in | ios::binary);

I tried bumping the version up to verify that the build passed, though I agree this is not an ideal solution. It may be something about the older macOS versions not defaulting to C++17, which I'll look into.

[sykhro]

The Mac build gave this error after removing the _MSC_VER check:

/Users/runner/work/vgmtrans/vgmtrans/src/main/RawFile.cpp:47:25: error: 'path' is unavailable: introduced in macOS 10.15
file.open(filesystem::path(theFileName), ios::in | ios::binary);

I tried bumping the version up to verify that the build passed, though I agree this is not an ideal solution. It may be something about the older macOS versions not defaulting to C++17, which I'll look into.

I suppose this is using xcode, it would make sense then.
Until recently, our CI has been always using clang (and its stdlib) from brew, effectively bypassing xcode.. maybe we could switch to ghc::filesystem, which can act as a fallback implementation in case the stdlib has no filesystem.

[AnonymousRandomPerson]

The CI build actually failed for Mac too before bumping the OS version (https://github.com/vgmtrans/vgmtrans/pull/250/checks?check_run_id=3634755614). From the logs, it seems to be pulling the Xcode version of filesystem despite using clang from brew.

/Applications/Xcode_12.4.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/filesystem:738:24: note: 'path' has been explicitly marked unavailable here
class _LIBCPP_TYPE_VIS path {

Perhaps it's possible to link in a more up-to-date version of filesystem that doesn't have this restriction.

[sykhro]

I see, what a mess. It's probably easier to set up ghc::filesystem (with the preprocessor machinery to detect if the platform needs it) and call it a day

[sykhro]

This is not how we handle external dependencies. Besides Qt, which is prebuilt (and optional, being Windows-only and intended to save the hassle of installing the whole framework), we use subtrees (this is preferable especially because ghc::filesystem is header only). We then wrap it in a CMakeLists for the CMake build system and a .vcprojx for WTL. If you don't know how to write a .vcprojx you can copy zlib's, which is pretty simple and was hand-written. It will need to be added to WTL using project references

[AnonymousRandomPerson]

Okay, I'll remove the submodule and copy the library instead.

[AnonymousRandomPerson]

Okay, managed to get a setup working. ghc::filesystem is a header-only library, so rather than link a project reference in VS, I added the library to AdditionalIncludeDirectories. For simplicity, I opted to use exclusively ghc::filesystem rather than using it as a fallback.

[sykhro]

Okay, managed to get a setup working. ghc::filesystem is a header-only library, so rather than link a project reference in VS, I added the library to AdditionalIncludeDirectories. For simplicity, I opted to use exclusively ghc::filesystem rather than using it as a fallback.

Thanks, I suppose this is fine. If it ends up being slower than the stdlib's implementation we can switch to including the header with the dynamic selection.