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

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.

* test for existence of urdf typedef
* if not existing, activate compatibility header
urdf::LinkSharedPtr l;
return 0;
}
" HAVE_URDFDOM_4)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

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

@rhaschke
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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

@v4hn
Copy link
Contributor

v4hn commented Sep 22, 2016

tests passed and feedback has been addressed
merging

Thanks lots @rhaschke

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