Revert "pybind11_catkin: 2.2.3-0 in 'melodic/distribution.yaml' [bloom]"

[tfoote]

Reverts #18471

Reverting pybind11_catkin from melodic due to installing a conflicting version of pybind11 which is upstream as of artful

@wxmerkt

We talked about in #18282 (comment) we can do the extra cmake-modules only but this appears to have the full implementation installed.

[wxmerkt]

Does it actually conflict? We have everything namespaced underneath pybind11_catkin (and it is meant to wrap the latest version) and it is only active and on the paths explicitly if it is included in the package.xml. I have checked that it does not add pybind11 to the global include but it's pybind11_catkin/pybind11.

[tfoote]

If you can provide an audit of all the resources and show that if a package depends on this and the system version there won't be any conflict then we can consider it. Looking inside the debian's include directory it looks like all the same headers. And that means if this package is added to your include path you will get this version of the headers and not the system version, which on first blush suggests that it will conflict quite quickly.

But in general our policy is not to duplicate things installed on the system. It rarely works out well. In the short run things can often be made to work, but as time goes on you can end up boxing yourself into a corner quite effectively and it ends up being much more work to dig out. If others have taken the time and effort to package something and qa and test it across a large range of systems, we should leverage their work.

[wxmerkt]

Hi @tfoote,
I'd like to come back to this - due to a concrete benefit of packaging this wrapper which contains a more recent version of the library containing numerous improvements which are of particular interest in robotics. The Debian/artful/bionic released version of pybind11 package 2.0.x, while the current version is 2.2.x. One of the key differences, and motivating factor for moving from 2.0 to 2.2, is the better support for wrapping Eigen <> NumPy (without copy overheads, and fixing a number of bugs that could lead to wrong data being transferred/memory mapped).

What can we do to get this into Melodic as well beyond just an CMake-overlay on the system package. E.g., perhaps adding a configure check whether the system path is defined on include of pybind11_catkin and throwing an error on configure. Keep in mind, users would have to include both find_package(pybind11 REQUIRED) and find_package(pybind11_catkin REQUIRED)/find_package(catkin REQUIRED COMPONENTS pybind11_catkin) to cause a conflict in the first place. While projects may revert to using an ExternalProject_Add step during the configuration to get a newer version, this project does provide a benefit beyond that.

Thank you very much :-)

[tfoote]

I understand that there are new features that we'd like to use. Everything actively under development usually has a new feature that we'd like to use.

But if we override the version of pybind11 we will potentially break anyone on the system who is independently using pybind11. Pushing forward is great, and if we were completely our own distro we would be able to move much more quickly and pull everything in sooner. However as we're a small part of a larger ecosystem part of our role is to work with the rest of the ecosystem and that includes using the versions of packages that are already packages.

As of the time of feature freeze for bionic pybind11 2.2 had not passed through their QA process to be vetted to be added to the release so it's still on 2.0.x For the next release Cosmic it's been vetted and 2.2.3 is packages and available. Also it's in Buster so looking forward it's all there we just have to be patient until the supported platforms make it available.

As mentioned above we can add it but it must be setup to not interfere with the system version to not break anyone else who's expecting the system version. It's a lot of effort to package up a non-conflicting version. But the difference is that doing so puts the cost of the change on the people who opt in. If you conflict or override the system version, it's a classic case of externalities where you don't know who you will break, and they won't know that it's your change that broke their software without significant debugging, but things just work better for you.

[wxmerkt]

Hi @tfoote,
We'd like to revisit this issue and release into melodic. I have added a few changes and carried out an audit (cf. wxmerkt/pybind11_catkin#5) on what files get installed where:

install/share/pybind11_catkin/package.xml
install/share/pybind11_catkin/cmake/pybind11Tools.cmake
install/share/pybind11_catkin/cmake/pybind11_catkinConfig-version.cmake
install/share/pybind11_catkin/cmake/pybind11_catkinConfig.cmake
install/share/pybind11_catkin/cmake/pybind11_catkin.cmake
install/share/pybind11_catkin/cmake/FindPythonLibsNew.cmake
install/lib/pkgconfig/pybind11_catkin.pc
install/lib/pkgconfig/catkin_tools_prebuild.pc
install/include/pybind11_catkin/pybind11/stl.h
install/include/pybind11_catkin/pybind11/stl_bind.h
install/include/pybind11_catkin/pybind11/pytypes.h
install/include/pybind11_catkin/pybind11/pybind11.h
install/include/pybind11_catkin/pybind11/options.h
install/include/pybind11_catkin/pybind11/operators.h
install/include/pybind11_catkin/pybind11/numpy.h
install/include/pybind11_catkin/pybind11/iostream.h
install/include/pybind11_catkin/pybind11/functional.h
install/include/pybind11_catkin/pybind11/eval.h
install/include/pybind11_catkin/pybind11/embed.h
install/include/pybind11_catkin/pybind11/eigen.h
install/include/pybind11_catkin/pybind11/detail/typeid.h
install/include/pybind11_catkin/pybind11/detail/internals.h
install/include/pybind11_catkin/pybind11/detail/init.h
install/include/pybind11_catkin/pybind11/detail/descr.h
install/include/pybind11_catkin/pybind11/detail/common.h
install/include/pybind11_catkin/pybind11/detail/class.h
install/include/pybind11_catkin/pybind11/complex.h
install/include/pybind11_catkin/pybind11/common.h
install/include/pybind11_catkin/pybind11/chrono.h
install/include/pybind11_catkin/pybind11/cast.h
install/include/pybind11_catkin/pybind11/buffer_info.h
install/include/pybind11_catkin/pybind11/attr.h

As can be seen - none are installed outside of the package's scope or override a system-version. We'd like to get the impact assessed (we believe none as we aren't overriding system versions) and pybind11_catkin released into melodic - especially as we have been getting requests and other users in the ROS community. What other checks should we undertake to move this forward?

Thanks,
Wolfgang

[tfoote]

The simple test is that a package working with the system pybind11 won't be effected by depending on another package that uses this package.

With the header files in a different location they won't collide immediately, but any package that depends on pybind11_catkin will extend the catkin_INCLUDE_DIRS to include install/include/pybind11_catkin which now will cause the newer headers to appear on the path potentially breaking the application that was previously working based on the system pybind11 version.

You can play games with the include directories being explicitly added but to really fix this you need to rename the headers so they don't collide. Otherwise if you think about a case where an intermediate package is using the new headers in their header the new headers must be on their exported path and will collide.

And in that case you actually have to change the symbols too. For if my package C with your package B that depends on pybind11_catkin. If I add a dependency on B's headers that use symbols from pybind11_catkin but I am following the API from the system pybind11 either B or C will fail to compile because it depends on the order in the path as to which version of pybind11 will be found. Or alternatively you will get everything as multiply defined because the #pragma once directive in pybind11 won't work for two slightly different copies of the library. To fix that the best thing to do is to go through and change the namespace of all the files in pybind11_catkin to be something like pybind11_catkin so that there's no accidental collisions.

[VladimirIvan]

To fix that the best thing to do is to go through and change the namespace of all the files in pybind11_catkin to be something like pybind11_catkin so that there's no accidental collisions.

That is already done by pybind11 for GCC.

[wxmerkt]

@tfoote: We have now addressed this by:

(a) Not exporting an altered include path. Our special macro appends include_directories locally, where necessary. This macro is distinct from the upstream macro.
(b) The symbol collision issue is already handled by pybind11 itself as @VladimirIvan pointed out.

Is there anything else left todo?

[wxmerkt]

Ping @tfoote - is there anything else left to do or are we good to PR Melodic in?

[tfoote]

If you're confident that both can run in parallel now we can go ahead and re-release it. A small test package which demonstrates them working side by side would be great to see just to confirm and also to be able to use as a canary in case there are potential future issues. The biggest risk of these sort of conflicts is that it will bite people way downstream who aren't well equipped for debugging this sort of issue and won't have a clue what's causing it.