Add a std::filesystem::path <-> os.PathLike caster.

[anntzer]

Description

On C++17/Py3.6+, convert std::filesystem::path to pathlib.Path and os.PathLike to std::filesystem::path.

I don't know whether std::filesystem::path gets much usage, but pathlib.Path is certainly getting quite popular, so having a way to auto-handle it would be nice.

Suggested changelog entry:

``pybind11/stl/filesystem.h`` registers a type caster that, on C++17/Python 3.6+, converts ``std::filesystem::path`` to ``pathlib.Path`` and any ``os.PathLike`` to ``std::filesystem::path``.

[skoslowski]

I have been using such a caster for quite a while now and find it very useful.
Haven't had a chance to make a proper PR yet, here is my version skoslowski@767ac4d

Looking at this PR

• I really like that a pathlib.Path is returned (instead of str)
• using the os-native string types is probably more efficient compared to the utf8 API
• I am pretty sure the error state must be cleared before returning.

[anntzer]

I am pretty sure the error state must be cleared before returning.

Thanks for pointing this out, I was not aware of that. I can confirm this is needed e.g. to make variant casters work. OTOH it looks like the docs at https://pybind11.readthedocs.io/en/stable/advanced/cast/custom.html#custom-type-casters are wrong? The example load clearly returns !PyErr_Occurred()...

---

AFAICT the only relevant failing tests occur with versions of clang for which is it still necessary to link -lc++fs. I am a bit confused as to why, as the tests use clang 10 but https://libcxx.llvm.org/docs/UsingLibcxx.html#using-filesystem says it's only needed until clang 9. Any help on that side would be most appreciated (https://gitlab.kitware.com/cmake/cmake/-/issues/17834 is not very helpful...).

[anntzer]

Looks like PyPy is actually broken per https://foss.heptapod.net/pypy/pypy/-/issues/3168. I just ifdef'd it out for now...

[henryiii]

Without looking into it in detail, would it be best to put this into stlfs.h or something like that instead? I'm worried about it if it's not clearly opt-in, because most compilers made std::filesystem optional via an extra library, such as with -lstdc++fs, until pretty recently.

I'm a fan, just trying to avoid any issues with disruption; users should not be required to link to the filesystem lib if they don't use it. If it only requires linking with the filesystem if really is used, then ignore this and it can stay here.

[bstaletic]

because most compilers made std::filesystem optional via an extra library, such as with -lstdc++fs, until pretty recently.

Is clang 8 considered "pretty recent"? I guess it is.

I'm a fan, just trying to avoid any issues with disruption; users should not be required to link to the filesystem lib if they don't use it. If it only requires linking with the filesystem if really is used, then ignore this and it can stay here.

Templates aren't instantiated if they aren't actually used. This should PR should be safe, though I can't guarantee.

would it be best to put this into stlfs.h

It would definitely make stl.h not larger and people needing stlfs.h wouldn't need to include list_caster and similar. I was thinking about proposing moving the caster to a new header, but that's because I'm not a big fan of stl.h.

[YannickJadoul]

Without looking into it in detail, would it be best to put this into stlfs.h or something like that instead?

I was thinking about proposing moving the caster to a new header, but that's because I'm not a big fan of stl.h.

Yes, for exactly the reason @bstaletic mentions; I think that's a good one. This might also be the moment to think about another PR where optional is moved out of stl.h (and e.g. have stl_utils.h vs. stl_containers.h and a (soon-to-be-deprecated?) stl.h that just includes both), as @bstaletic has been requesting for a while? :-)

[bstaletic]

think about another PR where optional is moved out of stl.h

First attempt of that was 3 years ago in #1210.

[YannickJadoul]

think about another PR where optional is moved out of stl.h

First attempt of that was 3 years ago in #1210.

OK, so if extensions not using this don't need these special flags (as @henryiii said), this part could be ignored until a later PR? (I added a resolution of #1210 to the 2.7 milestone.)

[YannickJadoul]

An annoying question: is this something that should be in pybind11, rather than say ... an external plugin pybind11_filesystem? I was first skeptical about this, but the actual implementation is rather clean/small, so I guess I'm OK with this.
I'd like to see pybind11 evolve to a more "plugin-based" architecture, but I guess this specific feature isn't a huge burden to maintain?

I wanted to suggest thinking about pathlib2, for Python 2 (and PyPy 2, which isn't deprecated), but PyPy doesn't work, currently, ofc. And in Python 2 we still lack os.PathLike and __fspath__?

Next question: does this also accept strings? If not, should strings be accepted, maybe if convert == true in load? If they already are, a test on this would be nice.

Finally, a request: could this be turned into an abstraction like optional_caster? Basically, this would rename the current caster, and just have template <> struct type_caster<std::filesystem::path> : public path_caster<std::filesystem::path> {};.

This would allow for other C++ path-like type to quickly opt-in to this behavior, if they support a minimal interface (be constructible/assignable from std::string and having .native() or so).

[henryiii]

think about another PR where optional is moved out of stl.h

This is just about the first thing I thought of. I'd do it boost-style, where you have:

stl.h
stl/optional.h
stl/filesystem.h
stl/...

Then stl.h just includes all the most common items in stl/. But users can pick and choose now, it's no longer all or nothing. Better separation makes it easier to find what you need, too.

[YannickJadoul]

think about another PR where optional is moved out of stl.h

That's indeed exactly the proposal of #1210 (long outdated and full of conflicts by now, ofc), and yes, that seems perfect to me :-) But so maybe we can keep this PR in stl.h (if no extra flags/linking are/is needed to build stuff that's not using the new casters) and do this in in a follow-up PR?

[anntzer]

Thanks for the reviews.

