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

rwgk · 2021-07-06T19:15:46Z

Unfortunately PR #2730 forces users of pybind11/stl.h to link against -lstdc++fs (or similar, depending on the platform) even if they don't actually use the new type caster. This issue is resolved here by moving the new type caster to pybind11/stl/filesystem.h. The std::filessytem-related link options will be needed only if the new header is included.

Further refinements of the cmake files (see PR comments) is beyond the scope of this PR and left for follow-on PRs.

Example breakage due to PR #2730: https://github.com/deepmind/open_spiel/runs/2999582108

This PR is mostly following the suggestion here: #2730 (comment)

Except using pybind11/stl/filesystem.h instead of pybind11/stlfs.h, as decided via chat.

stl.h restored to the exact state before merging PR #2730 via:

git checkout 733f8de24feed964f96b639a0a44247f46bed868 stl.h

NOTE: The #2730 change log entry was edited after this PR was merged.

To solve breakages like: https://github.com/deepmind/open_spiel/runs/2999582108 Mostly following the suggestion here: pybind#2730 (comment) Except using pybind11/stl/filesystem.h instead of pybind11/stlfs.h, as decided via chat. stl.h restored to the exact state before merging PR pybind#2730 via: ``` git checkout 733f8de stl.h ```

henryiii · 2021-07-06T19:20:38Z

include/pybind11/stl/filesystem.h

+#ifdef __has_include
+#  if defined(PYBIND11_CPP17) && __has_include(<filesystem>) && \
+      PY_VERSION_HEX >= 0x03060000
+#    include <filesystem>
+#    define PYBIND11_HAS_FILESYSTEM 1
+#  endif
+#endif


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?

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

(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.

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.

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.

__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.

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

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

__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>)

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.

Skylion007 · 2021-07-06T19:21:38Z

include/pybind11/stl/filesystem.h

