Skip to content

Splitting out pybind11/stl/filesystem.h. #3077

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jul 8, 2021
Merged

Splitting out pybind11/stl/filesystem.h. #3077

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7b8c8f4
Splitting out pybind11/stl/filesystem.h.
rwgk Jul 6, 2021
9365a3a
Properly including new stl subdirectory in pip wheel config.
rwgk Jul 6, 2021
3e6b76a
iwyu cleanup.
rwgk Jul 6, 2021
bc84589
Adding PYBIND11_HAS_FILESYSTEM_IS_OPTIONAL.
rwgk Jul 6, 2021
b0f60fe
Eliminating else after return.
rwgk Jul 6, 2021
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
3 changes: 2 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ set(PYBIND11_HEADERS
include/pybind11/pybind11.h
include/pybind11/pytypes.h
include/pybind11/stl.h
include/pybind11/stl_bind.h)
include/pybind11/stl_bind.h
include/pybind11/stl/filesystem.h)

# Compare with grep and warn if mismatched
if(PYBIND11_MASTER_PROJECT AND NOT CMAKE_VERSION VERSION_LESS 3.12)
Expand Down
81 changes: 0 additions & 81 deletions include/pybind11/stl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,21 +41,11 @@
# include <variant>
# define PYBIND11_HAS_VARIANT 1
# endif
// std::filesystem::path
# if defined(PYBIND11_CPP17) && __has_include(<filesystem>) && \
PY_VERSION_HEX >= 0x03060000
# 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 @@ -387,77 +377,6 @@ 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) {
#if !defined(PYPY_VERSION)
return PyUnicode_DecodeFSDefaultAndSize(w.c_str(), ssize_t(w.size()));
#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);
}
}
}
Py_XDECREF(native);
Py_DECREF(buf);
if (PyErr_Occurred()) {
PyErr_Clear();
return false;
} 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
103 changes: 103 additions & 0 deletions include/pybind11/stl/filesystem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// Copyright (c) 2021 The Pybind Development Team.
// All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

#pragma once

#include "../cast.h"
#include "../pybind11.h"
#include "../pytypes.h"

#include "../detail/common.h"
#include "../detail/descr.h"

#include <string>

#ifdef __has_include
# if defined(PYBIND11_CPP17) && __has_include(<filesystem>) && \
PY_VERSION_HEX >= 0x03060000
# include <filesystem>
# define PYBIND11_HAS_FILESYSTEM 1
# endif
#endif
Comment on lines +16 to +22
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a thought, do we need this PYBIND11_HAS_FILESYSTEM now? If I include <stl/filesystem.h> do I want it to silently not cast things instead of getting a compile-time error?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(If not, it should be moved to the include in the test ;) )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(If not, it should be moved to the include in the test ;) )

Copying my question from the slack channel here:

In this first pass, I simply kept the #ifdef block defining PYBIND11_HAS_FILESYSTEM in the header file. Does that still make sense? Would it be better to move that to the test? — It seem useful to keep it in the header, so others can easily use it if available without having to do anything much otherwise, but I'm not sure how this shakes out best.

Ideally we'd give people a choice? Bail out with a compile-time error or have them handle the runtime error.

It's not really great if we lead users down the path of copy-pasting the __has_include logic from the test.

Copy link
Collaborator

@henryiii henryiii Jul 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

But do users want "works if available" behavior for this? (The answer may be yes) It seems a little weird if your package only accepts pathlib.Path arguments if it was compiled in C++17 mode on some compilers. And only allows you to return std::filesystem on Python 3 (though that I'm less worried about). I'm guessing there will be some extra work most users will have to do to make sure this works (including requiring macOS 10.15+), so forcing the user to explicit might be better.

That is, I expect most users will either make an effort to support std::filesystem/pathlib everywhere, or they will not support it, but very few will support it if available. But could easily be wrong. I don't find the "auto" selection of C++ level all that useful either (setuptools helpers), but some users want it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm okay to leave it as is, a user (who looks at the source) can check PYBIND11_HAS_FILESYSTEM if they want. But I'm also okay to (re)move it to the tests.

Copy link
Collaborator

@henryiii henryiii Jul 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

__has_include is not actually a C++17 feature, it's a C++20 one. Though I think all major compilers added it very early, but still, maybe it's best to remove this and just #include filesystem and let the failure happen if it's not available, rather than calling #error. This then makes PYBIND11_HAS_FILESYSTEM rather weird, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@henryiii This page suggest it is a C++17 feature though? https://en.cppreference.com/w/cpp/preprocessor/include

Copy link
Collaborator

Choose a reason for hiding this comment

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

official spec: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0061r1.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

__has_include currently appears in 10 source code locations on master (see below).

If think for this PR it's important to stay focused on the one problem we want to solve as soon as possible: users shouldn't be forced to work on the link options if they don't actually want to use the new caster. I'll update the PR description accordingly.

include/pybind11/cast.h:#  if defined(__has_include)
include/pybind11/cast.h:#    if __has_include(<string_view>)
include/pybind11/stl.h:#ifdef __has_include
include/pybind11/stl.h:#  if defined(PYBIND11_CPP17) && __has_include(<optional>)
include/pybind11/stl.h:#  if defined(PYBIND11_CPP14) && (__has_include(<experimental/optional>) && \
include/pybind11/stl.h:                                 !__has_include(<optional>))
include/pybind11/stl.h:#  if defined(PYBIND11_CPP17) && __has_include(<variant>)
include/pybind11/stl.h:#  if defined(PYBIND11_CPP17) && __has_include(<filesystem>) && \
include/pybind11/detail/common.h:#if defined(__has_include)
include/pybind11/detail/common.h:#  if __has_include(<version>)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, @Skylion007 is correct, I was muddling has_include with feature test macros - probably shouldn't comment on things while in a conference. :)