• I personally don't like the splitting over multiple headers (if it was up to me everything in stdlib {Python,C++} would go into stl.h...) especially as (IIRC?) you get no warning until runtime if you forgot to include a header. Perhaps at least module.def could check at compile time that all relevant type casters are registered, and warn if not? In any case, that's a separate issue, and the agreement here seems to be to keep this in stl.h for this PR and possibly split it out later. At least I believe (in agreement with @bstaletic) that there's no need to link stdc++fs unless you actually use the functionality, so that specific point is not a problem.

• I switched to having a template path_caster, similar to optional_caster.

• str/bytes and PathLikes are fine, I added tests. On the other hand pathlib backports won't really work here given that the whole PR is based on using the newer fspath-based CPython APIs. If you want to support older versions, then you'd need to add fully separated codepaths for them, which I guess can be done but could be, well, a separate PR.

[YannickJadoul]

• I personally don't like the splitting over multiple headers (if it was up to me everything in stdlib {Python,C++} would go into stl.h...) especially as (IIRC?) you get no warning until runtime if you forgot to include a header. Perhaps at least module.def could check at compile time that all relevant type casters are registered, and warn if not? In any case, that's a separate issue, and the agreement here seems to be to keep this in stl.h for this PR and possibly split it out later. At least I believe (in agreement with @bstaletic) that there's no need to link stdc++fs unless you actually use the functionality, so that specific point is not a problem.

Indeed, let's keep this for another PR.
You're welcome to argue against it, then (I'll ping you, if I don't forget). But the current proposal would be to still keep a stl.h anyway (even if it'd just be for backwards compatibility). The thing is that the container casters are pretty invasive/dangerous, so I can see why people would not like to include those.

• I switched to having a template path_caster, similar to optional_caster.

Thanks, I think that looks good!

Potentially, T::value_type could be passed as a second template parameter rather than as dependent type, but let's see what @bstaletic & @henryiii think about that.

• str/bytes and PathLikes are fine, I added tests. On the other hand pathlib backports won't really work here given that the whole PR is based on using the newer fspath-based CPython APIs. If you want to support older versions, then you'd need to add fully separated codepaths for them, which I guess can be done but could be, well, a separate PR.

OK, fair enough; given that this is new functionality, I can live with not having this for Python 2 and 3.5, I think. Thanks for clarifying this further!

[bstaletic]

Potentially, T::value_type could be passed as a second template parameter

Personally, I don't see a need for that.

[anntzer]

I think the only relevant failure remaining is with clang10, which is missing an explicit linking to -lc++fs. Any suggestions on how to achieve that? (my cmake-fu is pretty weak...)

[YannickJadoul]

I think the only relevant failure remaining is with clang10, which is missing an explicit linking to -lc++fs. Any suggestions on how to achieve that? (my cmake-fu is pretty weak...)

target_link_libraries? I can't immediately find anything better/built-in into CMake, than https://gist.github.com/berkus/aafb15b06f753f025e009d5dd2ae7e5b. But @henryiii will probably know better.

[bstaletic]

with clang10, which is missing an explicit linking to -lc++fs

Uhmmm... -lc++fs has nothing to do with clang, but rather the version of the C++ standard library. So the question is which standard library is being picked up.

Either way, this is how I've solved the problem in my project.

[anntzer]

Thanks, looks like everything is working now :)

[bstaletic]

This is likely wrong. The try_compile failed in all three cases and we're going *shrug* "let's try it anyway!", so should probably be a fatal error.

[anntzer]

I don't think so? We could be on a compiler that doesn't support C++17 at all, in which case we'll be just fine, everything filesystem-related will be ifdef'd out anyways?

[bstaletic]

Good point. My project received complaints from users hitting this branch, but I unconditionally depend on <filesystem>. Maybe still wrap the whole block in an if ( standard is 17 ) or whatever the spelling is for cmake.

[anntzer]

Does this have to be done as part of this PR? TBH I'm not so comfortable with cmake...

[henryiii]

This can’t be done in CMake in general, too many ways someone could set the std level, though since it’s our tests we can do it. I can look at this later if it’s needed.

[EricCousineau-TRI]

I'll throw in peanut-gallery 2 cents: Looks good for a starting point! Can address acute defects after merge?

[anntzer]

Kindly bumping?

[anntzer]

rebased

[rwgk]

This also passed Google-internal global testing. Merging!

[henryiii]

I believe this broke support for building wheels that set anything less than macOS 10.15 (10.9 is the default, 10.12 is "safe", but 10.15 is painfully new). I thought I asked for this to be placed in a separate file?

See #2730 (comment)

[henryiii]

I have tried to add things like this before to CLI11, and it's really hard to get right, especially for features that depend on compiled libraries being linked (like libfs), since you can't detect that at header time. Putting this in a separate file would have avoided this and made it opt in.

[henryiii]

Example of a broken build: https://github.com/deepmind/open_spiel/runs/2999582108

[rwgk]

I believe this broke support for building wheels that set anything less than macOS 10.15 (10.9 is the default, 10.12 is "safe", but 10.15 is painfully new). I thought I asked for this to be placed in a separate file?

See #2730 (comment)

Sorry I missed that, with so many approvals I was thinking "What could possibly go wrong?"
Let's fix this asap.

[henryiii]

I clearly said exactly what could go wrong, it was just ignored. :P

[rwgk]

I told the open_spiel folks about this mishap.
Let me try a fix.

[henryiii]

Just pull it out into a file and make it opt-in by including the file, say stl/path.h. That's how all the contents of stl should be, with "safe" ones included in a "convenience" stl.h. Just because past organization wasn't ideal doesn't mean we shouldn't follow best practices for new code. Especially non-header-only additions to the STL.