From 19ee71fe60a1dfe0bbebd2829d4c783b0f826113 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Tue, 27 Feb 2024 13:27:50 +0000 Subject: [PATCH 1/3] Allow topic and service to construct messages from description files. The current implementation allows for additional messages to be read from these files, it was just not properly configured for the files to be accessible. Signed-off-by: Michael Carroll --- src/Factory.cc | 67 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 26 deletions(-) diff --git a/src/Factory.cc b/src/Factory.cc index b8a10c06..f606eaf4 100644 --- a/src/Factory.cc +++ b/src/Factory.cc @@ -25,6 +25,7 @@ #include #include +#include #include #include #include @@ -115,40 +116,54 @@ class DynamicFactory // Split all the directories containing .desc files. std::vector descDirs = split(_paths, ':'); - for (const std::string &descDir : descDirs) + auto loadDescFile = [this](const std::string &descFile) { - for (DirIter dirIter(descDir); dirIter != DirIter(); ++dirIter) + // Ignore files without the correct extensions. + if (descFile.rfind(".desc") == std::string::npos && + descFile.rfind(".proto") == std::string::npos && + descFile.rfind(".proto.bin") == std::string::npos) + return; + + // Parse the .desc file. + std::ifstream ifs(descFile); + if (!ifs.is_open()) { - // Ignore files without the .desc extension. - if ((*dirIter).rfind(".desc") == std::string::npos) - continue; + std::cerr << "DynamicFactory(): Unable to open [" << descFile << "]" + << std::endl; + return; + } - // Parse the .desc file. - std::ifstream ifs(*dirIter); - if (!ifs.is_open()) - { - std::cerr << "DynamicFactory(): Unable to open [" << *dirIter << "]" - << std::endl; - continue; - } + google::protobuf::FileDescriptorSet fileDescriptorSet; + if (!fileDescriptorSet.ParseFromIstream(&ifs)) + { + std::cerr << "DynamicFactory(): Unable to parse descriptor set from [" + << descFile << "]" << std::endl; + return; + } - google::protobuf::FileDescriptorSet fileDescriptorSet; - if (!fileDescriptorSet.ParseFromIstream(&ifs)) + // Place the real descriptors in the descriptor pool. + for (const google::protobuf::FileDescriptorProto &fileDescriptorProto : + fileDescriptorSet.file()) + { + if (!this->pool.BuildFile(fileDescriptorProto)) { - std::cerr << "DynamicFactory(): Unable to parse descriptor set from [" - << *dirIter << "]" << std::endl; - continue; + std::cerr << "DynamicFactory(). Unable to place descriptors from [" + << descFile << "] in the descriptor pool" << std::endl; } + } + }; - // Place the real descriptors in the descriptor pool. - for (const google::protobuf::FileDescriptorProto &fileDescriptorProto : - fileDescriptorSet.file()) + for (const std::string &descDir : descDirs) + { + if (!std::filesystem::is_directory(descDir)) + { + loadDescFile(descDir); + } + else + { + for (DirIter dirIter(descDir); dirIter != DirIter(); ++dirIter) { - if (!pool.BuildFile(fileDescriptorProto)) - { - std::cerr << "DynamicFactory(). Unable to place descriptors from [" - << *dirIter << "] in the descriptor pool" << std::endl; - } + loadDescFile(*dirIter); } } } From 4f257e3b4679b61ef363bee793156b09bc339b83 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Fri, 8 Mar 2024 20:19:06 +0000 Subject: [PATCH 2/3] Add test for descriptor loading Signed-off-by: Michael Carroll --- test/desc/testing.proto | 0 test/desc/testing.proto.bin | 16 +++++++ test/desc/testing/bytes.proto | 8 ++++ test/desc/testing/message.proto | 15 ++++++ test/integration/CMakeLists.txt | 5 ++ test/integration/descriptors.cc | 84 +++++++++++++++++++++++++++++++++ 6 files changed, 128 insertions(+) create mode 100644 test/desc/testing.proto create mode 100644 test/desc/testing.proto.bin create mode 100644 test/desc/testing/bytes.proto create mode 100644 test/desc/testing/message.proto create mode 100644 test/integration/descriptors.cc diff --git a/test/desc/testing.proto b/test/desc/testing.proto new file mode 100644 index 00000000..e69de29b diff --git a/test/desc/testing.proto.bin b/test/desc/testing.proto.bin new file mode 100644 index 00000000..ea7cbe8c --- /dev/null +++ b/test/desc/testing.proto.bin @@ -0,0 +1,16 @@ + +C +testing/bytes.prototesting" +Bytes +data ( Rdatabproto3 +ˆ +testing/message.prototesting" + +FooMessage +foo ( Rfoo" + +BarMessage +foo ( Rfoo" + +BazMessage +foo ( Rfoobproto3 \ No newline at end of file diff --git a/test/desc/testing/bytes.proto b/test/desc/testing/bytes.proto new file mode 100644 index 00000000..b2524164 --- /dev/null +++ b/test/desc/testing/bytes.proto @@ -0,0 +1,8 @@ +syntax = "proto3"; + +package testing; + +message Bytes +{ + bytes data = 1; +} diff --git a/test/desc/testing/message.proto b/test/desc/testing/message.proto new file mode 100644 index 00000000..751197d6 --- /dev/null +++ b/test/desc/testing/message.proto @@ -0,0 +1,15 @@ +syntax = "proto3"; + +package testing; + +message FooMessage { + string data = 1; +} + +message BarMessage { + FooMessage foo = 1; +} + +message BazMessage { + BarMessage bar = 1; +} diff --git a/test/integration/CMakeLists.txt b/test/integration/CMakeLists.txt index d1b9e17f..56b1586f 100644 --- a/test/integration/CMakeLists.txt +++ b/test/integration/CMakeLists.txt @@ -6,3 +6,8 @@ if(MSVC) endif() gz_build_tests(TYPE INTEGRATION SOURCES ${tests}) + +if (TARGET INTEGRATION_descriptors) + target_compile_definitions(INTEGRATION_descriptors + PRIVATE GZ_MSGS_TEST_PATH="${PROJECT_SOURCE_DIR}/test") +endif() diff --git a/test/integration/descriptors.cc b/test/integration/descriptors.cc new file mode 100644 index 00000000..6d38b969 --- /dev/null +++ b/test/integration/descriptors.cc @@ -0,0 +1,84 @@ +/* + * Copyright (C) 2024 Open Source Robotics Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include + +#include + +#include "gz/msgs/Factory.hh" + +static constexpr const char * kMsgsTestPath = GZ_MSGS_TEST_PATH; + +TEST(FactoryTest, DynamicFactory) +{ + EXPECT_EQ(nullptr, gz::msgs::Factory::New("example.msgs.StringMsg")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.Bytes")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.FooMessage")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BarMessage")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BazMessage")); + + // Test loading an invalid path + { + std::filesystem::path test_path(kMsgsTestPath); + std::string paths = (test_path / "desc" / "does_not_exist.desc").string(); + gz::msgs::Factory::LoadDescriptors(paths); + } + EXPECT_EQ(nullptr, gz::msgs::Factory::New("example.msgs.StringMsg")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.Bytes")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.FooMessage")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BarMessage")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BazMessage")); + + // Test loading an empty file + { + std::filesystem::path test_path(kMsgsTestPath); + std::string paths = (test_path / "desc" / "testing.proto").string(); + gz::msgs::Factory::LoadDescriptors(paths); + } + EXPECT_EQ(nullptr, gz::msgs::Factory::New("example.msgs.StringMsg")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.Bytes")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.FooMessage")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BarMessage")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BazMessage")); + + // Load a valid descriptor file with one message type + { + std::filesystem::path test_path(kMsgsTestPath); + std::string paths = (test_path / "desc" / "stringmsg.desc").string(); + gz::msgs::Factory::LoadDescriptors(paths); + } + + EXPECT_NE(nullptr, gz::msgs::Factory::New("example.msgs.StringMsg")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.Bytes")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.FooMessage")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BarMessage")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BazMessage")); + + // Load a directory + { + std::filesystem::path test_path(kMsgsTestPath); + std::string paths = (test_path / "desc").string(); + gz::msgs::Factory::LoadDescriptors(paths); + } + + EXPECT_NE(nullptr, gz::msgs::Factory::New("example.msgs.StringMsg")); + EXPECT_NE(nullptr, gz::msgs::Factory::New("testing.Bytes")); + EXPECT_NE(nullptr, gz::msgs::Factory::New("testing.FooMessage")); + EXPECT_NE(nullptr, gz::msgs::Factory::New("testing.BarMessage")); + EXPECT_NE(nullptr, gz::msgs::Factory::New("testing.BazMessage")); + +} From fbe8627911c4f97f790dc599cc004dd9d5e6b529 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Fri, 8 Mar 2024 20:33:23 +0000 Subject: [PATCH 3/3] More test data Signed-off-by: Michael Carroll --- test/desc/testing.invalid | 8 ++++++++ test/desc/testing.proto | 8 ++++++++ test/integration/descriptors.cc | 15 +++++++++++++-- 3 files changed, 29 insertions(+), 2 deletions(-) create mode 100644 test/desc/testing.invalid diff --git a/test/desc/testing.invalid b/test/desc/testing.invalid new file mode 100644 index 00000000..b2524164 --- /dev/null +++ b/test/desc/testing.invalid @@ -0,0 +1,8 @@ +syntax = "proto3"; + +package testing; + +message Bytes +{ + bytes data = 1; +} diff --git a/test/desc/testing.proto b/test/desc/testing.proto index e69de29b..b2524164 100644 --- a/test/desc/testing.proto +++ b/test/desc/testing.proto @@ -0,0 +1,8 @@ +syntax = "proto3"; + +package testing; + +message Bytes +{ + bytes data = 1; +} diff --git a/test/integration/descriptors.cc b/test/integration/descriptors.cc index 6d38b969..2d7ee0d5 100644 --- a/test/integration/descriptors.cc +++ b/test/integration/descriptors.cc @@ -43,7 +43,19 @@ TEST(FactoryTest, DynamicFactory) EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BarMessage")); EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BazMessage")); - // Test loading an empty file + // Test loading an incorrect extension + { + std::filesystem::path test_path(kMsgsTestPath); + std::string paths = (test_path / "desc" / "testing.invalid").string(); + gz::msgs::Factory::LoadDescriptors(paths); + } + EXPECT_EQ(nullptr, gz::msgs::Factory::New("example.msgs.StringMsg")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.Bytes")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.FooMessage")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BarMessage")); + EXPECT_EQ(nullptr, gz::msgs::Factory::New("testing.BazMessage")); + + // Test loading a file with invalid content { std::filesystem::path test_path(kMsgsTestPath); std::string paths = (test_path / "desc" / "testing.proto").string(); @@ -80,5 +92,4 @@ TEST(FactoryTest, DynamicFactory) EXPECT_NE(nullptr, gz::msgs::Factory::New("testing.FooMessage")); EXPECT_NE(nullptr, gz::msgs::Factory::New("testing.BarMessage")); EXPECT_NE(nullptr, gz::msgs::Factory::New("testing.BazMessage")); - }