-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: support basic dual includes #2804
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
Conversation
Wait, that's all? I was expecting this to be a horrible PR! |
("horrible" in the amount of code/hacks necessary to get this done, I mean; not horrible in it's implementation by you, ofc, @henryiii ;-) ) |
It's not horrible if one writes good CMake to begin with. ;) |
And, this is just to make it possible, it's not really testing to see if all the needed directory-level variables get defined per-directory. But that should hopefully only be minor tweaks on top. Hopefully @dmikushin can test. :) |
@@ -11,7 +11,7 @@ endif() | |||
|
|||
project(test_subdirectory_target CXX) | |||
|
|||
add_subdirectory(${PYBIND11_PROJECT_DIR} pybind11) | |||
add_subdirectory("${pybind11_SOURCE_DIR}" pybind11) |
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.
How are these two variables different?
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 is now the same name as the original variable, rather than being renamed, so it works when added from the main file as well.
@@ -25,7 +25,7 @@ function(pybind11_add_build_test name) | |||
endif() | |||
|
|||
if(NOT ARG_INSTALL) | |||
list(APPEND build_options "-DPYBIND11_PROJECT_DIR=${pybind11_SOURCE_DIR}") | |||
list(APPEND build_options "-Dpybind11_SOURCE_DIR=${pybind11_SOURCE_DIR}") |
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.
@bstaletic this is the important bit.
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.
Ah, that makes sense!
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.
Obligatory moaning about cmake: for once, reading cmake source didn't hurt... too much.
@@ -25,7 +25,7 @@ function(pybind11_add_build_test name) | |||
endif() | |||
|
|||
if(NOT ARG_INSTALL) | |||
list(APPEND build_options "-DPYBIND11_PROJECT_DIR=${pybind11_SOURCE_DIR}") | |||
list(APPEND build_options "-Dpybind11_SOURCE_DIR=${pybind11_SOURCE_DIR}") |
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.
Ah, that makes sense!
Description
This allows pybind11 to be included twice in CMake. It is just a basic check; it is possible some variables will need to be set again that are being skipped, but I think it mostly should work.
(Some variables are directory scoped, and IMPORT targets are as well).
Fixes #2800.
Suggested changelog entry: