[sebasje]

Directory paths reach libdjinterop as std::string. SQLite reads them as UTF-8, but path_exists() and create_dir() passed them to stat() and _mkdir(), which on Windows use the ANSI code page. A library under a directory named outside the code page, such as a Japanese artist folder on a USB stick, could be created by SQLite and then not found.

This adds util::path_from_utf8(), which converts through std::u8string on Windows, and uses std::filesystem for both helpers and for opening the schema script. The public headers now document that directory strings are UTF-8.

The new test creates and loads a database in a non-ASCII directory for every schema. It passes before and after on Linux and macOS, because the bytes go through untouched there. It only fails without the fix on Windows, which this repository's CI doesn't build. I verified the fix in Seabass's MSVC build.

One side effect on POSIX: new directories get 0777 minus the umask instead of a fixed 0755. That is the same result under the usual 022 umask.

[mr-smidge]

Thanks for this - I mainly develop on Linux so it's very likely I never would have caught this bug!

Historical point: from memory, the create_directory() and path_exists() functions are there because libdjinterop used to target older OSes which didn't have std::filesystem available. The baseline OS requirements have moved on since then, and there's probably no reason for those utility functions to even exist anymore, given that we now have std::filesystem::create_directory() and std::filesystem::exists() - seeing the std library calls are probably more obvious to the casual reader.

Also, I think the change to 777 perms on POSIX systems is a regression: there are overloads of std::filesystem::create_directory() which take an existing_p path, and perhaps if we remove the utility function and move up to the call site, there will be an obvious reference directory (the parent) from which to copy attributes.

Last point: because libdjinterop is still in the 0.x phase of its life, breaking API changes are still a workable possibility. Do you think the interface would be higher quality if the public API replaced the use of std::string as a path with std::filesystem::path instead?

So in summary, my recommendation is:

• Review whether these utility functions even need to exist anymore.
• Ensure that any newly-created dir takes on appropriate permissions, not just 777.
• Maybe it's worth going crazy and replacing all use of std::string with std::filesystem::path in the public API? Keen to hear your thoughts on this.

[sebasje]

Thanks a lot for your thorough review, the background, and your thoughts on it! Let me explain mine.

Dropping create_dir() / path_exists(): agreed. They are thin wrappers around std::filesystem already, so the next step is to call it directly at the call sites (all ten or so are in engine_library_dir_utils.cpp). That also fixes something the wrapper hides: it throws away the error_code and throws a generic "Failed to create directory", so calling the standard function directly lets the real OS error reach the exception message. The UTF-8 conversion itself has to stay as long as the public API takes std::string, since that is the actual Windows fix.

Permissions: you're right that the requested mode changed, from mkdir(dir, 0755) to the 0777 that std::filesystem::create_directory() uses. Both go through the umask, though, so with the usual 022 the result is 0755 either way. They only differ under a looser umask such as 002, and then 0775 is what that user asked for; it is also what mkdir(1) does. I'd be a bit wary of the existing_p overload: it copies the parent's mode, so a directory created under /tmp or a shared directory would inherit the sticky bit and the parent's broad permissions, and on FAT/exFAT sticks (where most Engine libraries live) the mount options decide anyway. My suggestion is to keep the umask default. If you'd rather keep exactly the old behaviour, I'm happy to request 0755 explicitly instead.

std::filesystem::path in the public API: I like it, it is the better interface. The type carries the encoding, and Windows callers could pass wide paths without going through UTF-8 at all. A few things I noticed while thinking it through:

• SQLite still wants UTF-8 strings. The library builds paths like directory + "/m.db" and hands them to sqlite::database{...} and ATTACH ?, so internally we still need a path-to-UTF-8 helper. In C++20 path::u8string() returns std::u8string, so that takes a small conversion, and path::string() on Windows is the same ANSI code page trap in the other direction.
• It has to be a clean break: keeping std::string overloads next to std::filesystem::path ones makes every call with a string literal ambiguous, since both accept const char*.
• It is a breaking change for downstream users such as Mixxx, which the 0.x phase allows, but it's worth a line in the release notes.

So my proposal: I'll update this PR to call std::filesystem directly and drop the two helpers, keep the umask default, and keep the UTF-8 test, so the Windows fix can land on its own. I'm happy to follow up with a separate PR that moves the public API to std::filesystem::path, if you agree that's sensible.

The updated version of this PR is coming up shortly.

[sebasje]

The update is pushed as ac95573: the two helpers are gone, and the call sites use std::filesystem directly.

Thanks again for the thorough review, the feedback, and the time you put into this. By the way, I'm a Linux user and developer too. I'm just making my software available on Windows and Mac as well, and that's when these things surface.

[sebasje]

Here is what the std::filesystem::path API change looks like on top of this PR: #207. It is stacked on this branch, so the change itself is the last commit there.

[mr-smidge]

Thanks - LGTM