Unicode filesystem path support for Windows

[jdlangs]

I'm trying to build this repository for Windows 10 UWP and am hitting the hardcoded error that unicode is not supported (from @hidmic in #35). From looking at this just for a few minutes, it seems it should be feasible to use the wide character versions of the Windows API calls and convert the data to a UTF8 encoded std::string in this case. Does this seem reasonable? Was there a reason it wasn't done this way initially?

[hidmic]

Does this seem reasonable?

While UTF-8 encoding would definitely be an option, these helpers aim to mimic std::filesystem, available in C++17 STL onwards. The latter uses std::wstring if need be instead, and I'd rather not deviate too much.

Was there a reason it wasn't done this way initially?

Lack of need and consistency. As you may have realized already, neither rcpputils nor rcutils attempt to handle wide characters. And AFAIK i18n support is not in the near term roadmap. Contributions are most welcomed though.

Tagging @clalancette and the @ros2/team for additional feedback.

[wjwwood]

While UTF-8 encoding would definitely be an option, these helpers aim to mimic std::filesystem, available in C++17 STL onwards. The latter uses std::wstring if need be instead, and I'd rather not deviate too much.

I wouldn't deviate at all. The goal would be to replace this interface with std::filesystem entirely without downstream changes.

[clalancette]

I wouldn't deviate at all. The goal would be to replace this interface with std::filesystem entirely without downstream changes.

Right, exactly. And with Galactic probably switching to C++17 support, that may be feasible now. Whether we get to it is another question.

[jdlangs]

Thanks all, that helps me understand the context a lot better. If I were to put in some time on this over the next few weeks what would be the recommended approach to solving this?

[clalancette]

The long-term plan is to remove rcpputils::fs completely in favor of std::filesystem. But I don't think we are quite there yet.

So that leaves us with implementing this functionality in rcpputils::fs in a 100% compatible way with std::filesystem. If you want to have an implementation you can get in right now (and backport to Foxy), you'll have to figure out how to do that without std::filesystem. If you want to make a bit of a leap and assume we'll go with C++17 for Galactic (as in https://github.com/ros-infrastructure/rep/pull/291), then you can use std::filesystem to do the implementation.

Does that all make sense?

[jdlangs]

Makes sense. For a Foxy compatible option, would it be appropriate to grab an existing 3rd party filesystem header library and rely on that before later switching it to the standard lib? This one seems fairly popular.

[clalancette]

Makes sense. For a Foxy compatible option, would it be appropriate to grab an existing 3rd party filesystem header library and rely on that before later switching it to the standard lib? This one seems fairly popular.

I have to say that I'm not enthusiastic about adding a new dependency that we'll have to vendor. While it is more work, could we just code the functionality into rcpputils::fs for Foxy?

[jdlangs]

a new dependency that we'll have to vendor

Is it off the table to simply include the single header file in the repo? If so, I'll just make a PR that uses std::filesystem directly for Galactic.

[wjwwood]

I'm not opposed to including a dependency like the one you linked to, personally, since those are designed to be easily incorporated into a project. But I do agree with @clalancette that it's more straight forward to fix this as narrowly as possible in foxy, due to testing and ABI concerns (like if the size of Path changed due to this vendoring, that would be an issue for backporting).

For newer versions, like rolling and galactic, having this more complete implementation as a fallback isn't a terrible idea, imo. However, I have some lingering concerns about it:

• There appears to be some differences from the official version w.r.t. unicode depending on the C++ version used:
  • https://github.com/gulrak/filesystem#differences-of-specific-interfaces
  • This may make using this to fix this specific issue insufficient?
• issues for users wanting to certify and/or test our code at a higher level
  • this basically boils down to: "more code == bad", so our implementation of a few features, while lacking, is easier to handle than the >6kloc from this project that was linked
• deciding when to update this dependency or remove it, in the future (on going maintenance burden)
  • though to be fair we have something similar already with our custom implementation

[jdlangs]

So I did some experimenting and the external filesystem library doesn't even build on UWP anyways, which is my original problem. So it would seem the best option is to just update to C++17 and std::filesystem for Galactic and not bother with backporting.