-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Cmake with add subdirectory #970
Conversation
Would someone mind reviewing this PR? |
Can you give me an example parent project Cc: @dean0x7d whose CMake understanding is much better than mine. One other note about this PR (for now)--it'll need a fix for the pypy issue. (I think it would be enough to add |
The So Regarding the second part, I assume the intended usage from the point of view of the parent project is something like: set(PYBIND11_INSTALL ON)
set(PYBIND11_INSTALL_EXPORT OFF)
add_subdirectory(pybind11)
install(EXPORT pybind11Targets ...) Is this correct? What's the intended usage for |
Ah, that makes sense. However, that doesn't work for projects which are embedding Pybind11 though; they're using it incidentally and missing the required headers since they don't do
Sort of. The idea is to just flag it based on For example, this fails without being able to change the export name of pybind11 (when placed in cmake_minimum_required(VERSION 2.8.12)
project(test_embed)
add_subdirectory("${CMAKE_SOURCE_DIR}/../.." pybind11)
add_library(test_embed
test_embed.cpp)
target_link_libraries(test_embed
pybind11::embed)
install(
TARGETS test_embed
EXPORT test_export
ARCHIVE DESTINATION bin
LIBRARY DESTINATION lib
RUNTIME DESTINATION lib)
install(
EXPORT test_export
DESTINATION lib/cmake/test_export/test_export-Targets.cmake)
#include <pybind11/embed.h>
int foo() {
return 0;
} With this error:
|
For completeness, the test suite would build and install that project and then have another project use the install of the first project (which is where |
The solution here should be for the parent package (
Oh, I see. I missed the fact that this would append pybind11's target to the existing export set. OK, this looks fine to me then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, the BUILD_INTERFACE
changes need to be reverted. The parent project's config file will need to call pybind11's Python config (transparent for end users).
The export
name solution looks good to me. I just left some minor organizational comments regarding the new option variables.
CMakeLists.txt
Outdated
@@ -18,6 +18,13 @@ project(pybind11) | |||
set(PYBIND11_MASTER_PROJECT OFF) | |||
if (CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR) | |||
set(PYBIND11_MASTER_PROJECT ON) | |||
set(PYBIND11_EXPORT_NAME "${PROJECT_NAME}Targets") | |||
set(PYBIND11_INSTALL_EXPORT ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an option()
just like PYBIND11_INSTALL
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really a user-facing option, but a project level option; it should not be in the cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the case (it shouldn't be configurable at all), it looks like if(PYBIND11_INSTALL_EXPORT)
could simply be replaced by if(PYBIND11_MASTER_PROJECT)
and PYBIND11_INSTALL_EXPORT
could be removed altogether.
CMakeLists.txt
Outdated
else () | ||
if (NOT PYBIND11_EXPORT_NAME) | ||
set(PYBIND11_EXPORT_NAME "${PROJECT_NAME}Targets") | ||
endif () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since PYBIND11_EXPORT_NAME
is only ever needed in the if(PYBIND11_INSTALL)
block, I'd move it in there to improve locality. No need for separate PYBIND11_MASTER_PROJECT
behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
The thing is that we're installing a vendored Pybind11; there isn't really a Pybind11 to find at all. Since it's embedded, we have no need for flexibility with the installed Python either. Is it fine if the |
I would very much like to avoid that if at all possible. Even if the config file can't |
When Pybind11 is used via `add_subdirectory`, when targets are installed from the parent project, CMake wants all of the dependencies built by the project in the same export set. Projects may now set `PYBIND11_EXPORT_NAME` to have Pybind11 put it targets into the project's export set. If so, do not install Pybind11's export file.
6f57b54
to
80c34e9
Compare
OK, I removed the |
Merged, thanks! |
Resubmission of #967 with better commit messages.
Cc: @tjcorona