Fix Python build#1278
Conversation
johnwparent
left a comment
There was a problem hiding this comment.
Looks good. This approach seems more in line with the conventions used in the PyBind11 docs as well.
dstoup
left a comment
There was a problem hiding this comment.
The only thing that stands out to me is that sometimes pybind11::pybind11 is being added to the PUBLIC link interface and sometime to PRIVATE and when neither is specified, PUBLIC is the default. It would be good if those links were consistent and correct. The general rule I follow is, if your headers are including the headers of the library being linked, they should be PUBLIC since you're adding it to your public interface. If their headers are not in .h file, e.d. .cxx or other, then the links should be PRIVATE so we don't expose the dependencies to user.
Several places are failing entirely to add the includes for PyBind11, and those places that do are adding them via include_directories. Instead, link the pybind11::pybind11 target in all places that need PyBind11. This fixes trying to build with a PyBind11 that is not in an include directory that is already being used for some other reason.
6ce469d to
99f04fe
Compare
|
@dstoup, I think for most of the python bindings, the distinction is academic, as these are expected to be leaf targets. I did, however, tweak one spot so that Are you asking me to also audit the linkage of |
Modify Python binding leaf libraries (but not utility libraries such as vital_python_util), which we don't expect that anything will ever link anyway, to always use PRIVATE linking. This improves consistency, as most were already PRIVATE, but some for no apparent reason used PUBLIC or even a mix of linking types.
|
Jenkins test this please |
No, I think if it's not relevant since they are all leaf targets, we can ignore this comment. I made the comment mostly because it's a touch confusing to have such inconsistency in how the link targets are added but I don't think it's worth any wide-spread audit. |
|
@dstoup, well, if you note the second commit, you'll see I went ahead and cleaned it up anyway 🙂. BTW, I know for sure |
Several places are failing entirely to add the includes for PyBind11, and those places that do are adding them via include_directories. Instead, link the pybind11::pybind11 target in all places that need PyBind11.
This fixes trying to build with a PyBind11 that is not in an include directory that is already being used for some other reason.