+        if (PyErr_Occurred()) {
+            PyErr_Clear();
+            return false;
+        } else {


else after return is not ideal style.

Yes, it's called "guard" style to put the short returning branch at the top without an else, and it's preferable.

@henryiii Plan on enabling a clang-tidy check for this soon.

else after return is not ideal style.

Done.

Skylion007 · 2021-07-06T19:24:45Z

Need to fix some of the pip tests.

henryiii · 2021-07-06T19:25:19Z

There's a place or two where this needs to be added to the Python build system too. I'd do a quick git grep stl.h or maybe a rarer file and see where there are lists. Possibly only in tests, maybe in setup.py somewhere.

Skylion007

Edit the lists as @henryiii suggested.

Skylion007 · 2021-07-06T19:25:47Z

include/pybind11/stl/filesystem.h

+        if (PyErr_Occurred()) {
+            PyErr_Clear();
+            return false;
+        } else {


@henryiii Plan on enabling a clang-tidy check for this soon.

rwgk · 2021-07-06T19:40:04Z

There's a place or two where this needs to be added to the Python build system too. I'd do a quick git grep stl.h or maybe a rarer file and see where there are lists. Possibly only in tests, maybe in setup.py somewhere.

I tried that already but apparently I need to do something more involved because stl is a new subdirectory. Working on it.

henryiii · 2021-07-06T19:40:37Z

One Python 2 failure is our random known flake, the other is just the packaging test (which happens to be Python 2 as well), which tests the file list.

We already have some directories (like detail) so whatever we do for that should work?

This now passes interactively: ``` pytest tests/extra_python_package/ ```

rwgk · 2021-07-06T20:26:38Z

One Python 2 failure is our random known flake, the other is just the packaging test (which happens to be Python 2 as well), which tests the file list.

We already have some directories (like detail) so whatever we do for that should work?

No good deed gets unpunished ... I think I finally figured it out. (I overlooked the tools directory longer than I'd like to admit.)

iwyuh.py -c -std=c++17 -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/include/python3.9 -I/usr/include/eigen3 include/pybind11/stl/filesystem.h

rwgk · 2021-07-06T23:14:41Z

Happy day, the mac os straggler made it :-)

@henryiii I hope this PR is final now, could you please review and approve if this looks good to you?

jagerman · 2021-07-07T15:01:44Z

I don't think any of the CI's are going to catch this, but with clang before 9 and gcc before 9 (or any clang with stdlibc++ before before 9) you also need to link with -lstdc++fs (stdlibc++) or -lc++fs (clang+llvm). Might be worth documenting that, or perhaps providing some opt-in cmake machinery to help with setting up the linking?

https://github.com/oxen-io/lokinet/blob/dev/cmake/check_for_std_filesystem.cmake

is some cmake code we wrote to deal with figuring out the linking mess.

Skylion007 · 2021-07-07T15:03:44Z

There is a cmake community module for FileSystem: https://github.com/vector-of-bool/CMakeCM/blob/master/modules/FindFilesystem.cmake

henryiii · 2021-07-07T15:20:33Z

Might be worth documenting that, or perhaps providing some opt-in cmake machinery to help with setting up the linking?

This is why I liked just making it an error if pybind11/std/filesystem.h is included but the filesystem library is not available, either due to linking requirements, macOS target settings, or for other reasons. Having a "try to use it if available" makes us responsible for deciding that correctly (which is hard), and for providing tools to make it optionally available elsewhere. The current, undocumented, tiny bit of code that enables this is not too bad, since we aren't advertising it.

It's not our responsibility to provide support for filesystem for the user, just for allowing conversion if a user already supports it. IMO. But docs on how to use it, including links to CMake modules that do the linking, etc, are fine and useful. (and the above caveats).

rwgk · 2021-07-07T17:16:45Z

Having a "try to use it if available" makes us responsible for deciding that correctly (which is hard),

Yes exactly. This is the issue meant to be solved by this PR.

It's not our responsibility to provide support for filesystem for the user, just for allowing conversion if a user already supports it. IMO. But docs on how to use it, including links to CMake modules that do the linking, etc, are fine and useful. (and the above caveats).

These could be nice follow-on PRs.

rwgk requested a review from henryiii as a code owner July 6, 2021 19:15

henryiii reviewed Jul 6, 2021

View reviewed changes

Skylion007 reviewed Jul 6, 2021

View reviewed changes

Skylion007 approved these changes Jul 6, 2021

View reviewed changes

Skylion007 reviewed Jul 6, 2021

View reviewed changes

Properly including new stl subdirectory in pip wheel config.

9365a3a

This now passes interactively: ``` pytest tests/extra_python_package/ ```

rwgk added 3 commits July 6, 2021 13:37

iwyu cleanup.

3e6b76a

iwyuh.py -c -std=c++17 -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/include/python3.9 -I/usr/include/eigen3 include/pybind11/stl/filesystem.h

Adding PYBIND11_HAS_FILESYSTEM_IS_OPTIONAL.

bc84589

Eliminating else after return.

b0f60fe

henryiii approved these changes Jul 8, 2021

View reviewed changes

rwgk merged commit 6d1b197 into pybind:master Jul 8, 2021

github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 8, 2021

rwgk deleted the stlfs branch July 8, 2021 16:03

rwgk removed the needs changelog Possibly needs a changelog entry label Jul 8, 2021

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rwgk commented Jul 6, 2021 •

edited

Loading

henryiii Jul 6, 2021

henryiii Jul 6, 2021

rwgk Jul 6, 2021

henryiii Jul 6, 2021 •

edited

Loading

henryiii Jul 6, 2021

henryiii Jul 7, 2021 •

edited

Loading

Skylion007 Jul 7, 2021

Skylion007 Jul 7, 2021

rwgk Jul 7, 2021

henryiii Jul 8, 2021

Skylion007 Jul 6, 2021

henryiii Jul 6, 2021

Skylion007 Jul 6, 2021

rwgk Jul 6, 2021

Skylion007 commented Jul 6, 2021

henryiii commented Jul 6, 2021

Skylion007 left a comment •

edited

Loading

Skylion007 Jul 6, 2021

rwgk commented Jul 6, 2021

henryiii commented Jul 6, 2021

rwgk commented Jul 6, 2021

rwgk commented Jul 6, 2021

jagerman commented Jul 7, 2021

Skylion007 commented Jul 7, 2021

henryiii commented Jul 7, 2021 •

edited

Loading

rwgk commented Jul 7, 2021

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

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

Conversation

rwgk commented Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skylion007 commented Jul 6, 2021

henryiii commented Jul 6, 2021

Skylion007 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk commented Jul 6, 2021

henryiii commented Jul 6, 2021

rwgk commented Jul 6, 2021

rwgk commented Jul 6, 2021

jagerman commented Jul 7, 2021

Skylion007 commented Jul 7, 2021

henryiii commented Jul 7, 2021 • edited Loading

rwgk commented Jul 7, 2021

rwgk commented Jul 6, 2021 •

edited

Loading

henryiii Jul 6, 2021 •

edited

Loading

henryiii Jul 7, 2021 •

edited

Loading

Skylion007 left a comment •

edited

Loading

henryiii commented Jul 7, 2021 •

edited

Loading