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

Fix tf2_bullet dependency export #428

Merged
merged 2 commits into from
May 28, 2021
Merged

Conversation

gleichdick
Copy link
Contributor

tf2_bullet does not have the dependencies on the bullet library exported. The standard approach of ament does not work because of two reasons:

  • the CMake variable BULLET_ROOT seems to be needed on windows
  • FindBullet sets the INCLUDE_DIRS and LIBRARIES all uppercase, which is unexpected by ament.

This was split out of #423.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

A few things to change, but this is definitely the right direction, thanks!

@@ -46,5 +46,4 @@ if(BUILD_TESTING)
endif()

ament_export_include_directories(include)
ament_export_libraries(${PROJECT_NAME})
ament_package()
ament_package(CONFIG_EXTRAS_POST bullet-extra.cmake)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just use CONFIG_EXTRAS here:

Suggested change
ament_package(CONFIG_EXTRAS_POST bullet-extra.cmake)
ament_package(CONFIG_EXTRAS bullet-extra.cmake)

Also, as convention, we almost always use "extras" as opposed to "extra", so rename this file to bullet-extras.cmake.

# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
# POSSIBILITY OF SUCH DAMAGE.


Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: extra whitespace.

@gleichdick
Copy link
Contributor Author

Thanks! I also converted this into a cmake header only library, but I think that's currently out of scope (also because I couldn't find a tutorial on how to make header only libraries in ROS2).

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'll run CI on this next.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

@ahcorde This one looks good to me, but I'd appreciate a second look by you.

@clalancette clalancette merged commit c75f9b3 into ros2:ros2 May 28, 2021
@gleichdick gleichdick deleted the bullet_include branch June 1, 2021 14:02
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

Successfully merging this pull request may close these issues.

None yet

3 participants