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

urdfdom compatibility #25

Merged
merged 3 commits into from Sep 22, 2016
Merged

Conversation

@rhaschke
Copy link
Contributor

rhaschke commented Sep 20, 2016

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 added 2 commits Sep 20, 2016
* test for existence of urdf typedef
* if not existing, activate compatibility header
urdf::LinkSharedPtr l;
return 0;
}
" HAVE_URDFDOM_4)

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Sep 20, 2016

Member

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

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Sep 20, 2016

Member

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

Copy link
Member

davetcoleman left a comment

Wow this looks like it was a lot of effort, thank you

@rhaschke

This comment has been minimized.

Copy link
Contributor Author

rhaschke commented Sep 21, 2016

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

This comment has been minimized.

Copy link
Member

v4hn commented Sep 21, 2016

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

This comment has been minimized.

Copy link
Member

davetcoleman commented Sep 21, 2016

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

This comment has been minimized.

Copy link
Contributor Author

rhaschke commented Sep 22, 2016

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

@v4hn

This comment has been minimized.

Copy link
Member

v4hn commented Sep 22, 2016

tests passed and feedback has been addressed
merging

Thanks lots @rhaschke

@v4hn v4hn merged commit df6fec5 into ros-planning:kinetic-devel Sep 22, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rhaschke rhaschke deleted the ubi-agni:urdfdom-compatibility branch Oct 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.