search

ros2 / tinyxml2_vendor

temporary vendor package for tinyxml2
Apache License 2.0
2 stars 9 forks source link

Inconsistent package/library name #13

Open alsora opened 2 years ago

alsora commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.