From 36298d7ec5a8d1c58ad33bd05df97f5ed1b67b09 Mon Sep 17 00:00:00 2001 From: Jose Luis Rivero Date: Fri, 4 Jun 2021 17:09:01 +0200 Subject: [PATCH 1/5] Check for SDF files containing sibling elements with the same name (Forward port of #3019) --- gazebo/Server.cc | 13 ++ test/integration/CMakeLists.txt | 1 + test/integration/sdf_errors.cc | 140 ++++++++++++++++++ ...est_sdf16_err_sibling_different_type.world | 46 ++++++ .../test_sdf16_err_sibling_same_type.world | 46 ++++++ ...est_sdf17_err_sibling_different_type.world | 46 ++++++ 6 files changed, 292 insertions(+) create mode 100644 test/integration/sdf_errors.cc create mode 100644 test/worlds/test_sdf16_err_sibling_different_type.world create mode 100644 test/worlds/test_sdf16_err_sibling_same_type.world create mode 100644 test/worlds/test_sdf17_err_sibling_different_type.world diff --git a/gazebo/Server.cc b/gazebo/Server.cc index da19c12e6c..138a3b4962 100644 --- a/gazebo/Server.cc +++ b/gazebo/Server.cc @@ -30,6 +30,7 @@ #include #include +#include #include #include @@ -77,6 +78,16 @@ namespace gazebo { struct ServerPrivate { + void InspectSDFElement(const sdf::ElementPtr _elem) + { + if (common::getEnv("GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS")) + return; + + if (not sdf::recursiveSiblingUniqueNames(_elem)) + gzerr << "SDF is not valid, see errors above. " + << "This can lead to an unexpected behaviour." << "\n"; + } + /// \brief Boolean used to stop the server. static bool stop; @@ -496,6 +507,8 @@ bool Server::PreLoad() bool Server::LoadImpl(sdf::ElementPtr _elem, const std::string &_physics) { + this->dataPtr->InspectSDFElement(_elem); + // If a physics engine is specified, if (_physics.length()) { diff --git a/test/integration/CMakeLists.txt b/test/integration/CMakeLists.txt index 70ecd74253..6806306f01 100644 --- a/test/integration/CMakeLists.txt +++ b/test/integration/CMakeLists.txt @@ -95,6 +95,7 @@ set(tests rest_web.cc saving_and_loading.cc sdf.cc + sdf_errors.cc sensor.cc sdf_frame_semantics.cc server_fixture.cc diff --git a/test/integration/sdf_errors.cc b/test/integration/sdf_errors.cc new file mode 100644 index 0000000000..19522f1e40 --- /dev/null +++ b/test/integration/sdf_errors.cc @@ -0,0 +1,140 @@ +/* + * Copyright (C) 2021 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 "gazebo/common/CommonIface.hh" +#include "gazebo/test/ServerFixture.hh" + +using namespace gazebo; + +///////////////////////////////////////////////// +#ifdef _WIN32 +static int setenv(const char *envname, const char *envval, int overwrite) +{ + char *original = getenv(envname); + if (!original || !!overwrite) + { + std::string envstring = std::string(envname) + "=" + envval; + return _putenv(envstring.c_str()); + } + return 0; +} +#endif + +class SDFLogsTest : public ServerFixture +{ + public: void SetUp() + { +#ifndef _WIN32 + const boost::filesystem::path home = common::getEnv("HOME"); +#else + const boost::filesystem::path home = common::getEnv("HOMEPATH"); +#endif + boost::filesystem::path log_path("/.gazebo/server-11345/default.log"); + path = home / log_path; + } + + public: void EXPECT_LOG_STRING(const std::string expected_text) + { + EXPECT_TRUE(log_string_search(expected_text)) << + "The text '" + expected_text + "'" + + " was not found in the log. The test expects it to be there"; + } + + public: void EXPECT_NO_ERR_IN_LOG() + { + EXPECT_NO_LOG_STRING("[Err] "); + } + + public: void EXPECT_NO_LOG_STRING(const std::string no_expected_text) + { + EXPECT_FALSE(log_string_search(no_expected_text)) << + "The text '" + no_expected_text + "'" + + " was found in the log. The test does not expect it be there"; + } + + public: void EXPECT_SDF_ERR_IN_LOG() + { + EXPECT_LOG_STRING("[Err] "); + std::string sdfErrorString = "SDF is not valid"; + EXPECT_LOG_STRING(sdfErrorString); + } + + private: bool log_string_search(const std::string expected_text) + { + // Open the log file, and read back the string + std::ifstream ifs(path.string().c_str(), std::ios::in); + std::string loggedString; + + while (!ifs.eof()) + { + std::string line; + std::getline(ifs, line); + loggedString += line; + } + + return loggedString.find(expected_text) != std::string::npos; + } + + private: boost::filesystem::path path; +}; + +///////////////////////////////////////////////// +TEST_F(SDFLogsTest, EmptyWorldNoErrors) +{ + Load("worlds/empty.world"); + EXPECT_NO_ERR_IN_LOG(); +} + +///////////////////////////////////////////////// +TEST_F(SDFLogsTest, DuplicateSiblingSameType16) +{ + Load("worlds/test_sdf16_err_sibling_same_type.world"); + EXPECT_SDF_ERR_IN_LOG(); +} + +///////////////////////////////////////////////// +TEST_F(SDFLogsTest, DuplicateSiblingSameTypeDisabled16) +{ + setenv("GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS", "", 1); + Load("worlds/test_sdf16_err_sibling_same_type.world"); + EXPECT_NO_ERR_IN_LOG(); + unsetenv("GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS"); +} + +///////////////////////////////////////////////// +TEST_F(SDFLogsTest, DuplicateSiblingDifferentType16) +{ + // 1.6 SDF does NOT enforce different names for different types + Load("worlds/test_sdf16_err_sibling_different_type.world"); + EXPECT_NO_ERR_IN_LOG(); +} + +TEST_F(SDFLogsTest, DuplicateSiblingDifferentType17) +{ + // 1.7+ SDF does enforce different names for different types + Load("worlds/test_sdf17_err_sibling_different_type.world"); + EXPECT_SDF_ERR_IN_LOG(); +} + +///////////////////////////////////////////////// +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/test/worlds/test_sdf16_err_sibling_different_type.world b/test/worlds/test_sdf16_err_sibling_different_type.world new file mode 100644 index 0000000000..da8182fb94 --- /dev/null +++ b/test/worlds/test_sdf16_err_sibling_different_type.world @@ -0,0 +1,46 @@ + + + + + + + 1 + 0 0 1 0 0 1 + + + -2.5 0 0 0 0 0 + + + 5 0.2 2 + + + + + -2.5 0 0 0 0 0 + + + 5 0.2 2 + + + + + -2.5 0 0 0 0 0 + + + 5 0.2 2 + + + + + -2.5 0 0 0 0 0 + + + 5 0.2 2 + + + + + + + + diff --git a/test/worlds/test_sdf16_err_sibling_same_type.world b/test/worlds/test_sdf16_err_sibling_same_type.world new file mode 100644 index 0000000000..600a92dce0 --- /dev/null +++ b/test/worlds/test_sdf16_err_sibling_same_type.world @@ -0,0 +1,46 @@ + + + + + + + 1 + 0 0 1 0 0 1 + + + -2.5 0 0 0 0 0 + + + 5 0.2 2 + + + + + -2.5 0 0 0 0 0 + + + 5 0.2 2 + + + + + -2.5 0 0 0 0 0 + + + 5 0.2 2 + + + + + -2.5 0 0 0 0 0 + + + 5 0.2 2 + + + + + + + + diff --git a/test/worlds/test_sdf17_err_sibling_different_type.world b/test/worlds/test_sdf17_err_sibling_different_type.world new file mode 100644 index 0000000000..1d6a430553 --- /dev/null +++ b/test/worlds/test_sdf17_err_sibling_different_type.world @@ -0,0 +1,46 @@ + + + + + + + 1 + 0 0 1 0 0 1 + + + -2.5 0 0 0 0 0 + + + 5 0.2 2 + + + + + -2.5 0 0 0 0 0 + + + 5 0.2 2 + + + + + -2.5 0 0 0 0 0 + + + 5 0.2 2 + + + + + -2.5 0 0 0 0 0 + + + 5 0.2 2 + + + + + + + + From ad148c79f6f6b7e347e9adc7870e677f5e387c91 Mon Sep 17 00:00:00 2001 From: Jose Luis Rivero Date: Fri, 4 Jun 2021 21:20:46 +0200 Subject: [PATCH 2/5] Workaround for bug 586 from sdformat --- test/integration/sdf_errors.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/integration/sdf_errors.cc b/test/integration/sdf_errors.cc index 19522f1e40..008ec4657f 100644 --- a/test/integration/sdf_errors.cc +++ b/test/integration/sdf_errors.cc @@ -120,9 +120,13 @@ TEST_F(SDFLogsTest, DuplicateSiblingSameTypeDisabled16) ///////////////////////////////////////////////// TEST_F(SDFLogsTest, DuplicateSiblingDifferentType16) { - // 1.6 SDF does NOT enforce different names for different types Load("worlds/test_sdf16_err_sibling_different_type.world"); - EXPECT_NO_ERR_IN_LOG(); + // 1.6 SDF does NOT enforce different names for different types + // The test is affected by https://github.com/osrf/sdformat/issues/586/ so it + // is not true that 1.6 does not enforce since errors are being displayed + // if the test fails, probably the issue was solved. Replace the EXPECT + // EXPECT_NO_ERR_IN_LOG(); + EXPECT_SDF_ERR_IN_LOG(); } TEST_F(SDFLogsTest, DuplicateSiblingDifferentType17) From 0987978b73c621b27bf88b4643eb595597cbc730 Mon Sep 17 00:00:00 2001 From: Jose Luis Rivero Date: Wed, 9 Jun 2021 00:56:31 +0200 Subject: [PATCH 3/5] Include changelog entry --- Changelog.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Changelog.md b/Changelog.md index dd18619115..12a064eff3 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,5 +1,13 @@ ## Gazebo 11 +## Gazebo 11.6.0 (2021-06-xx) + +1. Enable output of gzerr for SDF sibling elements of any type with same name, + following the SDF 1.7 specification. + Environment variable GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS can be set to + use the previous behaviour and do not report these problems. + * [Pull request #3017](https://github.com/osrf/gazebo/pull/3017) + ## Gazebo 11.5.1 (2021-05-05) 1. Avoid range-loop-construct in TopicManager From 4db469737431a6fdd3aa0b18d761e899c314cd7b Mon Sep 17 00:00:00 2001 From: Jose Luis Rivero Date: Wed, 9 Jun 2021 00:58:52 +0200 Subject: [PATCH 4/5] Use _putenv instead of unsetenv for Windows --- test/integration/sdf_errors.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/integration/sdf_errors.cc b/test/integration/sdf_errors.cc index 008ec4657f..c04c61069d 100644 --- a/test/integration/sdf_errors.cc +++ b/test/integration/sdf_errors.cc @@ -114,7 +114,11 @@ TEST_F(SDFLogsTest, DuplicateSiblingSameTypeDisabled16) setenv("GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS", "", 1); Load("worlds/test_sdf16_err_sibling_same_type.world"); EXPECT_NO_ERR_IN_LOG(); +#ifdef _WIN32 + _putenv("GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS"); +#else unsetenv("GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS"); +#endif } ///////////////////////////////////////////////// From 0219c47574def474db97355b3b0517aaabf37de5 Mon Sep 17 00:00:00 2001 From: Jose Luis Rivero Date: Wed, 9 Jun 2021 00:59:21 +0200 Subject: [PATCH 5/5] Implement logic to handle 1.6 sibling elements --- gazebo/Server.cc | 13 ++++++++++++- test/integration/sdf_errors.cc | 6 +----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/gazebo/Server.cc b/gazebo/Server.cc index 138a3b4962..596a79ce6c 100644 --- a/gazebo/Server.cc +++ b/gazebo/Server.cc @@ -34,6 +34,7 @@ #include #include +#include #include #include #include @@ -83,7 +84,17 @@ namespace gazebo if (common::getEnv("GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS")) return; - if (not sdf::recursiveSiblingUniqueNames(_elem)) + // SDF 1.7 and above consider invalid sibling elements of ANY type with + // repeated names while SDF 1.6 and lower only elements of the SAME type + auto sdfVersion = + ignition::math::SemanticVersion(_elem->OriginalVersion()); + bool result; + if (sdfVersion >= ignition::math::SemanticVersion(1, 7)) + result = sdf::recursiveSiblingUniqueNames(_elem); + else + result = sdf::recursiveSameTypeUniqueNames(_elem); + + if (not result) gzerr << "SDF is not valid, see errors above. " << "This can lead to an unexpected behaviour." << "\n"; } diff --git a/test/integration/sdf_errors.cc b/test/integration/sdf_errors.cc index c04c61069d..c1bce2b223 100644 --- a/test/integration/sdf_errors.cc +++ b/test/integration/sdf_errors.cc @@ -126,11 +126,7 @@ TEST_F(SDFLogsTest, DuplicateSiblingDifferentType16) { Load("worlds/test_sdf16_err_sibling_different_type.world"); // 1.6 SDF does NOT enforce different names for different types - // The test is affected by https://github.com/osrf/sdformat/issues/586/ so it - // is not true that 1.6 does not enforce since errors are being displayed - // if the test fails, probably the issue was solved. Replace the EXPECT - // EXPECT_NO_ERR_IN_LOG(); - EXPECT_SDF_ERR_IN_LOG(); + EXPECT_NO_ERR_IN_LOG(); } TEST_F(SDFLogsTest, DuplicateSiblingDifferentType17)