Support node namespaces #95

wjwwood · 2017-03-29T07:51:53Z

This is the first change (as it includes the actual storage for the namespace) in a series of several changes up and down the stack. The purpose of these pull requests are to add the notion of a namespace for each node. This namespace will later be used to expand relative topic names and remapping rules.

This pr builds on #93 and include the commits from there. It should be rebased after that is merged.

This particular pr is relatively simple as it only adds storage for the namespace and adds it to the API for rmw_create_node(), see 14cd727.

I'm going to leave this "in progress" until I get the other pull requests opened.

wjwwood · 2017-03-29T08:16:48Z

Ok, now that I got all the pr's open, and took a second look at it, I think these are all ready for review and to be merged after #93.

dirk-thomas · 2017-03-29T15:49:37Z

rmw/include/rmw/rmw.h

@@ -88,7 +88,7 @@ rmw_init(void);
 RMW_PUBLIC
 RMW_WARN_UNUSED
 rmw_node_t *
-rmw_create_node(const char * name, size_t domain_id);
+rmw_create_node(const char * name, const char * name_space, size_t domain_id);


Shouldn't this be one word: namespace?

It's a keyword in c++ so when this header is included in cpp files it still fails to compile, even though this is inside an extern c block.

That was an obvious one 🤕

I am not sure I like the idea of "separating" the variable name like this though. Some style guide recommend appending an underscore to names which collide with keywords (e.g. PEP 8). Even though this has the potential to collide with the C++ guide for class members I would prefer namespace_ over name_space in this case.

I agree, I'll go through and update all of the prs soon.

Ok, I think I replaced all instances of name_space with namespace_ in all the prs.

dirk-thomas · 2017-03-29T15:55:41Z

Can you please also create a PR for rmw_opensplice.

wjwwood · 2017-03-29T18:03:37Z

Why do I need to open a pr for opensplice?

dirk-thomas · 2017-03-29T18:17:50Z

Why do I need to open a pr for opensplice?

Because we want to continue to be able to build with rmw_opensplice in order to test it in the future. Especially if the changes are as minimal as in this case.

wjwwood · 2017-03-30T02:07:25Z

I opened ros2/rmw_opensplice#164 and made this commit ros2/rmw_connext@2e65593 in order to move this all along, but honestly I think it's waste of time.

I think we should rethink our policy of having opensplice and connext dynamic try to compile on a fresh clone, by disabling them in upstream with an AMENT_IGNORE, add a note to their README's telling people how to re-enable them, and have the farm remove those files when the options are selected.

wjwwood · 2017-04-04T03:49:48Z

I rebased this after #93 was merged.

wjwwood · 2017-04-05T00:25:26Z

Ok, with two new pr's to fix linger issues I didn't see on my mac, here is CI post rebase:

Linux:
macOS:
Windows:

Still looking for reviews for some of these pr's. Thanks!

wjwwood · 2017-04-05T03:50:06Z

@Karsten1987 can you review this pr too?

wjwwood · 2017-04-05T08:14:26Z

New CI after addressing comments:

Linux:
macOS:
Windows:
- (the test failures appear to be unrelated/flaky)

dirk-thomas

Beside the question this looks good to me.

dirk-thomas · 2017-04-05T14:55:45Z

rmw/include/rmw/validate_node_name.h

@@ -37,7 +37,7 @@ extern "C"
 *
 * - must not be an empty string
 * - must only contain alphanumeric characters and underscores (a-z|A-Z|0-9|_)
- * - must not start with an number
+ * - must start with an alphabetical character


Maybe I missed the discussion in another thread. Why do we want to prohibit node name starting with an underscore?

I think I was just trying to simplify the rules, but I forgot about the underscores. The implementation didn't actually change. I'll switch it back b6f79db.

wjwwood · 2017-04-07T00:11:29Z

In 383db0f I added rmw_validate_namespace() as requested here: ros2/rcl#112 (comment)

In 383db0f I made invalid_index optional as requested here: ros2/rcl#112 (comment)

wjwwood · 2017-04-07T02:57:48Z

I believe all the comments on all pull requests have been addressed, I'm looking for final reviews. I'll probably merge later tonight if no one says anything.

Here's the latest CI:

Linux:
Linux-aarch64:
macOS:
Windows:
- (flaky python service requester test failure only)

dirk-thomas · 2017-04-07T16:43:30Z

rmw/src/validate_namespace.c

