-
Notifications
You must be signed in to change notification settings - Fork 341
[all] Replace tinyxml by external tinyxml2 #4240
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
[all] Replace tinyxml by external tinyxml2 #4240
Conversation
|
CMake has the version 3.16.3 on the CI. It will not configure if you use the find_or_fetch feature |
|
|
||
| foreach(target ${EXTLIBS_TARGETS}) | ||
| set_target_properties(${target} PROPERTIES DEBUG_POSTFIX "_d") | ||
| set_target_properties(${target} PROPERTIES FOLDER "SofaExtlibs") | ||
| endforeach() |
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.
Is it only related to tinyxml?
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.
To me, yes, as it related to
Lines 3 to 5 in f6bee9f
| set(EXTLIBS_TARGETS | |
| tinyxml | |
| ) |
${EXTLIBS_TARGETS} is defined here only with the embedded version of tinyxml (contains all targets linked for embedded libraries). It is then empty and should not be used in the future to me as we are moving out non-header only embedded libraries.
|
In addition to my initial comment, this version of "find or fetch" using |
|
Alternatively, we could have a naive custom "find or fetch" mechanism such as But, we typically want to avoid this call at each dependency requirement (so here replacing the current |
Can it be written in our own |
|
I guess so. What you have in mind is that it could be a way to avoid multiple fetches in case of multiple calls to |
|
As a conclusion, what is needed:
|
|
Following our recent discussions, the "find or fetch" is replaced by a more classical find package strategy. The package management is fully left to the user.
|
|
Tested and compiled on VS22/Ubuntu22.04 and it works. |
* Replace tinyxml by external tinyxml2. * Customize TinyXML2 cmake module to use a find _package or fetch content mechanism. * Replace find or fetch mechanism by classical cmake find module. * Fix missed cmake target name change * Handle error if tinyxml2 package not found but required * Fix compilation with Dump Visitor activated * Install libtinyxml * FIX error checking * Actually fix... * FIX error loading VTU xml files * FIX scene test --------- Co-authored-by: bakpaul <bakpaul@hotmail.fr> Co-authored-by: Paul Baksic <30337881+bakpaul@users.noreply.github.com>
This aims to tackle one point of #4221 . Required for the conda-forge package (conda-forge/staged-recipes#23085).
This PR propose to replace tinyxml embedded library, which is unmaintained now for a long time, by tinyxml2 (https://github.com/leethomason/tinyxml2/tree/master, under Z-lib license) which looks to be its successor and best candidate, as it is maintened, well packaged with cmake and available on conda-forge.
Also, it is compatible for a future "find package or fetch content" mechanism in cmake. I don't know if we want to split the implementation of this additional feature for this package in another PR because if we do so, current code would need an additional required dependency to tinyxml2.
The "find or fetch" cmake mechanism for this package, avoiding the requirement for this dependency to be installed, can be implemented as it:
However, this would required cmake >=3.24 and this call to be at a toplevel cmake (duplicating this code to replace all current calls to
find_package(TinyXML2)does not seem desirable neither, so we might think also about how we can implement that in the cmake chain).By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).
Reviewers will merge this pull-request only if