fix: Working build with static library

[henryiii]

Fixes #5

The example as it stands does not work, and even has invalid Python syntax in it. This fixes it and does minor standardization in structure. The min version is CMake 3.14, which is fine because you can get 3.17 with pip on most platforms, and the old example required at least 3.11 anyway, even though it lied about it (besides not working).

I've added PEP 518 support, so it should work out of the box on a recent pip, no other requirements. Added a README.

I did not remove the configure and version code in CMake, even though a) it's overkill for a basic example, and b) it's not synchronized with the version in Python in any way, making it unnecessary in a Python example.

I'd like to make this a shared library and see if I can get it to work there too.

[henryiii]

This would allow shared libraries:

-add_library(hellocore ${hello_sources} )
+add_library(hellocore SHARED ${hello_sources} )
 target_include_directories(hellocore PUBLIC
   $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>
   $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
@@ -16,9 +16,9 @@ set_property(TARGET hellocore PROPERTY POSITION_INDEPENDENT_CODE ON)
 # install targets
 install(TARGETS hellocore
         EXPORT helloTargets
-        RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
-        LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
-        ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
+        RUNTIME DESTINATION src/hello
+        LIBRARY DESTINATION src/hello
+        ARCHIVE DESTINATION src/hello
         )
 install(DIRECTORY ${PROJECT_SOURCE_DIR}/include/
diff --git a/projects/hello-pybind11/src/hello/CMakeLists.txt b/projects/hello-pybind11/src/hello/CMakeLists.txt
index 19379f0..690729b 100644
--- a/projects/hello-pybind11/src/hello/CMakeLists.txt
+++ b/projects/hello-pybind11/src/hello/CMakeLists.txt
@@ -9,6 +9,12 @@ pybind11_add_module(${python_module_name} MODULE
   )
 target_link_libraries(${python_module_name} PRIVATE hellocore)
+if (APPLE)
+  set_target_properties(${python_module_name} PROPERTIES BUILD_WITH_INSTALL_RPATH TRUE INSTALL_RPATH "@loader_path")
+else()
+  set_target_properties(${python_module_name} PROPERTIES BUILD_RPATH_USE_ORIGIN TRUE)
+endif()
+
 install(TARGETS ${python_module_name} DESTINATION src/hello)

Is there a way to use the @rpath with an install into ${CMAKE_INSTALL_LIBDIR}? It doesn't seem that ${CMAKE_INSTALL_LIBDIR} is always accessible. Or maybe there's some other standard trick used here?

This may actually be the correct way to do it, with the only other alternative being the manual loading of the libraries required in __init__.py if you want to put these in a "shared" location. See https://stackoverflow.com/a/30941390/2402816

[ktbarrett]

Your RPATH handling code doesn't work in Linux. Try this.

if (APPLE)
  set(rpath "@loader_path")
else()
  set(rpath "$ORIGIN")
endif()
set_target_properties(${python_module_name} PROPERTIES INSTALL_RPATH ${rpath})

I don't think the other RPATH related options are necessary. The build RPATH will be hardcoded, but we don't really care, ctest will still run, so we don't need BUILD_WITH_INSTALL_RPATH or BUILD_RPATH_USE_ORIGIN. We set the INSTALL_RPATH which is the only thing necessary to get the installed module to work.

I wonder if we need to anything to support Windows...

[ktbarrett]

What you provided does work, but I think there could be more radical changes.

In my mind there are two possible example projects:

1. Python package
   • does not install any headers or cmake files
2. Independent C++ project with Python bindings and packaging
   • running cmake will build just the C++ library (not the bindings) and install libs, headers, and cmake files to the system
   • running with sckit-build defines SKBUILD, which is used to turn on the binding build, turns off installation of headers and cmake files, and handles anything else to install all assets into the package.

What was here before seems like it was trying to do option 2. You seem more fond of option 1. I'd pick one and stick to it, and we can develop the other at some point in the future.

Additional, I don't think the directory structure in this example is what we want people using.

A saner structure for option 1 is below, which looks a lot like the regular C version.

.
├── cmake
│   └── helloConfig.cmake.in
├── CMakeLists.txt
├── pyproject.toml
├── hello
│   ├── CMakeLists.txt
│   ├── hello.cpp
│   └── __init__.py
├── tests
│   └── test_hello_pybind11.py
├── README.md
├── setup.py
└── src
    ├── CMakeLists.txt
    └── hello.cpp

A saner structure that I've seen for option 2.

.
├── cmake
│   └── helloConfig.cmake.in
├── CMakeLists.txt
├── include
│   └── hello.h
├── pyproject.toml
├── python
│   ├── hello
│   │   ├── CMakeLists.txt
│   │   ├── hello_py.cpp
│   │   └── __init__.py
│   └── tests
│       └── test_hello_pybind11.py
├── README.md
├── setup.py
└── src
    ├── CMakeLists.txt
    └── hello.cpp

[henryiii]

Your RPATH handling code doesn't work in Linux

Thanks! I was originally running in a docker container, but when I did the shared lib testing, I was just on macOS directly.

more radical changes

I was mostly focused on just getting this to work, but fully agree, there are two paths, and currently we seem to be somewhere in the middle. I think option 1 is what is more "hello-world" like, and what would the the most helpful for a newcomer. Option 2 is also very useful to have, though, but more complex and tricky. Should you install CMake files, for example? I'd rather think you should, so a user could access the installed package with CMake. See PyBind11, which does not install cmake files with the package, and therefore can't be accessed from CMake from the Python install.

I could duplicate the folder then do one for each? Focusing on option 1 first?

A saner structure for option 1

You really need to have at least one level of nesting for the python folder. While there are a few (2?) people who like flat over nested for simple projects, I don't think it's even a discussion for packages that have compiled pieces - you have to keep the package folder out of the top level. If you have it at the top level, then tools like pytest cannot be run from the top level directory reliably, since they may pick up <package>/__init__.py instead of the installed package (depending on the run method), and unless you build inline, this is the wrong one to use. I like using src/<package>, which puts it alongside the C++ code, but python/<package> is another option.

@ktbarrett I can give you access to my fork if you'd like to help.

[ktbarrett]

See PyBind11, which does not install cmake files with the package, and therefore can't be accessed from CMake from the Python install.

I ran into this recently, it's quite annoying.

If you have it at the top level, then tools like pytest cannot be run from the top level directory reliably, since they may pick up /init.py instead of the installed package

I've also run into this recent as well; you've convinced me. Certainly option 1 is the best place to start, and I'm willing to help.

[thewtex]

LGTM

[thewtex]

@henryiii very nice!

@phcerdan @jcfr

[henryiii]

@henryiii very nice!

@ktbarrett did just as much to clean it up further, if not more. :)

[phcerdan]

I am too late for the review since @thewtex pinged me 3 hours ago, and I wasn't watching this repo.
Thanks a lot for greatly improving the pybind11 example @henryiii and @ktbarrett. This example was extracted at the early stages of a c++ library with pybind11 wrappings that I maintain: sgext. And it definitely falls in the option 2 you were commenting before.

There are a couple of things that I think were not "too extra" to have. We disagree on what is the sweet point in complexity for an example, but it's completely fine for me if you want to simplify it, especially if you fix the bug importing the library doing so!

From: baca405
pybind11 example: remove ctest support .
To the rhetorical question: This is supposed to be a Python extension module, why are we using ctest?
ctest is the CMake way to run tests, it can run c-tests, c++-tests, fortran-tests, python-tests, you name it. I find it great to run all my tests with one command: ctest -V. You can still add tox or whatever extra python library you want on top.

From: 9059c15
pybind11 example: merge init and body files.
Splitting the compilation of source files is recommended in pybind11: how-can-i-reduce-the-build-time. When you have more than ten source files, you really want to compile them in parallel. Not sure what is the desired ratio of easy to digest versus technical dept for the future.

About not exporting, and not installing targets... well, I thought scikit-build was mainly a CMake glue with setuptools. And exporting and installing targets is a must do in any CMake library. I understand there are cases where you only would use C++ files to speed up some portions of your python package, but then, I would recommend using the hello-cython example instead of pybind11. Or the user can always remove those CMake lines, but I don't see how they are "too much".

But, as I said earlier, thanks for fixing this!

[henryiii]

I didn't notice the merging of init and body, I would rather have them separate since that's really how projects are supposed to be designed with PyBind11.

I think the exporting and installing targets was mostly talking about adding shared/static libraries and then, if you don't expect someone to link (without Python) to your libraries in your wheel, you don't need it. I do want to have a new, separate example that has the complex parts of this done correctly (but is not broken). I think by splitting it into "simple, just shows the important parts" and "here's what a real-world-like example looks like" would be ideal. I (or @ktbarrett) could start a PR in the near future with a advanced-pybind11 example. I'm also hoping to improve the CMake support in PyBind11 in the near future too. :)

[phcerdan]

Not sure if the strategy to trim down a more complete example to a "simple" version, to then wait for you or @ktbarrett to start a PR in the near future with an advanced-pybind11 is a sustainable approach. Fixing bugs and broken examples is always great though.

If you don't need static libraries in your wheel, you can use this PR: Add cmake_target option on scikit-build, or a manifest introduced in this PR to exclude them. That could be introduced in the advanced-pybind11.

[henryiii]

Well, my PR originally mostly fixed things, and that sat here for exactly a month. Then I merged @ktbarrett's simplifications and it got merged in a day. ;)

I think an example labeled "hello" tends to imply a simple example, there needs to be a simple example, and the complex one had lots of issues, so having something correct. Also, if you are writing a binding to your own library or a header only library, you build the target yourself, so have full control over what is produced (but the two PRs above are fantastic to have!). I'll be revisiting scikit-build integration soon, after working on PyBind11's CMake. I can take the first commit, maybe add 1-2 of the non-simplification changes, rename it, and put that into a PR for the time being, then just update it later when needed, would that be more sustainable?

[henryiii]

Though, if we wait, @ktbarrett may have a better example (and I might be able to fix the missing CMake config files issue in PyBind11)

[ktbarrett]

@phcerdan With the simple example, the idea was that the extension module - and by extension (:rofl:) the choice of pybind11 and scikit-build for building that extension module - is an implementation detail. In that case distributing things unrelated to the use of the Python library is unfitting of such an example. This mirrors the other examples in this repo that don't "appear" as anything more than Python packages. I believe the first pybind11 example in the project should be similar to them and not a totally different project architecture. Such a project architecture is not typical either, in fact it's fairly unsupported. At least that was my train of thought.

Certainly there are use cases where supporting both the C++ and the Python wrapper in the same project are advantageous, and pip/pypi can be used as an effective distribution for both Python libraries and C libraries. I am working (well eventually) on such a project. I have a mostly finalized an example project that explicitly does this. Whether I move that example into this repo (and maybe remove some bits), or fix and rename this project from the 2nd or 3rd commit, I don't particularly mind.

Not sure if the strategy to trim down a more complete example to a "simple" version, to then wait for you or @ktbarrett to start a PR in the near future with an advanced-pybind11 is a sustainable approach.

In what way?

@henryiii

I might be able to fix the missing CMake config files issue in PyBind11

That would be awesome. It also might be advantageous for CMake as distributed by SciKit to default searching the Python package installation prefix for CMake packages. Or should they go in share/? There are still some best practices related to this method of distribution to work out.

[thewtex]

@phcerdan I apologize for merging so quickly -- I should have provided more time for your review. 😞 @henryiii @ktbarrett hopefully you can help to keep us moving forward and address the issues @phcerdan mentioned either in improvements to this example or an advanced-pybind11.

Regarding CMake configuration exports and C++ header installations; yes, there is work to be done in Python wheels and CMake discovery to make these useful. However, scikit-build + conda should also work for generating a conda package. The headers in a conda package go into their standard platform locations. CMake can then find them with standard methods for building against a project. There are also tools like conda-press that can help with the distribution of shared library dependencies.

Maybe an advance-pybind11 example would help. Additionally, there is an effort to auto-generate pybind11 bindings, autopybind11. This would make another good example for a larger library.

after working on PyBind11's CMake.

@henryiii this sounds quite helpful all around!

[phcerdan]

No worries at all @thewtex, thanks for pointing to autopybind11, looks good!