roscpp: Fixed remapping of private parameters in anonymous nodes

clients/roscpp/src/libros/this_node.cpp

@@ -167,6 +167,22 @@ void ThisNode::init(const std::string& name, const M_string& remappings, uint32_
     char buf[200];
     std::snprintf(buf, sizeof(buf), "_%llu", (unsigned long long)WallTime::now().toNSec());
     name_ += buf;
+
+    // names::init() above did not yet know the full name of the node, so the
+    // remappings for private names it set are wrong. This second call fixes
+    // that. However, the old remappings (without the anonymizing part) still
+    // remain set. There is no easy way to remove them, so we keep them there.
+    M_string privateRemappings;
+    for (const auto& leftRight : remappings)
+    {
+      const auto& left = leftRight.first;
+      if (!left.empty() && left[0] == '~')
+      {
+        privateRemappings[left] = leftRight.second;
+      }
+    }
+
+    names::init(privateRemappings);
   }
 
   ros::console::setFixedFilterToken("node", name_);

test/test_roscpp/test/CMakeLists.txt

@@ -156,3 +156,5 @@ add_rostest(launch/multiple_latched_publishers.xml)
 
 # Test spinners
 add_rostest(launch/spinners.xml)
+
+add_rostest(launch/anonymous_node_remap_private.xml DEPENDENCIES ${PROJECT_NAME}-anonymous_node_remap_private)

test/test_roscpp/test/launch/anonymous_node_remap_private.xml

@@ -0,0 +1,23 @@
+<!-- SPDX-License-Identifier: BSD-3-Clause -->
+<!-- SPDX-FileCopyrightText: Open Source Robotics Foundation -->
+<launch>
+  <!-- Test that the private remap works in case no __name:= arg is passed to ros::init() -->
+  <!-- This simulates manually running the node via rosrun -->
+  <test test-name="anonymous_node_remap_private_noname" pkg="test_roscpp"
+        type="test_roscpp-anonymous_node_remap_private" args="noname">
+    <remap from="~topic" to="/remapped" />
+  </test>
+
+  <!-- Test that the private remap works in case an explicit name for the node is set -->
+  <test test-name="anonymous_node_remap_private_name" pkg="test_roscpp"
+        type="test_roscpp-anonymous_node_remap_private" args="test_node" name="test_node">
+    <remap from="~topic" to="/remapped" />
+  </test>
+
+  <!-- Test that the private remap works in case an explicit anonymous name for the node is set -->
+  <test test-name="anonymous_node_remap_private_anonname" pkg="test_roscpp"
+        type="test_roscpp-anonymous_node_remap_private"
+        args="$(anon test_node)" name="$(anon test_node)">
+    <remap from="~topic" to="/remapped" />
+  </test>
+</launch>

test/test_roscpp/test/src/CMakeLists.txt

@@ -227,6 +227,9 @@ add_executable(${PROJECT_NAME}-subscriber EXCLUDE_FROM_ALL subscriber.cpp)
 target_link_libraries(${PROJECT_NAME}-subscriber ${catkin_LIBRARIES})
 add_dependencies(${PROJECT_NAME}-subscriber ${std_msgs_EXPORTED_TARGETS})
 
+add_executable(${PROJECT_NAME}-anonymous_node_remap_private EXCLUDE_FROM_ALL anonymous_node_remap_private.cpp)
+target_link_libraries(${PROJECT_NAME}-anonymous_node_remap_private ${GTEST_LIBRARIES} ${catkin_LIBRARIES})
+
 if(TARGET tests)
   add_dependencies(tests
     ${PROJECT_NAME}-handles
@@ -294,6 +297,7 @@ if(TARGET tests)
     ${PROJECT_NAME}-stamped_topic_statistics_empty_timestamp
     ${PROJECT_NAME}-topic_statistic_frequency
     ${PROJECT_NAME}-multiple_latched_publishers
+    ${PROJECT_NAME}-anonymous_node_remap_private
   )
 endif()
 
@@ -361,3 +365,4 @@ add_dependencies(${PROJECT_NAME}-stamped_topic_statistics_empty_timestamp
  ${${PROJECT_NAME}_EXPORTED_TARGETS})
 add_dependencies(${PROJECT_NAME}-topic_statistic_frequency ${${PROJECT_NAME}_EXPORTED_TARGETS})
 add_dependencies(${PROJECT_NAME}-multiple_latched_publishers ${${PROJECT_NAME}_EXPORTED_TARGETS})
+add_dependencies(${PROJECT_NAME}-anonymous_node_remap_private ${${PROJECT_NAME}_EXPORTED_TARGETS})

test/test_roscpp/test/src/anonymous_node_remap_private.cpp

@@ -0,0 +1,88 @@
+/*
+ * SPDX-License-Identifier: BSD-3-Clause
+ * SPDX-FileCopyrightText: Open Source Robotics Foundation
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in the
+ *       documentation and/or other materials provided with the distribution.
+ *     * Neither the name of the Willow Garage, Inc. nor the names of its
+ *       contributors may be used to endorse or promote products derived from
+ *       this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <algorithm>
+#include <string>
+
+#include <ros/ros.h>
+#include <gtest/gtest.h>
+
+std::string name_;
+bool removeName_ {false};
+
+/**
+ * \brief This test tests that even anonymous nodes launched directly via rosrun
+ *        (without an explicit __name:= arg) will get the private names remapped
+ *        correctly.
+ */
+TEST(AnonymousNodeRemapPrivate, test)
+{
+  ros::NodeHandle pnh("~");
+
+  if (!removeName_)
+  {
+    EXPECT_EQ(name_, ros::this_node::getName());
+    EXPECT_EQ(name_, pnh.getNamespace());
+  }
+
+  const auto remappedName = "/remapped";
+  EXPECT_EQ(remappedName, pnh.resolveName("topic"));
+  EXPECT_EQ(remappedName, ros::names::resolve("~topic"));
+}
+
+int main(int argc, char** argv)
+{
+  testing::InitGoogleTest(&argc, argv);
+
+  // the first arg of this test has to be the name of the test node, or "noname"
+  // "noname" means that no __name:= will be passed to node::init().
+
+  name_ = ros::names::resolve(argv[1], false);
+  removeName_ = name_ == "/noname";
+
+  // remove __name:= so that ros::init() does not use it
+  if (removeName_)
+  {
+    for (int i = 0; i < argc; ++i)
+    {
+      if (std::string(argv[i]).find("__name:=") == 0)
+      {
+        std::swap(argv[i], argv[argc - 1]);
+        --argc;
+        break;
+      }
+    }
+  }
+
+  ros::init(argc, argv, "anonymous_node_remap_private",
+            ros::InitOption::AnonymousName);
+  return RUN_ALL_TESTS();
+}