export interface lib through pybind11Config.cmake
#506
Conversation
|
Quick comment: Could you please separate out the ICPC patch, since it is not related to cmake? |
|
Sure thing, done. |
There was a problem hiding this comment.
Nice. This should be quite useful. I left some smaller comments in the code. But I think the main thing to work out is the CMake version compatibility and testing.
How certain is the CMake > v3.0.1 requirement? I'm not that familiar with this part of CMake, but I seem to remember that exports work in v2.8.12. However, I think there are some issues with defining header-only libraries on versions older than 3.1.0.
It would be very good to set up a minimal build test which can be invoked from pytest. The various configurations on Travis and AppVeyor have both CMake v2.8.12 and > v3.0. It would also make sure everything works cross-platform. A subdirectory could be added to tests containing a CMakeLists.txt and a simple cpp source file. A pytest function could run the shell commands to build it and make sure everything checks out.
| "${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Config.cmake" | ||
| INSTALL_DESTINATION ${CMAKECONFIG_INSTALL_DIR}) | ||
| write_basic_package_version_file(${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}ConfigVersion.cmake | ||
| VERSION 1.8 |
There was a problem hiding this comment.
It would be nice to parse the version number from common.h:L55-L57 to avoid duplication and possible mismatches.
There was a problem hiding this comment.
(And this is already a mismatch: current master version is 1.9.dev0).
@loriab - this should work to extract it:
file(STRINGS "${PYBIND11_INCLUDE_DIR}/pybind11/common.h" pybind11_version_defines
REGEX "#define PYBIND11_VERSION_(MAJOR|MINOR|PATCH) ")
foreach(ver ${pybind11_version_defines})
if (ver MATCHES "#define PYBIND11_VERSION_(MAJOR|MINOR|PATCH) +([^ ]+)$")
set(PYBIND11_VERSION_${CMAKE_MATCH_1} "${CMAKE_MATCH_2}" CACHE INTERNAL "")
endif()
endforeach()
message(STATUS "pybind11 version ${PYBIND11_VERSION_MAJOR}.${PYBIND11_VERSION_MINOR}.${PYBIND11_VERSION_PATCH}")| # PYBIND11 cmake module. | ||
| # This module sets the following variables in your project: | ||
| # | ||
| # :: |
There was a problem hiding this comment.
Any reason not to include this on the last text line? ... in your project::
There was a problem hiding this comment.
No reason at all except that I copied the FindZLIB.cmake long ago and never noticed the formatting error. Good point.
| # | ||
| # pybind11::pybind11 - the main pybind11 interface library (i.e., headers) | ||
| # | ||
| # find_package(PythonLibsNew REQUIRED) |
There was a problem hiding this comment.
FindPythonLibsNew is not a standard CMake module and it's unlikely that users will have it. It should be added to the install files along with the pybind11 config package. That would be pretty convenient.
There was a problem hiding this comment.
Yep, and it looks like our version will even have some modifications.
Just to clarify: the gcc 4.8 builds are 2.8.12, while the gcc 6, OS X, and MSVC builds are all currently on 3.6.2 (and will probably get 3.7.0 soon). So we don't actually have much in the middle: just the three-year old 2.8.12, and the two-month old 3.6.2, in case that matters. |
|
On the CMake version, testing locally on what I could find, 2.8.11 didn't work and 3.0.1 did (and if I recall correctly, the installed Configs matched my 3.3-generated ones). So when the Travis tests failed (first two and barebones), I concluded 2.8.12 wasn't safe and just excluded it. I think that leaves only 3.0.0 uncertain. Thanks for all the comments. I'll see about patching them up. |
Yeah, those are the only ones with old cmake. |
|
A general question: I'm wondering: what is the benefit (if any) to using It doesn't seem to me that this is really a question about size of the project. For instance, Clang/LLVM is a very big project, but it doesn't use |
|
(I'm also not sure what seems wrong with |
|
We (the Psi4 project) find Admittedly, I've never dealt with any non-GnuMake backends to CMake, but having converted to a CMake superbuild scheme (thanks to @bennybp), haven't had any race condition problems. Regarding |
|
CMake has (The superbuild method actually separates |
|
It should also be possible to add
I'm not sure I understand this. |
|
Thanks, @dean0x7d, for expressing more clearly than I was that the Config file is primarily to locate the pybind11 installation. We could disallow detection of pre-built pybind11 and instead build internally so that we always know were the pybind headers were to be found and thus didn't require either
Looking again, I think I agree the add_module could be used by us, if it was accessible from the installed pybind. Some low barriers are (1) right now we use the Thanks for the suggestion of adding |
|
1.) ExternalProjects come in handy if you have very different packages with different building mechanisms, build flags, different languages, etc. Unfortunately, this often happens in science (for example, the external project may run a code generator first). In addition, it can help keep projects separate during the build, keeping them from inheriting incompatible build flags, etc. 2.) This feels like pybind11 is becoming the top project, whereas it really should be treated as a header library. Also, if there was another library that had its own function like add_module, a project couldn't use both. |
|
Personally, I think it would be useful to have both: Slightly off topic, but note that it would be pretty easy to add options to add_library(<name> [STATIC | SHARED | MODULE] [EXCLUDE_FROM_ALL] source1 [source2 ...])we could have something similar: pybind11_add_module(<name> [MODULE | SHARED] [LTO] [EXCLUDE_FROM_ALL] source1 [source2 ...])
(2) is not a problem.
I think there might be some misunderstanding of the extent of cuda_add_library(foo ${cuda_sources})
pybind11_add_library(bar ${binding_sources})
target_link_libraries(bar PRIVATE foo) |
|
Thanks, @jagerman, for the versioning snippet. CMake isn't very flexible in its allowed versions, and it can't do version comparison with the string "dev0" patch level (at least not without additional training). With allowing CMake to write its own (non-customized) ConfigVersion.cmake file, versioned package detection is working pretty much as expected: sensible for MAJOR.minor, unaware of pre- or dev-releases, unable to cope with strings. for an installation of
|
|
That looks perfectly fine to me. The |
| # defs because (1) that's really the domain of the upstream project to decide, | ||
| # (2) we can get away with it since installation (headers) don't care about | ||
| # python version or C++ standard, and (3) this will make the easily target relocatable. | ||
| add_library(pb11 INTERFACE) |
There was a problem hiding this comment.
Can't we call it pybind11? pb11 seems a bit obscure to me.
There was a problem hiding this comment.
Sure, done. It was just a dummy name so I didn't claim pybind11 as a target name that you might have wanted to use.
| set_target_properties(pb11 PROPERTIES EXPORT_NAME "pybind11") | ||
| target_include_directories(pb11 INTERFACE $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>) | ||
| if(APPLE) | ||
| target_link_libraries(pb11 INTERFACE "-undefined dynamic_lookup") |
There was a problem hiding this comment.
It's weird to add linker flags for APPLE here but then not link to the Python library on WIN32/64.
There was a problem hiding this comment.
That can't be done exactly here because we don't want paths to system libraries embedded in the ConfigTarget*.cmake file. If it's going to be done, Python will need to be redetected in the pybind11Config.cmake and appended there. See full comment for more on this possibility.
|
I had a more careful look at this PR now. I think the interface library approach is problematic because it just does a small subset of the things that are needed to ensure good behavior when linking pybind11 modules across various platforms. In practice, users will need to add a number of flags in addition to using the interface library, essentially replicating the contents of I suggest the following course of action:
|
|
The recently added commits mostly address comments before this morning. Sorry about the mac-psinet user authorship – I forgot the defaults on that computer, and I daresay this history will all be overwritten. Current state of the code
Comments on this morning's comments
|
|
Just a quick comment about test execution: In my original suggestion for pytest integration, I forgot about the need to forward CMake variables. Rather than pytest or bash, this could be done purely with CMake scripting. Let me just get to a computer to test this -- I have something in mind, I'll get back to you with a CMake code snippet a little later today. |
|
The most recent two commits are an unpolished implementation of having
There are now two additional tests (still bash-driven). |
|
Regarding test execution, the following CMake code should do the job nicely: add_custom_target(test_install
COMMAND ${CMAKE_COMMAND}
"-DCMAKE_INSTALL_PREFIX=${PROJECT_BINARY_DIR}/test_install"
-P "${PROJECT_BINARY_DIR}/cmake_install.cmake"
COMMAND ${CMAKE_CTEST_COMMAND}
--build-and-test "${CMAKE_CURRENT_SOURCE_DIR}/test_cmake_target"
"${CMAKE_CURRENT_BINARY_DIR}/test_cmake_target"
--build-noclean
--build-generator ${CMAKE_GENERATOR}
$<$<BOOL:${CMAKE_GENERATOR_PLATFORM}>:--build-generator-platform> ${CMAKE_GENERATOR_PLATFORM}
--build-makeprogram ${CMAKE_MAKE_PROGRAM}
--build-options "-DCMAKE_PREFIX_PATH=${PROJECT_BINARY_DIR}/test_install"
"-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}"
"-DPYTHON_EXECUTABLE=${PYTHON_EXECUTABLE}"
"-DPYBIND11_CPP_STANDARD=${PYBIND11_CPP_STANDARD}"
)The first command is essentially The second command uses I recommend giving this test its own target rather than attaching to |
There was a problem hiding this comment.
The approach with tools/pybind11Tools.cmake looks great. I left a few comments. It's a bit hard to look through the full changes right now. I'll do another pass when it's polished up a bit. After converting the bash test to CMake all the CI platforms should also pass.
| @@ -0,0 +1,48 @@ | |||
| // C++ file taken from https://github.com/pybind/cmake_example/blob/master/src/main.cpp | |||
There was a problem hiding this comment.
You can simplify the test cpp file to just:
#include <pybind11/pybind11.h>
namespace py = pybind11;
PYBIND11_PLUGIN(test_installed_target) {
py::module m("test_installed_target");
m.def("add", []() { return i + j; });
return m.ptr();
}Nothing else is really needed.
| @@ -17,32 +17,32 @@ endif() | |||
|
|
|||
| option(PYBIND11_INSTALL "Install pybind11 header files?" ${PYBIND11_MASTER_PROJECT}) | |||
| option(PYBIND11_TEST "Build pybind11 test suite?" ${PYBIND11_MASTER_PROJECT}) | |||
| option(PYBIND11_WERROR "Report all warnings as errors" OFF) | |||
There was a problem hiding this comment.
This option and the related pybind11_enable_warnings function should stay in the main CMakeLists.txt file. It's not for export.
| # (2) we can get away with it since installation (headers) don't care about | ||
| # python version or C++ standard, and (3) this will make the easily target relocatable. | ||
| add_library(pybind11 INTERFACE) | ||
| target_include_directories(pybind11 INTERFACE $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>) |
There was a problem hiding this comment.
Include $<BUILD_INTERFACE:${PYBIND11_INCLUDE_DIR}> so that the target is also usable with add_subdirectory.
|
The error on AppVeyor is because MSVC outputs the extension module in a subdirectory (
add_custom_target(check ${CMAKE_COMMAND} -E env PYTHONPATH=$<TARGET_FILE_DIR:test_installed_module>
${PYTHON_EXECUTABLE} ${PROJECT_SOURCE_DIR}/test.py)
|
| @@ -153,6 +45,8 @@ function(pybind11_enable_warnings target_name) | |||
| endif() | |||
| endfunction() | |||
|
|
|||
| include(pybind11Tools) | |||
There was a problem hiding this comment.
This include needs to happen before the variables are cached (line 24), otherwise it will break add_subdirectory in parent projects.
|
Thanks to tremendous help from @dean0x7d, testing of installed pybind11 is thoroughly passing (though the 2.8.12 cases aren't being tested as much as they could be). I think I've addressed other requests except for removing the interface library (which I still think is good CMake practice and useful to projects) and adding docs (which will be the last step). Let me know if anything looks amiss. |
|
I think the target makes a lot more sense now that the Python headers and library are also included. The target should also be useful in the future to support embedding the Python interpreter ( My only remaining questions:
Other than these nitpicks, the rest looks good to me. In light of these changes, I'd like to make some tweaks to (There was also mention of making |
Glad the target can stay. We (Psi4) actually were embedding Python when we first switched to pybind11 and started using the target in our fork. It should be easy enough to export an extra target that will (at
Yes, it's redundant. I include those sections out of habit, as a check that the installation hasn't been tampered with, and as a guide to what a FindProject.cmake would do, but your expression is equivalent. Removed.
My understanding of the convention is that singular
Once I realized you could temporarily set build type Added some basic docs. When this gets closer to acceptance, let me know if you'd like a rebase and history cleanup, or just a rebase, or whatever. |
|
@KerstinKeller That's a good point about @loriab I see pybind11Tools is already included in the config, but I'm guessing the extra include is required because of variable scope. In that case, the cache variables should just be moved from If |
|
Thanks, @KerstinKeller, I'm glad this may be useful beyond the two project I know of.
The Travis failure seems to be with Sphinx (error below). I don't think I'm the originator. Maybe this PR needs a rebase? |
|
For Don't worry about the sphinx error. It originates from the newly released |
There was a problem hiding this comment.
Hi,
I did a pass and left a few more comments.
Overall, I still think that the proposed interface library somewhat dangerous. It misses important compilation flags like "/bigobj, /LTCG, /GL" on Windows and "-fvisibility=hidden" and "-flto" on Linux. Remember that this is a metatemplate library, which will lead to horribly bloated binaries unless these kinds of parameters are specified (http://pybind11.readthedocs.io/en/latest/faq.html#how-can-i-create-smaller-binaries discusses some of the reasons). I am very worried that people will start using the interface library and then forget to pass these extra parameters, leading to a bad user experience when using this library. Unless these flags can be incorporated, I would rather not add the interface library to the build system.
The other changes involving the installation targets look good to me.
Thanks,
Wenzel
|
|
||
| # Cache variables so pybind11_add_module can be used in parent projects | ||
| set(PYBIND11_INCLUDE_DIR "${CMAKE_CURRENT_LIST_DIR}/include" CACHE INTERNAL "") | ||
| # explicit "share" not GNUInstallDirs "DATADIR" for CMake search path |
There was a problem hiding this comment.
This comment doesn't parse for me. What does this mean?
There was a problem hiding this comment.
Admittedly, it's obscure. If you were following GNUInstallDirs, you might expect everything for include to go into the user-configurable CMAKE_INSTALL_INCLUDEDIR and everything for share to go into CMAKE_INSTALL_DATADIR. But for purposes of hunting down packages, CMake doesn't care about GNUInstallDirs, it looks explicitly in share. I think it's worth notating. I changed the wording to GNUInstallDirs "DATADIR" wrong here; CMake search path wants "share".
| repository internally, an external installation can be detected | ||
| through `find_package(pybind11 ... CONFIG ...)`. See the `Config file | ||
| <https://github.com/pybind/pybind11/blob/master/tools/pybind11Config.cmake.in>`_ | ||
| docstring for details of influential CMake variables. |
| standard detection, the aforementioned ``pybind11_add_module`` | ||
| wrapper to ``add_library`` can | ||
| be employed as described above (after ``include(pybind11Tools)``). This | ||
| procedure is available when CMake >= 2.8.12 for the consuming project. A |
There was a problem hiding this comment.
when CMake -> when using CMake
remove "for the consuming project", sounds somewhat awkward and is not needed
|
|
||
| .. [test_installed_module] https://github.com/pybind/pybind11/blob/master/tests/test_installed_module/CMakeLists.txt | ||
|
|
||
| For consuming project CMake >= 3.0, pybind11 is additionally available |
There was a problem hiding this comment.
"When using a version of CMake greater than 3.0, pybind11 can additionally be used as a special interface library following the call to find_package"
| # | ||
| function(pybind11_add_module target_name) | ||
| set(lib_type "MODULE") | ||
| set(do_lto True) |
There was a problem hiding this comment.
It appears that this is not used?
There was a problem hiding this comment.
Correct, it's unused. I kept it since @dean0x7d said he had plans for it.
| # Add a CMake parameter for choosing a desired Python version | ||
| set(PYBIND11_PYTHON_VERSION "" CACHE STRING "Python version to use for compiling modules") | ||
|
|
||
| set(Python_ADDITIONAL_VERSIONS 3.4 3.5 3.6 3.7) |
There was a problem hiding this comment.
This line can go away. Python_ADDITIONAL_VERSIONS was used by find_package(PythonLibs), which we haven't been using for a while now.
There was a problem hiding this comment.
The variable is still needed because FindPythonLibsNew calls find_package(PythonInterp) and on older CMake version FindPythonInterp cannot find new Python 3.x executables (e.g. CMake 2.8.12 only checks up to 3.3).
There was a problem hiding this comment.
Good catch! Should the order maybe be different in that case? (try to find the newest version first)
There was a problem hiding this comment.
Yeah, newest-first would probably be a better match for expected behavior. I just checked and CMake also uses newest-first order internally.
There was a problem hiding this comment.
Kept in Tools and switched to set(Python_ADDITIONAL_VERSIONS 3.7 3.6 3.5 3.4)
|
|
||
| target_include_directories(${target_name} | ||
| PRIVATE ${PYBIND11_INCLUDE_DIR} # from project CMakeLists.txt | ||
| PRIVATE ${pybind11_INCLUDE_DIR} # from pybind11Config |
There was a problem hiding this comment.
Is the expectation that one of these two variables will be undefined based on how pybind11 is used/included?
There was a problem hiding this comment.
Yes. The former is present in your accustomed add_directory sense, and the project has already established the PYBIND11_INCLUDE_DIR name. For the latter, once you've selected a capitalization for the project name, the variable names are pretty rigid. (That's why I asked about your preferred capitalization in the original post.)
| include("${CMAKE_CURRENT_LIST_DIR}/${PN}Targets.cmake") | ||
|
|
||
| find_package(PythonLibsNew ${PYBIND11_PYTHON_VERSION} MODULE REQUIRED) | ||
| set_property(TARGET ${PN}::pybind11 APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES ${PYTHON_INCLUDE_DIRS}) |
There was a problem hiding this comment.
This was quite confusing to me, but maybe I just don't understand cmake well enough. What is this pybind11::pybind11 interface library (and why is it, e.g., not just called pybind11 similar to when the project is included via add_directory?). How come we are able to attach properties here even though there was no corresponding add_target call?
| include("${CMAKE_CURRENT_LIST_DIR}/${PN}Targets.cmake") | ||
|
|
||
| find_package(PythonLibsNew ${PYBIND11_PYTHON_VERSION} MODULE REQUIRED) | ||
| set_property(TARGET ${PN}::pybind11 APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES ${PYTHON_INCLUDE_DIRS}) |
There was a problem hiding this comment.
This was quite confusing to me, but maybe I just don't understand cmake well enough. What is this pybind11::pybind11 interface library (and why is it, e.g., not just called pybind11 similar to the interface library name when the project is included via add_directory?). How come we are able to attach properties here even though there was no corresponding add_target call?
There was a problem hiding this comment.
The pybind11Targets.cmake file defines the target (included 3 lines up). That file was created with the export command in the main CMakeLists.txt file and it just basically creates a copy of the pybind11 target from that CMakeLists.txt. The only different is that it attaches a namespace and marks the targets as IMPORTED.
| @@ -0,0 +1,91 @@ | |||
| # pybind11Config.cmake | |||
There was a problem hiding this comment.
pybind11Config.cmake and pybind11ConfigVersion.cmake should be added to .gitignore so that they don't show up when doing in-tree builds.
There was a problem hiding this comment.
Added share/cmake/pybind11/pybind11*cmake, though that'll only catch the default install case. Hope that's right for you purposes.
Moved back inside install block. I'm not familiar with
I suspect the former redundant
The current state of the PR really isn't allowing anything that can't already be done through
Nevertheless, I think the logic you want can be implemented for the interface library. To optimally avoid code repetition, the Also did all the docs edits requested and not directly responded to above. |
Hm, that's precisely what I am concerned about. Can we at least add a big fat warning sign to the documentation? Something like: One more very minor thing: for the .gitignore, what I meant was that the .cmake files are just dropped in to the main directory when doing an in-tree build. Can you apply the following patch to your PR? |
I think that will also ignore |
|
Ok, it looks good to me now. This is a fairly big change to the build system -- any comments from others? (@jagerman, @aldanor, @SylvainCorlay)? Otherwise I'll merge it soon. |
|
(thanks for incorporating all our change requests, @loriab!) |
|
Ping @JohanMabille who was looking at cmake configuration and finder for xtensor lately. |
You're welcome. Thanks for the thorough review; I've learned some useful testing patterns from @dean0x7d along the way. Let me know if you'd like a rebase to master and if you'd like me to give it a new and shorter history (and remove the offending space). |
|
It would be great if you could rebase onto master and crunch it down into a small number of commits. I was going to squash the PR, but that is perhaps a bit drastic. |
|
I think it looks like a great addition. One really minor comment: when rebasing/squashing, perhaps also reflow the text in |
|
Thanks. I've got the |
|
I plan on converting a project to use this to load pybind11 externally rather than in a submodule and see how it goes sometime over the next couple of days. I don't think that's a reason to hold it up though--if I run into any major or minor issues we can always fix them with a follow-up PR. |
|
Merged, thank you! |
|
Thanks everyone (also for considering my request)! I am now able to install pybind11 to a destination of my choice and use it from there without any issues. Just works out of the box 👍 |
Motivation
Acquiring the pybind11 repo (for large projects where the
pybind11_add_moduleisn't appropriate) is admittedly not hard either as a git submodule or as a CMake ExternalProject plus writing a Findpybind11.cmake file to search out the header location. But for the ExternalProject case, the better CMake route is to provide a pybind11Config.cmake in the installation so that consuming projects can use pre-built (installed) pybind11 without writing their own detection.PR Changes
pybind11Config.cmakeandpybind11Targets.cmakeare installed along with the headers. Instructions for detection and use given intools/pybind11Config.cmake.in-DCMAKE_INSTALL_INCLUDEDIRto specify non-standard install location. Defaults toinclude. Note that this isn't affecting thepybind11_add_modulefunction since it's not exporting interface include dirs.Questions
pybind11::pybind11for imported target name. If you want any other capitalization pattern for the project name, let me know and I'll adjust everything.1.8, but thewrite_basic_package_version_file(...)four lines and theConfigVersion.cmakeinstallation can be commented out, if you don't want to deal with versioning in development mode.CMake Version
2.8.12(https://gitlab.kitware.com/cmake/cmake/blob/v2.8.12/Modules/)$<INSTALL_INTERFACEgenerator present at2.8.12(https://cmake.org/cmake/help/v2.8.12/cmake.html#section_Generators)2.8.12to install, but can confirm the exported target exports at3.0.1