[BUG] find_package(pybind11) does not set up python header path properly

[xkszltl]

Issue description

Pybind11 2.6.0 (build from source by us)

We hit this issue when building onnx 1.7.0:

FAILED: CMakeFiles/onnx_cpp2py_export.dir/onnx/cpp2py_export.cc.o
ccache /usr/bin/g++-8 -DONNX_ML=1 -DONNX_NAMESPACE=onnx -Donnx_cpp2py_export_EXPORTS -I../ -I. -fdebug-prefix-map='/tmp/scratch'='/usr/local/src' -g -Wnon-virtual-dtor -O3 -DNDEBUG -fPIC -fvisibility=hidden -std=gnu++11 -MD -MT CMakeFiles/onnx_cpp2py_export.dir/onnx/cpp2py_export.cc.o -MF CMakeFiles/onnx_$pp2py_export.dir/onnx/cpp2py_export.cc.o.d -o CMakeFiles/onnx_cpp2py_export.dir/onnx/cpp2py_export.cc.o -c ../onnx/cpp2py_export.cc
In file included from /usr/local/include/pybind11/pytypes.h:12,
                 from /usr/local/include/pybind11/cast.h:13,
                 from /usr/local/include/pybind11/attr.h:13,
                 from /usr/local/include/pybind11/pybind11.h:45,
                 from ../onnx/cpp2py_export.cc:1:
/usr/local/include/pybind11/detail/common.h:122:10: fatal error: Python.h: No such file or directory
 #include <Python.h>
          ^~~~~~~~~~
compilation terminated.

Pybind11 is introduced by the following cmake code:
https://github.com/onnx/onnx/blob/0c070abb0c40fec649f81a73a75b0098662ec486/CMakeLists.txt#L401-L413

The same script worked before and there's no recent release from onnx, so I think it should be a regression related to pybind11 repo.

[xkszltl]

Double checked, pybind v2.5.0 works so it may be related to the recent refactoring in 2.6.0's CMakeConfig template.

[YannickJadoul]

I guess @henryiii knows this best.

[henryiii]

There is a typo: ${PYTHON_INCLUDE_DIR} should be ${PYTHON_INCLUDE_DIRS} here (see docs): ; the old ${pybind11_INCLUDE_DIRS} may have included PYTHON_INCLUDE_DIR by accident causing it to work before, perhaps?

[xkszltl]

While I'm not sure if that's the root cause, an issue has been filed in onnx repo to ask for their engagement: onnx/onnx#3084

[xkszltl]

@henryiii Do you think pybind11_INCLUDE_DIRS should carries pybind11's dependency? If not, which is the correct entrypoint for referencing pybind11's dependency tree?

[henryiii]

I think there is an issue - pybind11_INCLUDE_DIRS should include Python (not that that's really a great idea, but it's backward compatible / matches the description), and pybind11_INCLUDE_DIR should not. But it does not seem that they are currently set properly.

The correct way to use pybind11 (or most any CMake library, really) is to use targets, and those are correctly set up - that includes all dependencies, including flags, min C++ versions, and include directories. pybind11::pybind11 has everything you need, and pybind11::headers is just the pybind11 directory (no Python). If you do it manually (like onnx is doing), then you generally really know what you are doing, so you need the most granular variables, so you'd add both python and pybind11 dirs separately. Like onnx tried to do but had a typo.

[henryiii]

This is odd, I distinctly remember the code setting pybind11_INCLUDE_DIRS but it now seems to be gone...

[henryiii]

@xkszltl Could you try #2636? I've pushed it as a branch to the main repo to make it easier to checkout and use if you are not using GitHub's CLI. We don't really have a way to test the CMake variables.

[xkszltl]

Thanks @henryiii for the quick response!
I've verified the fix.
Would be great to see it in v2.6.1 soon.