-
Notifications
You must be signed in to change notification settings - Fork 778
default to not use vendored tinyxml2 in a ros environment #1033
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
default to not use vendored tinyxml2 in a ros environment #1033
Conversation
|
Looks like the pinned version of the library in an ubuntu 22.04 distro doesn't contain the cmake lookup macros. I am trying to add the vendor dep as well under the assumption that the installation of both will provide the required cmake macro as a fallback on that system and the default will be used otherwise... |
82ae4e5 to
4a78096
Compare
| message(STATUS "------------------------------------------") | ||
| include(cmake/conan_build.cmake) | ||
| endif() | ||
| option(USE_VENDORED_TINYXML2 "Use the bundled version of tinyxml2" ${_default_use_vendored_tiny_xml}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could leave the option declaration above with the rest and on line 97 use a combination of set + force to disable it.
That way all related options are still on the same place.
Another possible approach is to move up the find ament call so it happens before we declare the options and then do something like `set(AMENT_MODE ament_cmake_FOUND)
And use that as the default value for the tinyxml option
option(USE_VENDORED_TINYXML2 "Use the bundled version of tinyxml2" ${AMENT_MODE})The rationale is the same, keep all the options to opt out of vendored dependencies together
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried playing around with something like the first option, but the cached option didn't behave the way I expected. I am only a ROS user and I wasn't sure how to make the change without potentially breaking someone else's workflow by always force overriding a cached thing.
Open to doing the second thing. It is the way it is to minimise the line diff. If the logic is acceptable I will squash the commits and reorder to move the option back to the same spot as the others.
|
fixed my way. Thanks |
option vars have caching that I don't fully understand but this seems to do the trick. I ran the following build command sets to verify it gets set the right way in and out of ROS:
also quick check of the dependencies:
I think this is the correct one to depend on -> https://index.ros.org/d/tinyxml2/