search

ros2 / tinyxml2_vendor

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

Remove undesired reset of TinyXML2_FOUND and update TODO #5

Closed nuclearsandwich closed 5 years ago

nuclearsandwich commented 5 years ago

I found afbae40 while investigating #4 but I don't believe it's related. fde8000 is follow up from #3.

@jacobperron I couldn't get the full story from commit logs alone. Do you recall why this change in #2 was introduced?

jacobperron commented 5 years ago

@nuclearsandwich At the time of #2, TinyXML2_FOUND was not being set because the name passed to find_package_handle_standard_args() was lowercase (tinyxml2).

https://github.com/ros2/tinyxml2_vendor/pull/2/files#diff-120198e331f1dd3e7806c31af0cfb425R24

As it has since been changed to match the expected case, I agree the "reset" shouldn't be there.

nuclearsandwich commented 5 years ago

At the time of #2, TinyXML2_FOUND was not being set because the name passed to find_package_handle_standard_args() was lowercase (tinyxml2).

Okay. That confirms my suspicion that the change was a partial fix for the same kinds of issues encountered in pluginlib. The tinyxml2 vs TinyXML2 problem looks like it was even causing builds of tinyxml2_vendor to succeed when TinyXML2 wasn't installed on the system.

nuclearsandwich commented 5 years ago