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

Fix no specified namespace #153

Merged
merged 5 commits into from
Jun 22, 2020
Merged

Conversation

ivanpauno
Copy link
Member

If the user doesn't explicitly pass a namespace to Node action neither uses PushRosNamespace, the node namespace can't be known at launch time neither the fully qualified node name:

if self.__expanded_node_namespace != '':
cmd_extension = ['-r', LocalSubstitution("ros_specific_arguments['ns']")]
self.cmd.extend([normalize_to_list_of_substitutions(x) for x in cmd_extension])
validate_namespace(self.__expanded_node_namespace)

This PR fixes that incorrect assumption.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
launch_ros/launch_ros/actions/node.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/actions/node.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/actions/node.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/actions/node.py Outdated Show resolved Hide resolved
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno requested a review from hidmic June 22, 2020 16:47
@ivanpauno
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno ivanpauno merged commit e8894ce into master Jun 22, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/fix-no-specified-namespace branch June 22, 2020 19:08
sloretz added a commit to ros2/demos that referenced this pull request Jun 23, 2020
Fixes regression introduced by ros2/launch_ros#153 

Alternative to ros2/launch_ros#157
sloretz added a commit to ros2/demos that referenced this pull request Jun 23, 2020
Fixes regression introduced by ros2/launch_ros#153

Alternative to ros2/launch_ros#157

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@ivanpauno
Copy link
Member Author

I'm not sure of how this can be backported.
It should be somehow combined with #157.

I will try to figure out if there's a no API breaking way of backporting the fix.

@jacobperron
Copy link
Member

@ivanpauno Friendly ping. Let me know if you still plan to look at backporting this change and #157. I think backporting those changes will make other backports easier to do (currently I get conflicts since they touch similar lines of code).

@ivanpauno
Copy link
Member Author

@ivanpauno Friendly ping. Let me know if you still plan to look at backporting this change and #157. I think backporting those changes will make other backports easier to do (currently I get conflicts since they touch similar lines of code).

It's hard to come up with a backport of this.
This is fixing a bug, but it's also slightly changing behavior in ways that can break existing use cases.

I would prefer not backporting this, except we get an explicit request on doing so.
In that case, I will try to come up with something.

jacobperron pushed a commit that referenced this pull request Sep 18, 2020
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
jacobperron added a commit that referenced this pull request Oct 5, 2020
* Fix no specified namespace (#153)

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

* Make namespace parameter mandatory in LifecycleNode constructor (#157)

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

* Make namespace argument optional

This maintains backwards compatibility.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Remove test for construction without namespace

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Treat empty namespace as unset

This keeps the current behavior in Foxy.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Address feedback and fix bug in LifecycleNode

If no namespace (or empty string) is provided, assume global namespace.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants