moveit / srdfdom

Semantic Robot Description Format
BSD 3-Clause "New" or "Revised" License
13 stars 68 forks source link

urdfdom compatibility #25

Closed rhaschke closed 8 years ago

rhaschke commented 8 years ago

Attempt to solve #23. During configuration time, cmake will check for the existence of urdfdom's typedefs. If not available an appropriate compatibility header will be activated.

We will need a similar mechanism for #209 and we might want to remove this hack if Wily runs out of support... A lot of effort for supporting the nearly dead Wily distribution.

rhaschke commented 8 years ago

Will this break if any single character changes in the upstream file? Seems really unstable!

This check simply checks for existence of the required typedef. As long as the type name urdf::LinkSharedPtr exists, the check will succeed. Anything else can change wildly. Why do you think this is fragile?

Is there a way to instead just check for the ubuntu version?

I would consider this much more fragile, because the urdfdom version is not necessarily linked to the Ubunu version. Someone could have a more recent version of urdfdom installed in an old Ubuntu system.

v4hn commented 8 years ago

Thank you @rhaschke that's the usual way to support multiple versions of dependencies. In my opinion you could replace the compile time check by this (or something equivalent) though:

if( "0.4.0" VERSION_GREATER "${urdfdom_headers_VERSION}")
set(HAVE_URDFDOM_4 0)
else()
set(HAVE_URDFDOM_4 1)
endif()

Note that "0.4.0" VERSION_GREATER "" is true.

@davetcoleman there's more out there than Ubuntu :)

davetcoleman commented 8 years ago

Why do you think this is fragile?

I was first reading the check_cxx_source_compiles line as searching for that exact string in the source code. I read that totally wrong.

there's more out there than Ubuntu :)

I wouldn't know :)

In my opinion you could replace the compile time check by this (or something equivalent) though

Reading this again I'm happy with this fix, +1

rhaschke commented 8 years ago

Applied simplification suggested by @v4hn. I guess, that's ready for merging now.

v4hn commented 8 years ago

tests passed and feedback has been addressed merging

Thanks lots @rhaschke