From e7f5e8ed3a25b1cd397d7f9b5c4a28f7c9b0e820 Mon Sep 17 00:00:00 2001 From: Louise Poubel Date: Wed, 25 Nov 2020 09:44:15 -0800 Subject: [PATCH] Fix usage of relative paths with environment variables (#2890) Signed-off-by: Louise Poubel --- Migration.md | 8 ++ .../actor_collisions/actor_collisions.world | 4 +- .../actor_collisions.world.erb | 4 +- gazebo/common/CommonIface.cc | 8 +- gazebo/common/CommonIface.hh | 1 + gazebo/common/CommonIface_TEST.cc | 99 +++++++------------ test/worlds/actor_collisions.world | 4 +- worlds/actor.world | 4 +- worlds/actor_bvh.world | 2 +- worlds/cafe.world | 8 +- worlds/profiler.world | 4 +- 11 files changed, 65 insertions(+), 81 deletions(-) diff --git a/Migration.md b/Migration.md index fda4c762de..d8cc263659 100644 --- a/Migration.md +++ b/Migration.md @@ -5,6 +5,14 @@ Deprecated code produces compile-time warnings. These warning serve as notification to users that their code should be upgraded. The next major release will remove the deprecated code. +## Gazebo 11.1 to 11.3 + +The way Gazebo handles relative paths in SDF files has changed: + +* Until `11.1.0`: relative paths could only be resolved against environment variables `GAZEBO_MODEL_PATH` / `GAZEBO_RESOURCE_PATH`. +* On `11.2.0`: relative paths loaded from SDF files can only be resolved against the SDF file - the previous use case was broken by mistake. +* From `11.3.0`: relative paths are first resolved against the SDF file, and if that fails, fallback to the environment variables. + ## Gazebo 10.x to 11.0 ### Build system diff --git a/examples/plugins/actor_collisions/actor_collisions.world b/examples/plugins/actor_collisions/actor_collisions.world index 5f0cdea9ea..84dc1212b3 100644 --- a/examples/plugins/actor_collisions/actor_collisions.world +++ b/examples/plugins/actor_collisions/actor_collisions.world @@ -117,11 +117,11 @@ "/> - walk.dae + file://media/models/walk.dae 1.0 - walk.dae + file://media/models/walk.dae 1.000000 true diff --git a/examples/plugins/actor_collisions/actor_collisions.world.erb b/examples/plugins/actor_collisions/actor_collisions.world.erb index 2e86eb8bf1..8436875352 100644 --- a/examples/plugins/actor_collisions/actor_collisions.world.erb +++ b/examples/plugins/actor_collisions/actor_collisions.world.erb @@ -238,11 +238,11 @@ "/> - walk.dae + file://media/models/walk.dae 1.0 - walk.dae + file://media/models/walk.dae 1.000000 true diff --git a/gazebo/common/CommonIface.cc b/gazebo/common/CommonIface.cc index 271197013b..cdfa3f3fb2 100644 --- a/gazebo/common/CommonIface.cc +++ b/gazebo/common/CommonIface.cc @@ -513,7 +513,13 @@ std::string common::asFullPath(const std::string &_uri, #endif // Use platform-specific separator - return ignition::common::joinPaths(path, uri); + auto result = ignition::common::joinPaths(path, uri); + + // If result doesn't exist, return it unchanged + if (!exists(result)) + return _uri; + + return result; } ////////////////////////////////////////////////// diff --git a/gazebo/common/CommonIface.hh b/gazebo/common/CommonIface.hh index d7b5a86b1f..cc990d8a14 100644 --- a/gazebo/common/CommonIface.hh +++ b/gazebo/common/CommonIface.hh @@ -188,6 +188,7 @@ namespace gazebo /// If the URI is already a full path or contains a scheme, it won't be /// modified. /// If the URI is a relative path, the file path will be prepended. + /// If the URI and path combination doesn't exist, the URI won't be changed. /// \param[in] _uri URI, which can have a scheme, or be full or relative /// paths. /// \param[in] _filePath The path to a file in disk. diff --git a/gazebo/common/CommonIface_TEST.cc b/gazebo/common/CommonIface_TEST.cc index 5c5dc56a6b..995f2a4e0b 100644 --- a/gazebo/common/CommonIface_TEST.cc +++ b/gazebo/common/CommonIface_TEST.cc @@ -25,6 +25,7 @@ #include #include +#include "test_config.h" #include "gazebo/common/CommonIface.hh" #include "gazebo/common/SystemPaths.hh" #include "test/util.hh" @@ -259,20 +260,19 @@ TEST_F(CommonIface_TEST, directoryOps) ///////////////////////////////////////////////// TEST_F(CommonIface_TEST, asFullPath) { - const std::string relativeUriUnix{"meshes/collision.dae"}; - const std::string relativeUriWindows{"meshes\\collision.dae"}; - const std::string absoluteUriUnix{"/path/to/collision.dae"}; - const std::string absoluteUriWindows{"C:\\path\\to\\collision.dae"}; - const std::string schemeUri{"https://website.com/collision.dae"}; + auto src = std::string(PROJECT_SOURCE_PATH); + + const auto relativeUri = ignition::common::joinPaths("data", "box.dae"); + const auto absoluteUri = ignition::common::joinPaths(src, "test", "data", + "box.dae"); + const auto schemeUri{"https://website.com/collision.dae"}; // Empty path { const std::string path{""}; - EXPECT_EQ(relativeUriUnix, common::asFullPath(relativeUriUnix, path)); - EXPECT_EQ(relativeUriWindows, common::asFullPath(relativeUriWindows, path)); - EXPECT_EQ(absoluteUriUnix, common::asFullPath(absoluteUriUnix, path)); - EXPECT_EQ(absoluteUriWindows, common::asFullPath(absoluteUriWindows, path)); + EXPECT_EQ(relativeUri, common::asFullPath(relativeUri, path)); + EXPECT_EQ(absoluteUri, common::asFullPath(absoluteUri, path)); EXPECT_EQ(schemeUri, common::asFullPath(schemeUri, path)); } @@ -280,72 +280,38 @@ TEST_F(CommonIface_TEST, asFullPath) { const std::string path{"data-string"}; - EXPECT_EQ(relativeUriUnix, common::asFullPath(relativeUriUnix, path)); - EXPECT_EQ(relativeUriWindows, common::asFullPath(relativeUriWindows, path)); - EXPECT_EQ(absoluteUriUnix, common::asFullPath(absoluteUriUnix, path)); - EXPECT_EQ(absoluteUriWindows, common::asFullPath(absoluteUriWindows, path)); + EXPECT_EQ(relativeUri, common::asFullPath(relativeUri, path)); + EXPECT_EQ(absoluteUri, common::asFullPath(absoluteUri, path)); EXPECT_EQ(schemeUri, common::asFullPath(schemeUri, path)); } -#ifdef _WIN32 { - // Absolute Windows path - const std::string path{"C:\\abs\\path\\file"}; + // Absolute path + const auto path = ignition::common::joinPaths(src, "test", "path"); // Directory - EXPECT_EQ("C:\\abs\\path\\meshes\\collision.dae", - common::asFullPath(relativeUriUnix, path)); - EXPECT_EQ("C:\\abs\\path\\meshes\\collision.dae", - common::asFullPath(relativeUriWindows, path)); - // TODO(anyone) Support absolute Unix-style URIs on Windows - EXPECT_EQ(absoluteUriWindows, common::asFullPath(absoluteUriWindows, path)); + EXPECT_EQ(ignition::common::joinPaths(src, "test", "data", "box.dae"), + common::asFullPath(relativeUri, path)); + EXPECT_EQ(absoluteUri, common::asFullPath(absoluteUri, path)); EXPECT_EQ(schemeUri, common::asFullPath(schemeUri, path)); // File - auto filePath = ignition::common::joinPaths(path, "file.sdf"); - - EXPECT_EQ("C:\\abs\\path\\file\\meshes\\collision.dae", - common::asFullPath(relativeUriUnix, filePath)); - EXPECT_EQ("C:\\abs\\path\\file\\meshes\\collision.dae", - common::asFullPath(relativeUriWindows, filePath)); - // TODO(anyone) Support absolute Unix-style URIs on Windows - EXPECT_EQ(absoluteUriWindows, common::asFullPath(absoluteUriWindows, - filePath)); + const auto filePath = ignition::common::joinPaths(src, "test", "file.sdf"); + EXPECT_EQ(ignition::common::joinPaths(src, "test", "data", "box.dae"), + common::asFullPath(relativeUri, filePath)); + EXPECT_EQ(absoluteUri, common::asFullPath(absoluteUri, filePath)); EXPECT_EQ(schemeUri, common::asFullPath(schemeUri, filePath)); } -#else - { - // Absolute Unix path - const std::string path{"/abs/path/file"}; - - // Directory - EXPECT_EQ("/abs/path/meshes/collision.dae", - common::asFullPath(relativeUriUnix, path)); - EXPECT_EQ("/abs/path/meshes/collision.dae", - common::asFullPath(relativeUriWindows, path)); - EXPECT_EQ(absoluteUriUnix, common::asFullPath(absoluteUriUnix, path)); - // TODO(anyone) Support absolute Windows paths on Unix - EXPECT_EQ(schemeUri, common::asFullPath(schemeUri, path)); - - // File - auto filePath = ignition::common::joinPaths(path, "file.sdf"); - - EXPECT_EQ("/abs/path/file/meshes/collision.dae", - common::asFullPath(relativeUriUnix, filePath)); - EXPECT_EQ("/abs/path/file/meshes/collision.dae", - common::asFullPath(relativeUriWindows, filePath)); - EXPECT_EQ(absoluteUriUnix, common::asFullPath(absoluteUriUnix, filePath)); - // TODO(anyone) Support absolute Windows paths on Unix - EXPECT_EQ(schemeUri, common::asFullPath(schemeUri, filePath)); - } -#endif } ///////////////////////////////////////////////// TEST_F(CommonIface_TEST, fullPathsMesh) { - std::string filePath{"/tmp/model.sdf"}; - std::string relativePath{"meshes/collision.dae"}; + auto path = ignition::common::joinPaths(std::string(PROJECT_SOURCE_PATH), + "test"); + + std::string filePath = ignition::common::joinPaths(path, "model.sdf"); + std::string relativePath = ignition::common::joinPaths("data", "box.dae"); std::stringstream ss; ss << "" @@ -419,7 +385,7 @@ TEST_F(CommonIface_TEST, fullPathsMesh) auto uriElem = meshElem->GetElement("uri"); ASSERT_NE(nullptr, uriElem); - EXPECT_EQ(ignition::common::joinPaths("/tmp", relativePath), + EXPECT_EQ(ignition::common::joinPaths(path, relativePath), uriElem->Get()); } @@ -447,7 +413,7 @@ TEST_F(CommonIface_TEST, fullPathsMesh) auto uriElem = meshElem->GetElement("uri"); ASSERT_NE(nullptr, uriElem); - EXPECT_EQ(ignition::common::joinPaths("/tmp", relativePath), + EXPECT_EQ(ignition::common::joinPaths(path, relativePath), uriElem->Get()); } } @@ -455,8 +421,11 @@ TEST_F(CommonIface_TEST, fullPathsMesh) ///////////////////////////////////////////////// TEST_F(CommonIface_TEST, fullPathsActor) { - std::string filePath{"/tmp/actor.sdf"}; - std::string relativePath{"meshes/walk.dae"}; + auto path = ignition::common::joinPaths(std::string(PROJECT_SOURCE_PATH), + "test"); + + std::string filePath = ignition::common::joinPaths(path, "model.sdf"); + std::string relativePath = ignition::common::joinPaths("data", "box.dae"); std::stringstream ss; ss << "" @@ -499,14 +468,14 @@ TEST_F(CommonIface_TEST, fullPathsActor) ASSERT_NE(nullptr, skinElem); skinFilenameElem = skinElem->GetElement("filename"); ASSERT_NE(nullptr, skinFilenameElem); - EXPECT_EQ(ignition::common::joinPaths("/tmp", relativePath), + EXPECT_EQ(ignition::common::joinPaths(path, relativePath), skinFilenameElem->Get()); animationElem = actorElem->GetElement("animation"); ASSERT_NE(nullptr, animationElem); animationFilenameElem = animationElem->GetElement("filename"); ASSERT_NE(nullptr, animationFilenameElem); - EXPECT_EQ(ignition::common::joinPaths("/tmp", relativePath), + EXPECT_EQ(ignition::common::joinPaths(path, relativePath), animationFilenameElem->Get()); } diff --git a/test/worlds/actor_collisions.world b/test/worlds/actor_collisions.world index a1c14334d7..62e5f4ebe2 100644 --- a/test/worlds/actor_collisions.world +++ b/test/worlds/actor_collisions.world @@ -12,11 +12,11 @@ - walk.dae + file://media/models/walk.dae 1.0 - walk.dae + file://media/models/walk.dae 1.000000 true diff --git a/worlds/actor.world b/worlds/actor.world index f6a9c10c29..9bb88059de 100644 --- a/worlds/actor.world +++ b/worlds/actor.world @@ -10,12 +10,12 @@ - walk.dae + file://media/models/walk.dae 1.0 0 0 0 0 0 0 - walk.dae + file://media/models/walk.dae 1.000000 true diff --git a/worlds/actor_bvh.world b/worlds/actor_bvh.world index 050a4067ab..80aa28c2e4 100644 --- a/worlds/actor_bvh.world +++ b/worlds/actor_bvh.world @@ -14,7 +14,7 @@ 2 2 2 0 0 0 - walk.dae + file://media/models/walk.dae cmu-13_26.bvh diff --git a/worlds/cafe.world b/worlds/cafe.world index 81939d5ef3..b5ca5d392e 100644 --- a/worlds/cafe.world +++ b/worlds/cafe.world @@ -80,11 +80,11 @@ 0 1 1.25 0 0 0 - walk.dae + file://media/models/walk.dae 1.0 - walk.dae + file://media/models/walk.dae 1.000000 true @@ -106,11 +106,11 @@ -2 -2 1.25 0 0 0 - walk.dae + file://media/models/walk.dae 1.0 - walk.dae + file://media/models/walk.dae 1.000000 true diff --git a/worlds/profiler.world b/worlds/profiler.world index 7395c30324..f5657bed19 100644 --- a/worlds/profiler.world +++ b/worlds/profiler.world @@ -341,11 +341,11 @@ 0 5 1.25 0 0 0 - walk.dae + file://media/models/walk.dae 1.0 - walk.dae + file://media/models/walk.dae 1.000000 true