Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ros::this_node::getNamespace() ought to return sanitized namespace #1094

Closed
lucasw opened this issue Jul 11, 2017 · 6 comments
Closed

ros::this_node::getNamespace() ought to return sanitized namespace #1094

lucasw opened this issue Jul 11, 2017 · 6 comments

Comments

@lucasw
Copy link
Contributor

lucasw commented Jul 11, 2017

  ros::init(argc, argv, "ns_test");
  std::cout << ros::this_node::getNamespace() << std::endl;
  ros::NodeHandle nh("~");
  std::cout << nh.getNamespace() << std::endl;

This is fine

$ rosrun ns_test ns_test __ns:=foo
/foo
/foo/ns_test

This ought to print the same

rosrun ns_test ns_test __ns:=/foo
//foo
/foo/ns_test

An additional / was prefixed to the provided namespace.

and this (which is somewhat ridiculous, but why not fix it also)

rosrun ns_test ns_test __ns:=//foo//
///foo//
/foo/ns_test

The namespace gets cleaned up getting into the NodeHandle, but the this_node returns exactly what was on the command line plus the additional forward slash.

Using the released kinetic version currently.

@wjwwood
Copy link
Member

wjwwood commented Jul 11, 2017

Makes sense to me, though it might be someone is relying on this behavior (they want what came in over the CLI, not the sanitized version), but we can definitely change it for newer distributions and consider doing it anyways for existing distributions.

It's definitely worth making a pr against lunar-devel if you have the time.

@lucasw
Copy link
Contributor Author

lucasw commented Jul 14, 2017

I notice roslaunch

<node name="ns_test"
    pkg="ns_test"
    type="ns_test"
    output="screen"
    ns="//foo//"/>

prints out

NODES
  //foo//
    ns_test (ns_test/ns_test)
...
process[/foo//ns_test-1]: started with pid [26179]
...
[/foo//ns_test-1] process has finished cleanly

The NODES part prints the namespace verbatim, while the process messages are removing a leading forward slash.

@lucasw
Copy link
Contributor Author

lucasw commented Jul 14, 2017

If there were to be a namespace cleaning test test/test_roscpp/test/launch/namespace.xml and test/test_roscpp/test/src/namespaces.cpp could be the basis for it.

(and build and test with

catkin_make test_roscpp-namespaces-clean
rostest src/ros_comm/test/test_roscpp/test/launch/namespaces-clean.xml

)

lucasw added a commit to lucasw/ros_comm that referenced this issue Jul 15, 2017
…ut also prefix a slash as needed, so accommodating that here. ros#1094
lucasw added a commit to lucasw/ros_comm that referenced this issue Jul 18, 2017
@dirk-thomas
Copy link
Member

The argument __ns is not sanitized at the moment and the user is expected to pass a non-global namespace without a trailing slash.

I am not sure why you want to pass namespaces with leading and trailing namespaces in the first place but since the provided PR is straight forward it shouldn't be a problem to do the extra sanitization you proposed. Thanks for create the patch.

dirk-thomas pushed a commit that referenced this issue Jul 19, 2017
#1100)

* Clean the namespace to get rid of double or trailing forward slashes #1094

* Get rid of extra slashes in namespace #1094

* names::clean() will turn a '/' into '' (is that a bug or desired?), but also prefix a slash as needed, so accommodating that here.  #1094

* Simplifying and fixing, now namespaces.xml test passes. #1094
@dirk-thomas
Copy link
Member

Addressed by #1100.

@k-okada
Copy link
Contributor

k-okada commented Jul 25, 2017

I ran into similar situation and still confused that on python , we'll get

$ ROS_NAMESPACE=/hoge/ python -c 'import rospy; print(rospy.get_namespace())'
/hoge/
$ ROS_NAMESPACE=/hoge python -c 'import rospy; print(rospy.get_namespace())'
/hoge/
$ ROS_NAMESPACE=hoge python -c 'import rospy; print(rospy.get_namespace())'
/hoge/
$ ROS_NAMESPACE=hoge/ python -c 'import rospy; print(rospy.get_namespace())'
/hoge/

on rospy, but on roscpp (after applied this patch), we'll get

$ ROS_NAMESPACE=/hoge/ ../../devel/lib/test_namespace/test_namespace
[ INFO] [1501025799.775532508]: namespace -> /hoge
$ ROS_NAMESPACE=/hoge ../../devel/lib/test_namespace/test_namespace
[ INFO] [1501025804.109156084]: namespace -> /hoge
$ ROS_NAMESPACE=hoge ../../devel/lib/test_namespace/test_namespace
[ INFO] [1501025806.913568473]: namespace -> /hoge
$ ROS_NAMESPACE=hoge/ ../../devel/lib/test_namespace/test_namespace
[ INFO] [1501025810.195600461]: namespace -> /hoge

the source code is

$ cat src/test_namespace.cpp
#include <ros/ros.h>


int main(int argc, char **argv) {
  ros::init(argc, argv, "test_namespace");
  ROS_INFO_STREAM("namespace -> " <<  ros::this_node::getNamespace());
}

so question is, do we have tailing '/' on namespace or not.

sputnick1124 pushed a commit to sputnick1124/ros_comm that referenced this issue Jul 30, 2017
ros#1100)

* Clean the namespace to get rid of double or trailing forward slashes ros#1094

* Get rid of extra slashes in namespace ros#1094

* names::clean() will turn a '/' into '' (is that a bug or desired?), but also prefix a slash as needed, so accommodating that here.  ros#1094

* Simplifying and fixing, now namespaces.xml test passes. ros#1094
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants