Load always the own gz_msgs*.gz_desc first and avoid duplicates#567
Load always the own gz_msgs*.gz_desc first and avoid duplicates#567
Conversation
Signed-off-by: Jose Luis Rivero <jrivero@honurobotics.com>
caguero
left a comment
caguero
left a comment
There was a problem hiding this comment.
Can we perhaps add a test under test/integrations/descriptors.cc that reproduces the issue and fixes it in this branch?
core/src/DynamicFactory.cc
Outdated
| else | ||
| { | ||
| // Default to loading the descriptor file for this gz-msgs major version if it exists | ||
| const std::string ownDescFile = |
There was a problem hiding this comment.
This constant could live outside the loop.
Adds NoDuplicateDescriptors test that reproduces the scenario where the same .gz_desc file is reachable from multiple paths (e.g. GZ_DESCRIPTOR_PATH and global share dir), verifying no libprotobuf "File already exists" errors. Use protobuf log handler to properly detect duplicate descriptor errors Generated-By: Claude Sonnet 4.6
Signed-off-by: Jose Luis Rivero <jrivero@honurobotics.com>
Added. Test failing without this branch https://build.osrfoundation.org/job/gz_msgs-ci-pr_any-noble-amd64/173/testReport/(root)/FactoryTest/NoDuplicateDescriptors/ |
Co-authored-by: Carlos Agüero <caguero@openrobotics.org> Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Jose Luis Rivero <jrivero@honurobotics.com>
core/src/DynamicFactory.cc
Outdated
| }; | ||
|
|
||
| const std::string ownDescFile = | ||
| "gz-msgs" + std::to_string(GZ_MSGS_MAJOR_VERSION) + ".gz_desc"; |
There was a problem hiding this comment.
#569 is removing major version numbers from .gz_desc files. Now that we don't support side-by-side installs, would we run into this problem of duplicate .gz_desc files?
There was a problem hiding this comment.
That is right. I've refactored the gz_desc name to be used in all the places 0b7dbd2 .
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org> Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
After the desc filename was changed to drop the major version number, refactor the filename into a CMake variable and a C++ preprocessor define so it is defined in a single place. This also fixes the DynamicFactory.cc runtime construction which was still using the old versioned filename format. Generated-by: Claude Opus 4.6 Signed-off-by: Jose Luis Rivero <jrivero@honurobotics.com>
Signed-off-by: Jose Luis Rivero <jrivero@honurobotics.com>
3 participants
🦟 Bug fix
Fixes #460
Summary
Seems to happen when there are two files (particularly if you two gazebo versions or using GZ_DESCRIPTOR_PATH) that loads the same defined symbols. The loaders loads all files in these directories no matter what files are: https://github.com/gazebosim/gz-msgs/blob/gz-msgs12/core/src/DynamicFactory.cc#L66-L82
When there are different versions of gz-msgs installed we probably want to load first the gz-msgs{MAJOR_VERSION}.gz_desc before others are loaded since it is probably expected by this software to use he default symbols first. Not sure if this fits 100% in all use cases.
The PR does two things:
Checklist
codecheckpassed (See contributing)Generated-by: Opus 4.6
Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-byandGenerated-bymessages.Backports: If this is a backport, please use Rebase and Merge instead.