Skip to content
Open
21 changes: 21 additions & 0 deletions core/src/DynamicFactory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "DynamicFactory.hh"
#include "gz/utils/Environment.hh"

#include <gz/msgs/config.hh>
#include <gz/msgs/InstallationDirectories.hh>

namespace {
Expand Down Expand Up @@ -121,6 +122,13 @@ void DynamicFactory::LoadDescriptors(const std::string &_paths)
for (const google::protobuf::FileDescriptorProto &fileDescriptorProto :
fileDescriptorSet.file())
{
// Skip protos already loaded (e.g. same .gz_desc reached via
// both GZ_DESCRIPTOR_PATH and the global share directory).
if (this->pool.FindFileByName(fileDescriptorProto.name()) != nullptr)
{
continue;
}

if (!static_cast<bool>(this->pool.BuildFile(fileDescriptorProto)))
{
std::cerr << "DynamicFactory(). Unable to place descriptors from ["
Expand All @@ -133,6 +141,9 @@ void DynamicFactory::LoadDescriptors(const std::string &_paths)
}
};

const std::string ownDescFile =
"gz-msgs" + std::to_string(GZ_MSGS_MAJOR_VERSION) + ".gz_desc";
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is right. I've refactored the gz_desc name to be used in all the places 0b7dbd2 .


for (const std::string &descDir : descDirs)
{
if (!std::filesystem::is_directory(descDir))
Expand All @@ -141,8 +152,18 @@ void DynamicFactory::LoadDescriptors(const std::string &_paths)
}
else
{
// Default to loading the descriptor file for this gz-msgs major version if it exists
auto ownDescPath = std::filesystem::path(descDir) / ownDescFile;
if (std::filesystem::exists(ownDescPath))
{
loadDescFile(ownDescPath.string());
}

for (auto const &dirIter : std::filesystem::directory_iterator{descDir})
{
auto filename = dirIter.path().filename().string();
if (filename == ownDescFile) // Skip the ownDesc already loaded
continue;
loadDescFile(dirIter.path().string());
}
}
Expand Down
35 changes: 35 additions & 0 deletions test/integration/descriptors.cc
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,38 @@ TEST(FactoryTest, DynamicFactory)
EXPECT_NE(nullptr, gz::msgs::Factory::New("testing.BarMessage"));
EXPECT_NE(nullptr, gz::msgs::Factory::New("testing.BazMessage"));
}

TEST(FactoryTest, NoDuplicateDescriptors)
{
// Regression test for https://github.com/gazebosim/gz-msgs/issues/460
// When the same .gz_desc file is reachable via both GZ_DESCRIPTOR_PATH
// and the global share directory, the DynamicFactory used to print:
// [libprotobuf ERROR] File already exists in database: ...
// The fix silently skips proto definitions already present in the pool.

std::filesystem::path test_path(kMsgsTestPath);
std::string path = (test_path / "desc" / "stringmsg.desc").string();

#ifdef _WIN32
std::string paths = path + ";" + path;
#else
std::string paths = path + ":" + path;
#endif

// Capture stderr to detect any "already exists in database" errors from
// libprotobuf. Using gtest's CaptureStderr avoids depending on the
// protobuf-internal SetLogHandler API, which was removed in protobuf 4.x.
::testing::internal::CaptureStderr();

// Loading the same descriptor twice must not cause protobuf errors.
// The second occurrence is skipped because the protos are already in the pool.
gz::msgs::Factory::LoadDescriptors(paths);

std::string stderr_output = ::testing::internal::GetCapturedStderr();
EXPECT_EQ(stderr_output.find("already exists in database"), std::string::npos)
<< "libprotobuf ERROR emitted during duplicate descriptor load: "
<< stderr_output;

// The message type must remain accessible after duplicate loading.
EXPECT_NE(nullptr, gz::msgs::Factory::New("example.msgs.StringMsg"));
}
Loading