@jcwchen That one LGTM

@jcwchen Pybind11 release tag (2.6.0) + pybind11 patch + onnx release tag works. But the issue is in onnx, because it relies on undefined behavior in pybind11, which uses the non-recursive (so only pybind, no python) cmake var to link pybind11. The pybind11 patch restore the old behavior of that UB.

Ah, I missed the PRIVATE/PUBLIC.

The line here:

  target_include_directories(onnx_cpp2py_export PRIVATE
                             $<BUILD_INTERFACE:${ONNX_ROOT}>
                             $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>
                             $<INSTALL_INTERFACE:include>
                             ${PYTHON_INCLUDE_DIR})

Will not propagate PYTHON_INCLUDE_DIRS (okay, without the S, but they are the same) to users of onnx_cpp2py_export. If there are any dependent targets, this will fail, unless you get PYTHON_INCLUDE_DIRS from this line:

  if(pybind11_FOUND)
    target_include_directories(onnx_cpp2py_export PUBLIC
      ${pybind11_INCLUDE_DIRS})

But in 2.6.0, you don’t get that. You really shouldn’t, in CMake variables should not be transitive, so including "${PYTHON_INCLUDE_DIRS}" here too would be better. CMake deduplicates repeats, so there’s no harm in dropping it from the PRIVATE one and adding it here.

By the way, the following else is always wrong:

  else()
    if(EXISTS ${ONNX_ROOT}/third_party/pybind11/include/pybind11/pybind11.h)
      target_include_directories(onnx_cpp2py_export PUBLIC
        ${ONNX_ROOT}/third_party/pybind11/include)

This will never include Python’s include directory, so it only works if you have Python.h already included in a common location somehow. This is one of the reasons “PACKAGE_INCLUDE_DIRS” should not be transitive, but only include the directory(s) that a project itself provides. Targets (like pybind11::pybind11) should be used if transitive requirements are needed.

I’m hoping to fix the usage of pybind11_INCLUDE_DIRS to be identical to previous versions in pybind11 2.6.1. It looks like PYTHON_INCLUDE_DIR is always set by the module and is it internal cached value, but the official output variable has always been PYTHON_INCLUDE_DIRS. Now I’m not totally sure why it would have failed for @xkszltl. Yes, pybind11_INCLUDE_DIRS unintentionally dropped Python’s include dir in 2.6.0 (I personally think it should not have ever been there, as variables should not be recursive, but didn’t intend to change the existing behavior), but Onyx is adding it (as it should), so don’t see why there was a problem.

It is slightly better to use the publicly documented output PYTHON_INCLUDE_DIRS instead of PYTHON_INCLUDE_DIR, which is only documented as an input CACHE variable. But they are functionally identical in master and CMake 3.0.0.

There is a typo: ${PYTHON_INCLUDE_DIR} should be ${PYTHON_INCLUDE_DIRS} here (see docs). pybind11 2.6 dropped Python from the pybind11_INCLUDE_DIRS. It may be restored, as it wasn’t really intentional AFAIK; pybind11_INCLUDE_DIR is just the pybind11 directory, and pybind11_INCLUDE_DIRS has both (not a great idea to combine IMO, but users should be using targets anyway, and both variables are available).