search

ros / urdfdom

URDF parser
http://ros.org/wiki/urdf
Other
97 stars 131 forks source link

Install headers to include/${PROJECT_NAME} #167

Closed sloretz closed 2 years ago

sloretz commented 2 years ago

Part of https://github.com/ros2/ros2/issues/1150 - this installs headers to a unique include directory to prevent include directory search order issues when overriding packages from a merged workspace.

clalancette commented 2 years ago

While I agree with this in principle, I'm slightly concerned about the non-ROS 2 users of this repository. In particular, we use this same branch for ROS 2, ROS 1, and standalone compilations. If we can show that all of those users will be happy with this change, then I'm all for it.

sloretz commented 2 years ago

I'm slightly concerned about the non-ROS 2 users of this repository. In particular, we use this same branch for ROS 2, ROS 1, and standalone compilations. If we can show that all of those users will be happy with this change, then I'm all for it.

I did some testing (https://github.com/sloretz/investigate_urdfom_from_source) and made a few changes to this PR. I tested using catkin_make, plain cmake, and a Makefile using pkg-config. All cases built and ran fine.

ROS 1 uses https://packages.ubuntu.com/focal/liburdfdom-dev (see use of the rosdep key here https://index.ros.org/d/liburdfdom-dev/ ), so it won't be affected by this PR. That said, it doesn't seem to be a problem as long as the packages use urdfdom_INCLUDE_DIRS.

When it's a system package the include directory doesn't really matter. We care about it in ROS 2 because we release it into the ROS distro ( https://index.ros.org/r/urdfdom/github-ros-urdfdom/#rolling ). The Debian package maintainers (@j-rivero FYI) may not want the extra directory, so I added a CMake option APPEND_PROJECT_NAME_TO_INCLUDEDIR. It can be set to OFF to get the old behavior of installing to just include.

$ cmake .... -DAPPEND_PROJECT_NAME_TO_INCLUDEDIR=OFF
...

Lastly, I added a new urdf_parser target because I wanted it for testing this PR with modern CMake targets.

sloretz commented 2 years ago

though I guess we have to wait on https://github.com/ros/urdfdom_headers/pull/71 to be ready before we merge this

I updated ros/urdfdom_headers#71 with the same CMake option, but there's no need to wait for it. This PR doesn't depend on that one.

sloretz commented 2 years ago

CI (build: --packages-above-and-dependencies urdfdom test: --packages-select-regex "urdf.*")

sloretz commented 2 years ago

The compiler warnings in the aarch64 build are all in the nightly as well: https://ci.ros2.org/view/nightly/job/nightly_linux-aarch64_release/1852/ , so approved and CI LGTM. Merging.