+        break;
+      default:
+        RMW_SET_ERROR_MSG(
+          "rmw_validate_namespace(): unknown rmw_validate_topic_name() validation result"


It would be good to include the value of t_validation_result in the error message.

I think this should work, it does on macOS at least. I'll need to run CI again to check for warning on Windows: 833c760

dirk-thomas

Beside the last comment this LGTM.

wjwwood · 2017-04-08T09:02:40Z

Ok, I addressed all the comments and here is passing CI:

Linux:
Linux-aarch64:
- (existing test failures)
macOS:
Windows:
- (failing tests are flaky)

wjwwood · 2017-04-08T09:03:35Z

I'm going to go ahead and merge, if there are more comments I'll address them afterwards.

wjwwood added enhancement New feature or request in progress Actively being worked on (Kanban column) labels Mar 29, 2017

wjwwood self-assigned this Mar 29, 2017

wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 29, 2017

dirk-thomas reviewed Mar 29, 2017

View reviewed changes

wjwwood mentioned this pull request Mar 30, 2017

update to support new rmw API with namespaces ros2/rmw_opensplice#164

Merged

wjwwood mentioned this pull request Mar 31, 2017

add rmw_validate_node_name() #93

Merged

wjwwood added 3 commits April 3, 2017 20:48

add rmw_validate_node_name()

621753e

add name_space to the node

90cea5a

name_space -> namespace_

0ebcc3e

wjwwood force-pushed the node_namespace branch from 43426de to 0ebcc3e Compare April 4, 2017 03:48

This was referenced Apr 4, 2017

pass new namespace_ argument through rmw_create_node ros2/rmw_implementation#19

Merged

pass namespace to rclcpp::Node constructors ros2/realtime_support#53

Merged

dirk-thomas approved these changes Apr 5, 2017

View reviewed changes

wjwwood added 4 commits April 6, 2017 15:19

fix docs

b6f79db

fix doc typo

e1eb636

add rmw_validate_namespace()

383db0f

make invalid_index optional (can pass NULL)

833c760

dirk-thomas reviewed Apr 7, 2017

View reviewed changes

dirk-thomas approved these changes Apr 7, 2017

View reviewed changes

wjwwood added 3 commits April 7, 2017 15:21

include the validation result value when it is unknown

2421302

use _snprintf_s on windows

6520d6f

uncrustify

d4fb6d3

wjwwood merged commit c6145ce into master Apr 8, 2017

wjwwood deleted the node_namespace branch April 8, 2017 09:03

wjwwood removed the in review Waiting for review (Kanban column) label Apr 8, 2017

esteve added a commit to ros2-java/ros2_java that referenced this pull request Apr 10, 2017

Updates for the rmw/rcl changes to support namespaces. See ros2/rmw#95

3494ab2

dhood mentioned this pull request May 1, 2017

New windows debug exceptions ros2/build_farmer#19

Closed

dirk-thomas mentioned this pull request Jun 27, 2018

rmw_get_node_names needs to include namespaces #142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support node namespaces #95

Support node namespaces #95

wjwwood commented Mar 29, 2017

wjwwood commented Mar 29, 2017

dirk-thomas Mar 29, 2017

wjwwood Mar 29, 2017

dirk-thomas Mar 29, 2017

wjwwood Mar 29, 2017

wjwwood Mar 30, 2017

dirk-thomas commented Mar 29, 2017

wjwwood commented Mar 29, 2017

dirk-thomas commented Mar 29, 2017

wjwwood commented Mar 30, 2017

wjwwood commented Apr 4, 2017

wjwwood commented Apr 5, 2017

wjwwood commented Apr 5, 2017

wjwwood commented Apr 5, 2017

dirk-thomas left a comment

dirk-thomas Apr 5, 2017

wjwwood Apr 6, 2017

wjwwood commented Apr 7, 2017

wjwwood commented Apr 7, 2017

dirk-thomas Apr 7, 2017

wjwwood Apr 7, 2017

dirk-thomas left a comment

wjwwood commented Apr 8, 2017

wjwwood commented Apr 8, 2017

Support node namespaces #95

Support node namespaces #95

Conversation

wjwwood commented Mar 29, 2017

wjwwood commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Mar 29, 2017

wjwwood commented Mar 29, 2017

dirk-thomas commented Mar 29, 2017

wjwwood commented Mar 30, 2017

wjwwood commented Apr 4, 2017

wjwwood commented Apr 5, 2017

wjwwood commented Apr 5, 2017

wjwwood commented Apr 5, 2017

dirk-thomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood commented Apr 7, 2017

wjwwood commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas left a comment

Choose a reason for hiding this comment

wjwwood commented Apr 8, 2017

wjwwood commented Apr 8, 2017