This plans seems fine, we need to fix this, then we can polish and or document a bit more later if needed.


#if !defined(PYBIND11_HAS_FILESYSTEM) && !defined(PYBIND11_HAS_FILESYSTEM_IS_OPTIONAL)
# error \
"#include <filesystem> is not available. (Use -DPYBIND11_HAS_FILESYSTEM_IS_OPTIONAL to ignore.)"
#endif

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
PYBIND11_NAMESPACE_BEGIN(detail)

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

private:
static PyObject* unicode_from_fs_native(const std::string& w) {
#if !defined(PYPY_VERSION)
return PyUnicode_DecodeFSDefaultAndSize(w.c_str(), ssize_t(w.size()));
#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);
}
}
}
Py_XDECREF(native);
Py_DECREF(buf);
if (PyErr_Occurred()) {
PyErr_Clear();
return false;
}
return true;
}

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

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

PYBIND11_NAMESPACE_END(detail)
PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
7 changes: 6 additions & 1 deletion tests/extra_python_package/test_files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@
"include/pybind11/detail/typeid.h",
}

stl_headers = {
"include/pybind11/stl/filesystem.h",
}

cmake_files = {
"share/cmake/pybind11/FindPythonLibsNew.cmake",
"share/cmake/pybind11/pybind11Common.cmake",
Expand All @@ -67,7 +71,7 @@
"setup_helpers.pyi",
}

headers = main_headers | detail_headers
headers = main_headers | detail_headers | stl_headers
src_files = headers | cmake_files
all_files = src_files | py_files

Expand All @@ -77,6 +81,7 @@
"pybind11/include",
"pybind11/include/pybind11",
"pybind11/include/pybind11/detail",
"pybind11/include/pybind11/stl",
"pybind11/share",
"pybind11/share/cmake",
"pybind11/share/cmake/pybind11",
Expand Down
5 changes: 5 additions & 0 deletions tests/test_stl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@
#include "constructor_stats.h"
#include <pybind11/stl.h>

#ifndef PYBIND11_HAS_FILESYSTEM_IS_OPTIONAL
#define PYBIND11_HAS_FILESYSTEM_IS_OPTIONAL
#endif
#include <pybind11/stl/filesystem.h>

#include <vector>
#include <string>

Expand Down
4 changes: 3 additions & 1 deletion tools/setup_global.py.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ class InstallHeadersNested(install_headers):

main_headers = glob.glob("pybind11/include/pybind11/*.h")
detail_headers = glob.glob("pybind11/include/pybind11/detail/*.h")
stl_headers = glob.glob("pybind11/include/pybind11/stl/*.h")
cmake_files = glob.glob("pybind11/share/cmake/pybind11/*.cmake")
headers = main_headers + detail_headers
headers = main_headers + detail_headers + stl_headers

cmdclass = {"install_headers": InstallHeadersNested}
$extra_cmd
Expand All @@ -58,6 +59,7 @@ setup(
(base + "share/cmake/pybind11", cmake_files),
(base + "include/pybind11", main_headers),
(base + "include/pybind11/detail", detail_headers),
(base + "include/pybind11/stl", stl_headers),
],
cmdclass=cmdclass,
)
2 changes: 2 additions & 0 deletions tools/setup_main.py.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@ setup(
"pybind11",
"pybind11.include.pybind11",
"pybind11.include.pybind11.detail",
"pybind11.include.pybind11.stl",
"pybind11.share.cmake.pybind11",
],
package_data={
"pybind11": ["py.typed", "*.pyi"],
"pybind11.include.pybind11": ["*.h"],
"pybind11.include.pybind11.detail": ["*.h"],
"pybind11.include.pybind11.stl": ["*.h"],
"pybind11.share.cmake.pybind11": ["*.cmake"],
},
extras_require={
Expand Down