Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jul 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/advanced/cast/overview.rst
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ as arguments and return values, refer to the section on binding :ref:`classes`.
+------------------------------------+---------------------------+-------------------------------+
| ``std::variant<...>`` | Type-safe union (C++17) | :file:`pybind11/stl.h` |
+------------------------------------+---------------------------+-------------------------------+
| ``std::filesystem::path<T>`` | STL path (C++17) [#]_ | :file:`pybind11/stl.h` |
+------------------------------------+---------------------------+-------------------------------+
| ``std::function<...>`` | STL polymorphic function | :file:`pybind11/functional.h` |
+------------------------------------+---------------------------+-------------------------------+
| ``std::chrono::duration<...>`` | STL time duration | :file:`pybind11/chrono.h` |
Expand All @@ -163,3 +165,7 @@ as arguments and return values, refer to the section on binding :ref:`classes`.
+------------------------------------+---------------------------+-------------------------------+
| ``Eigen::SparseMatrix<...>`` | Eigen: sparse matrix | :file:`pybind11/eigen.h` |
+------------------------------------+---------------------------+-------------------------------+

.. [#] ``std::filesystem::path`` is converted to ``pathlib.Path`` and
``os.PathLike`` is converted to ``std::filesystem::path``, but this requires
Python 3.6 (for ``__fspath__`` support).
81 changes: 81 additions & 0 deletions include/pybind11/stl.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,21 @@
# include <variant>
# define PYBIND11_HAS_VARIANT 1
# endif
// std::filesystem::path
# if defined(PYBIND11_CPP17) && __has_include(<filesystem>) && \
PY_VERSION_HEX >= 0x03060000
EricCousineau-TRI marked this conversation as resolved.
Show resolved Hide resolved
# include <filesystem>
# define PYBIND11_HAS_FILESYSTEM 1
# endif
#elif defined(_MSC_VER) && defined(PYBIND11_CPP17)
# include <optional>
# include <variant>
# define PYBIND11_HAS_OPTIONAL 1
# define PYBIND11_HAS_VARIANT 1
# if PY_VERSION_HEX >= 0x03060000
# include <filesystem>
# define PYBIND11_HAS_FILESYSTEM 1
# endif
#endif

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
Expand Down Expand Up @@ -377,6 +387,77 @@ template <typename... Ts>
struct type_caster<std::variant<Ts...>> : variant_caster<std::variant<Ts...>> { };
#endif

#if defined(PYBIND11_HAS_FILESYSTEM)
template<typename T> struct path_caster {

private:
static PyObject* unicode_from_fs_native(const std::string& w) {
YannickJadoul marked this conversation as resolved.
Show resolved Hide resolved
#if !defined(PYPY_VERSION)
return PyUnicode_DecodeFSDefaultAndSize(w.c_str(), ssize_t(w.size()));
YannickJadoul marked this conversation as resolved.
Show resolved Hide resolved
#else
// PyPy mistakenly declares the first parameter as non-const.
return PyUnicode_DecodeFSDefaultAndSize(
const_cast<char*>(w.c_str()), ssize_t(w.size()));
#endif
}

static PyObject* unicode_from_fs_native(const std::wstring& w) {
return PyUnicode_FromWideChar(w.c_str(), ssize_t(w.size()));
}

public:
static handle cast(const T& path, return_value_policy, handle) {
if (auto py_str = unicode_from_fs_native(path.native())) {
return module::import("pathlib").attr("Path")(reinterpret_steal<object>(py_str))
.release();
}
return nullptr;
}

bool load(handle handle, bool) {
// PyUnicode_FSConverter and PyUnicode_FSDecoder normally take care of
// calling PyOS_FSPath themselves, but that's broken on PyPy (PyPy
// issue #3168) so we do it ourselves instead.
PyObject* buf = PyOS_FSPath(handle.ptr());
if (!buf) {
PyErr_Clear();
return false;
}
PyObject* native = nullptr;
if constexpr (std::is_same_v<typename T::value_type, char>) {
if (PyUnicode_FSConverter(buf, &native)) {
if (auto c_str = PyBytes_AsString(native)) {
// AsString returns a pointer to the internal buffer, which
// must not be free'd.
value = c_str;
}
}
} else if constexpr (std::is_same_v<typename T::value_type, wchar_t>) {
if (PyUnicode_FSDecoder(buf, &native)) {
if (auto c_str = PyUnicode_AsWideCharString(native, nullptr)) {
// AsWideCharString returns a new string that must be free'd.
value = c_str; // Copies the string.
PyMem_Free(c_str);
YannickJadoul marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Py_XDECREF(native);
Py_DECREF(buf);
if (PyErr_Occurred()) {
PyErr_Clear();
return false;
YannickJadoul marked this conversation as resolved.
Show resolved Hide resolved
} else {
return true;
}
}

PYBIND11_TYPE_CASTER(T, _("os.PathLike"));
};

template<> struct type_caster<std::filesystem::path>
: public path_caster<std::filesystem::path> {};
#endif

PYBIND11_NAMESPACE_END(detail)

inline std::ostream &operator<<(std::ostream &os, const handle &obj) {
Expand Down
37 changes: 37 additions & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,41 @@ if(Boost_FOUND)
endif()
endif()

# Check if we need to add -lstdc++fs or -lc++fs or nothing
if(MSVC)
EricCousineau-TRI marked this conversation as resolved.
Show resolved Hide resolved
set(STD_FS_NO_LIB_NEEDED TRUE)
else()
file(
WRITE ${CMAKE_CURRENT_BINARY_DIR}/main.cpp
"#include <filesystem>\nint main(int argc, char ** argv) {\n std::filesystem::path p(argv[0]);\n return p.string().length();\n}"
)
try_compile(
STD_FS_NO_LIB_NEEDED ${CMAKE_CURRENT_BINARY_DIR}
SOURCES ${CMAKE_CURRENT_BINARY_DIR}/main.cpp
COMPILE_DEFINITIONS -std=c++17)
try_compile(
STD_FS_NEEDS_STDCXXFS ${CMAKE_CURRENT_BINARY_DIR}
SOURCES ${CMAKE_CURRENT_BINARY_DIR}/main.cpp
COMPILE_DEFINITIONS -std=c++17
LINK_LIBRARIES stdc++fs)
try_compile(
STD_FS_NEEDS_CXXFS ${CMAKE_CURRENT_BINARY_DIR}
SOURCES ${CMAKE_CURRENT_BINARY_DIR}/main.cpp
COMPILE_DEFINITIONS -std=c++17
LINK_LIBRARIES c++fs)
endif()

if(${STD_FS_NEEDS_STDCXXFS})
set(STD_FS_LIB stdc++fs)
elseif(${STD_FS_NEEDS_CXXFS})
set(STD_FS_LIB c++fs)
elseif(${STD_FS_NO_LIB_NEEDED})
set(STD_FS_LIB "")
else()
message(WARNING "Unknown compiler - not passing -lstdc++fs")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@anntzer anntzer Dec 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@anntzer anntzer Dec 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

set(STD_FS_LIB "")
endif()

EricCousineau-TRI marked this conversation as resolved.
Show resolved Hide resolved
# Compile with compiler warnings turned on
function(pybind11_enable_warnings target_name)
if(MSVC)
Expand Down Expand Up @@ -357,6 +392,8 @@ foreach(target ${test_targets})
target_compile_definitions(${target} PRIVATE -DPYBIND11_TEST_BOOST)
endif()

target_link_libraries(${target} PRIVATE ${STD_FS_LIB})

# Always write the output file directly into the 'tests' directory (even on MSVC)
if(NOT CMAKE_LIBRARY_OUTPUT_DIRECTORY)
set_target_properties(${target} PROPERTIES LIBRARY_OUTPUT_DIRECTORY
Expand Down
6 changes: 6 additions & 0 deletions tests/test_stl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,12 @@ TEST_SUBMODULE(stl, m) {
.def("member_initialized", &opt_exp_holder::member_initialized);
#endif

#ifdef PYBIND11_HAS_FILESYSTEM
// test_fs_path
m.attr("has_filesystem") = true;
EricCousineau-TRI marked this conversation as resolved.
Show resolved Hide resolved
m.def("parent_path", [](const std::filesystem::path& p) { return p.parent_path(); });
#endif

#ifdef PYBIND11_HAS_VARIANT
static_assert(std::is_same<py::detail::variant_caster_visitor::result_type, py::handle>::value,
"visitor::result_type is required by boost::variant in C++11 mode");
Expand Down
19 changes: 19 additions & 0 deletions tests/test_stl.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,25 @@ def test_exp_optional():
assert holder.member_initialized()


@pytest.mark.skipif(not hasattr(m, "has_filesystem"), reason="no <filesystem>")
def test_fs_path():
from pathlib import Path

class PseudoStrPath:
def __fspath__(self):
return "foo/bar"

class PseudoBytesPath:
def __fspath__(self):
return b"foo/bar"

assert m.parent_path(Path("foo/bar")) == Path("foo")
YannickJadoul marked this conversation as resolved.
Show resolved Hide resolved
assert m.parent_path("foo/bar") == Path("foo")
assert m.parent_path(b"foo/bar") == Path("foo")
assert m.parent_path(PseudoStrPath()) == Path("foo")
assert m.parent_path(PseudoBytesPath()) == Path("foo")


@pytest.mark.skipif(not hasattr(m, "load_variant"), reason="no <variant>")
def test_variant(doc):
assert m.load_variant(1) == "int"
Expand Down