use target_include_directories #54
Conversation
Signed-off-by: Karsten Knese <[email protected]>
|
for what it's worth: I am not part of the org unit and can't assign myself to this issue/PR |
|
|
@wjwwood just a friendly ping as you're listed as the maintainer. Not sure if you're watching this repo. |
|
@jonbinney is helping, but maybe never added himself to the |
|
I thought I was safe if I wasn't in the package xml! :-P I'll review this today. |
|
Not so long as I'm around :p But seriously, @jonbinney feel free ask me for help if you don't have time. |
|
@jonbinney I hope the change is trivial enough that it doesn't take much to review it. It's essentially really just avoiding a global |
| ${Eigen3_INCLUDE_DIRS} | ||
| ) | ||
| ament_target_dependencies(laser_geometry | ||
| "rclcpp" |
There was a problem hiding this comment.
@Karsten1987 do these need to be quoted?
There was a problem hiding this comment.
I guess they don’t have to be quoted since every variable is interpreted as a string (I might be wrong here)
But we do that in other parts of the ROS2 code base as well, e.g. in rclcpp: https://github.com/ros2/rclcpp/blob/master/rclcpp/CMakeLists.txt#L112
There was a problem hiding this comment.
That's fair, and I'll go ahead and merge this. I do think that quoting some things but not others (for example the arguments to find_package listed earlier in the file) might confuse people. In general I prefer only adding required characters, so that people don't wonder what they really need and what is optional.
|
Yeah, i mainly wanted to build it myself as a sanity check. It does build for me, and tests pass (yay!). I've left one comment with a question, once that is answered I'll merge. |
fixes #53
please also consider this for a backport to eloquent.
Signed-off-by: Karsten Knese [email protected]