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

Do not hardcode USE_EXTERNAL_TINYXML2 to OFF on Windows #116

Merged
merged 2 commits into from
Nov 9, 2020

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Nov 1, 2020

Fortunately now in 2020 we have package managers on Windows, so at least giving an option to use an external TinyXML2 also on Windows make sense. : )

Signed-off-by: Silvio Traversaro <silvio.traversaro@iit.it>
@traversaro
Copy link
Contributor Author

The ignition_common-ci-pr_any-homebrew-amd64 failure seems to be unrelated.

@mjcarroll
Copy link
Contributor

Do you think it makes sense to move this logic into ign-cmake so that it can be used in other places as well? I believe TinyXML2 is at least used here, in sdformat and a few more places up the stack.

@traversaro
Copy link
Contributor Author

Do you think it makes sense to move this logic into ign-cmake so that it can be used in other places as well? I believe TinyXML2 is at least used here, in sdformat and a few more places up the stack.

I don't think so. USE_EXTERNAL_TINYXML2 is an option that only make sense for projects that vendor tinyxml2, and ignition-cmake does not provide any function to vendor tinyxml2 itself. Furthermore, given that package managers such as conan, vcpkg and conda are becoming to be more used on Windows, I hope that for future projects the need to vendor dependencies such as tinyxml2 will disappear.

@mjcarroll mjcarroll merged commit 8eb57de into gazebosim:ign-common3 Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants