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

Inconsistent package/library name #13

Open
alsora opened this issue Sep 9, 2021 · 3 comments
Open

Inconsistent package/library name #13

alsora opened this issue Sep 9, 2021 · 3 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@alsora
Copy link

alsora commented Sep 9, 2021

Looking at the CMake files in this project there seems to be an inconsistency between the name of the package TinyXML2 and the name of the library tinyxml2::tinyxml2.

Looking at the upstream package, the correct name for the package seems to be all lower-case. See https://github.com/leethomason/tinyxml2/blob/master/test/CMakeLists.txt#L6

Is there a valid reason for this inconsistency in the ROS vendor package?
This makes it harder to use pre-built tinyxml2 packages and I think it does not follow standard CMake naming conventions.

This should be pretty easy to fix, as there are only a handful of ROS packages that depend on tinyxml2: https://index.ros.org/p/tinyxml2_vendor/#galactic-deps

@hidmic hidmic added enhancement New feature or request help wanted Extra attention is needed labels Sep 10, 2021
@hidmic
Copy link
Contributor

hidmic commented Sep 10, 2021

I'm onboard with the fix. Perhaps we can keep TinyXML2 around for one release cycle to avoid hard breaks.

Would you be willing to contribute the necessary patches @alsora?

@alsora
Copy link
Author

alsora commented Sep 12, 2021

Yes, I can take care of this.

Could you elaborate more on "keeping TinyXML2 around"?
Are you asking to add a mechanism to support both naming schemes (and maybe print a warning if using the old one)?

@hidmic
Copy link
Contributor

hidmic commented Sep 13, 2021

Are you asking to add a mechanism to support both naming schemes (and maybe print a warning if using the old one)?

That's reasonable. Plus exporting the same TinyXML2 prefixed CMake variables in addition to whatever else we start exporting. On a closer look to the commit history, #1 seems to suggest tinyxml2_vendor was made the way it is to support several tinyxml2 versions across different platforms. So if we can afford not removing functionality, that'd be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants