Skip to content
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

[ros2] cv_bridge boost dependency #289

Closed
jnoyola opened this issue Sep 6, 2019 · 8 comments
Closed

[ros2] cv_bridge boost dependency #289

jnoyola opened this issue Sep 6, 2019 · 8 comments

Comments

@jnoyola
Copy link
Contributor

jnoyola commented Sep 6, 2019

Problem
I'm trying to build the dashing branch but cv_bridge fails with the error:

cv_bridge\src\cv_bridge.cpp(39,10): error C1083: Cannot open include file: 'boost/endian/conversion.hpp'

I see that PR #212 intended to "remove boost dependence", removing Boost from the include_directories call, but cv_bridge\src\cv_bridge.cpp still contains the include (in fact the same PR updated that line).

Fix
If I add back the include to be include_directories(include ${Boost_INCLUDE_DIRS}), then it builds successfully.

Question
So my question is: am I doing something wrong or is it possible the ros2 branches haven't built in over a year?

I'm on Windows 10, but based on the evidence it shouldn't matter.

@mjcarroll
Copy link
Contributor

You are correct, it needs to be there for Windows for the current build. I believe that the header gets found correctly on Linux because it's in the /usr/include or /usr/local/include based on the distribution.

I added a shim to rcpputils to remove the boost/endian dependency (https://github.com/ros2/rcpputils/blob/master/include/rcpputils/endian.hpp), we could potentially get that backported to dashing to completely remove boost dependency from C++, but there won't be any python until pybind11 lands.

@mjcarroll
Copy link
Contributor

That was an oversight, I think that the cv_bridge package is blacklisted in the ROS2 CI on Windows, so I may have missed it.

@jnoyola
Copy link
Contributor Author

jnoyola commented Sep 6, 2019

I don't fully understand pybind11 work, but I believe it's working fine for me on Windows with boost::python, although I see how it can be more difficult to maintain.

So what's the best path forward here? I would love to see dashing in a buildable state, even if it still has the boost::python dependency in the interim.

@mjcarroll
Copy link
Contributor

Let's add include_directories(include ${Boost_INCLUDE_DIRS}) back for now, it shouldn't have any impact.

@mjcarroll
Copy link
Contributor

I don't fully understand pybind11 work, but I believe it's working fine for me on Windows with boost::python, although I see how it can be more difficult to maintain.

Generally, we're trying to avoid bringing in the Boost dependency in ROS2, if possible. C++11 and beyond add much of the functionality that ROS was using from Boost, and it keeps the dependency tree much smaller.

PyBind11 provides generally the same functionality as Boost::Python without the dependency.

@jnoyola
Copy link
Contributor Author

jnoyola commented Sep 6, 2019

If that's the case, should we reconsider #277? I don't have insight into the other changes to include OpenCV and export symbols. If that's overkill for now then we can just add the one line.

EDIT: I opened PR #290 with the one line change. Please take a look

@jnoyola
Copy link
Contributor Author

jnoyola commented Sep 30, 2019

Ping @mjcarroll, if you could please take a look at one of those 2 PRs

@jnoyola
Copy link
Contributor Author

jnoyola commented Nov 5, 2019

Resolved with #290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants