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

[Windows][ROS2] Fixed Windows build issues #277

Closed
wants to merge 2 commits into from

Conversation

seanyen
Copy link
Contributor

@seanyen seanyen commented May 22, 2019

This change is to address 2 issues found when I am building ROS2 Dashing from source.

  • Boost & OpenCV headers are not found at compile time because ${Boost_INCLUDE_DIRS} & ${OpenCV_INCLUDE_DIRS} are not in the includes path.

  • The imported library of cv_bridge.lib is not generated since no functions are dllexport'd from cv_bridge, consequently cv_bridge-utest fails to build when trying to link against cv_bridge. Use WINDOWS_EXPORT_ALL_SYMBOLS to make every functions from cv_bridge dllexport'd to fix it.

@seanyen
Copy link
Contributor Author

seanyen commented May 22, 2019

It seemed to me that Dpr__vision_opencv__ubuntu_bionic_amd64 is already broken before this change?

@seanyen
Copy link
Contributor Author

seanyen commented May 22, 2019

@gaoethan Hope I am pinging the correct owner. Can you help review/merge the change for fixing Windows build?

@mjcarroll
Copy link
Contributor

The build is yellow because of the CMake warning w/r/t Boost. I think that's okay here.

@mjcarroll mjcarroll self-assigned this Jul 9, 2019
@@ -5,6 +5,7 @@ ament_target_dependencies(${PROJECT_NAME}
"sensor_msgs"
)
target_link_libraries(${PROJECT_NAME} ${Boost_LIBRARIES} ${OpenCV_LIBRARIES})
set_target_properties(${PROJECT_NAME} PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS ON)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be doing this or using visibility macros?

@mjcarroll mjcarroll added the ros2 label Jul 10, 2019
@mjcarroll
Copy link
Contributor

I think some of the boost related problems will be resolved by removing it as a dependency here: #283

I would like to get that landed, then I'll come back and look at this one.

@jnoyola
Copy link
Contributor

jnoyola commented Sep 6, 2019

This would address #289, which I just opened, but I still don't understand why the Boost include only breaks the Windows build.

Also, I was able to build without adding back the OpenCV include, so maybe that is no longer necessary since the PR was opened.

@seanyen
Copy link
Contributor Author

seanyen commented Oct 15, 2019

Closed it and in favor of #301

@seanyen seanyen closed this Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants