Fix usage of relative paths with environment variables (#2890)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

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

examples/plugins/actor_collisions/actor_collisions.world

@@ -117,11 +117,11 @@
       "/>
     </plugin>
     <skin>
-      <filename>walk.dae</filename>
+      <filename>file://media/models/walk.dae</filename>
       <scale>1.0</scale>
     </skin>
     <animation name="walking">
-      <filename>walk.dae</filename>
+      <filename>file://media/models/walk.dae</filename>
       <scale>1.000000</scale>
       <interpolate_x>true</interpolate_x>
     </animation>

examples/plugins/actor_collisions/actor_collisions.world.erb

@@ -238,11 +238,11 @@
       "/>
     </plugin>
     <skin>
-      <filename>walk.dae</filename>
+      <filename>file://media/models/walk.dae</filename>
       <scale>1.0</scale>
     </skin>
     <animation name="walking">
-      <filename>walk.dae</filename>
+      <filename>file://media/models/walk.dae</filename>
       <scale>1.000000</scale>
       <interpolate_x>true</interpolate_x>
     </animation>

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;
 }
 
 //////////////////////////////////////////////////

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.

gazebo/common/CommonIface_TEST.cc

@@ -25,6 +25,7 @@
 #include <ignition/common/Filesystem.hh>
 #include <sdf/parser.hh>
 
+#include "test_config.h"
 #include "gazebo/common/CommonIface.hh"
 #include "gazebo/common/SystemPaths.hh"
 #include "test/util.hh"
@@ -259,93 +260,58 @@ 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));
   }
 
   // Data string
   {
     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 << "<sdf version='" << SDF_VERSION << "'>"
@@ -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<std::string>());
   }
 
@@ -447,16 +413,19 @@ 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<std::string>());
   }
 }
 
 /////////////////////////////////////////////////
 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 << "<sdf version='" << SDF_VERSION << "'>"
@@ -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<std::string>());
 
   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<std::string>());
 }
 

test/worlds/actor_collisions.world

@@ -12,11 +12,11 @@
 
   <actor name="actor">
     <skin>
-      <filename>walk.dae</filename>
+      <filename>file://media/models/walk.dae</filename>
       <scale>1.0</scale>
     </skin>
     <animation name="walking">
-      <filename>walk.dae</filename>
+      <filename>file://media/models/walk.dae</filename>
       <scale>1.000000</scale>
       <interpolate_x>true</interpolate_x>
     </animation>

worlds/actor.world

@@ -10,12 +10,12 @@
       </include>
       <actor name="actor">
          <skin>
-            <filename>walk.dae</filename>
+            <filename>file://media/models/walk.dae</filename>
             <scale>1.0</scale>
          </skin>
          <pose>0 0 0 0 0 0</pose>
          <animation name="walking">
-            <filename>walk.dae</filename>
+            <filename>file://media/models/walk.dae</filename>
             <scale>1.000000</scale>
             <interpolate_x>true</interpolate_x>
          </animation>

worlds/actor_bvh.world

@@ -14,7 +14,7 @@
     <actor name="actor">
       <pose>2 2 2 0 0 0</pose>
       <skin>
-        <filename>walk.dae</filename>
+        <filename>file://media/models/walk.dae</filename>
       </skin>
       <animation name="anim">
         <filename>cmu-13_26.bvh</filename>

worlds/cafe.world

@@ -80,11 +80,11 @@
     <actor name="actor1">
       <pose>0 1 1.25 0 0 0</pose>
       <skin>
-        <filename>walk.dae</filename>
+        <filename>file://media/models/walk.dae</filename>
         <scale>1.0</scale>
       </skin>
       <animation name="walking">
-        <filename>walk.dae</filename>
+        <filename>file://media/models/walk.dae</filename>
         <scale>1.000000</scale>
         <interpolate_x>true</interpolate_x>
       </animation>
@@ -106,11 +106,11 @@
     <actor name="actor2">
       <pose>-2 -2 1.25 0 0 0</pose>
       <skin>
-        <filename>walk.dae</filename>
+        <filename>file://media/models/walk.dae</filename>
         <scale>1.0</scale>
       </skin>
       <animation name="walking">
-        <filename>walk.dae</filename>
+        <filename>file://media/models/walk.dae</filename>
         <scale>1.000000</scale>
         <interpolate_x>true</interpolate_x>
       </animation>

worlds/profiler.world

@@ -341,11 +341,11 @@
     <actor name="actor1">
       <pose>0 5 1.25 0 0 0</pose>
       <skin>
-        <filename>walk.dae</filename>
+        <filename>file://media/models/walk.dae</filename>
         <scale>1.0</scale>
       </skin>
       <animation name="walking">
-        <filename>walk.dae</filename>
+        <filename>file://media/models/walk.dae</filename>
         <scale>1.000000</scale>
         <interpolate_x>true</interpolate_x>
       </animation>