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 misleading deprecated warning when using launch arguments #106

Merged
merged 1 commit into from
Dec 13, 2019

Conversation

ivanpauno
Copy link
Member

Before this patch, running:
ros2 launch any_package any_launch_file some_arg:=some_value
ended up with a warning:

[WARN] [1576175943.679492205] [rcl]: Found remap rule 'asd:=bsd'. This syntax is deprecated. Use '--ros-args --remap asd:=bsd' instead.

That's misleading, because the only way of passing launch arguments is by adding key:=value at the end of the command.

To fix this, I did the following:

  • Stop using the LaunchContext arguments in the initialization of the rclpy.Context:
    Add a custom way of passing those arguments.
  • Pass no arguments in the creation of the rclpy.Context in ros2launch
    rclpy.init(args=[], context=context)

    Before, we were passing the same key values configurations that ros2launch received, which can cause problems (e.g.: being used as remapping rules).
  • Finally, deleted the argv option of ros2launch arguments. IIUC, it was always empty as the previous argument (launch configurations) was taking a variable number of options.

I'm not sure if this is the correct way to go, maybe we want to do something different.
e.g.: Filtering the launch configurations from the arguments, and use the others as the arguments of the rclpy.Context. IMO, that may be confusing.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added bug Something isn't working in review labels Dec 12, 2019
@ivanpauno ivanpauno self-assigned this Dec 12, 2019
@ivanpauno
Copy link
Member Author

@Karsten1987 FYI.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

I don't know if we need to pass ROS arguments to the internal launch_ros node or not, but for now it's probably ok to not do so.

@ivanpauno
Copy link
Member Author

ivanpauno commented Dec 13, 2019

  • Linux Build Status
  • AArch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno ivanpauno merged commit 35c84cb into master Dec 13, 2019
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/fix-deprecated-warning branch December 13, 2019 19:47
@ivanpauno
Copy link
Member Author

I don't know if we need to pass ROS arguments to the internal launch_ros node or not, but for now it's probably ok to not do so.

As far as I see, we don't need to pass remapping rules or parameters.
The other ROS arguments are logging options, for which we could provide a different way of configuring that.

@ivanpauno ivanpauno added this to Needs Backport in Eloquent Patch Release 1 Dec 13, 2019
ivanpauno added a commit that referenced this pull request Jan 17, 2020
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@mjcarroll mjcarroll removed this from Needs Backport in Eloquent Patch Release 1 Jan 17, 2020
ivanpauno added a commit that referenced this pull request Jan 17, 2020
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: ivan <ivanpauno@gmail.com>
ivanpauno added a commit that referenced this pull request Jan 21, 2020
* Add pid to launch_ros node name as suffix

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
Signed-off-by: ivan <ivanpauno@gmail.com>

* Pass the node-name attribute through the substitution parser (#101)

Signed-off-by: ivan <ivanpauno@gmail.com>

* Fix push-ros-namespace in xml/yaml launch files (#100)

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

* Maintain order of parameters regarding name and from (#99)

Signed-off-by: Brian Ezequiel Marchi <brian.marchi65@gmail.com>
Signed-off-by: ivan <ivanpauno@gmail.com>

* Use imperative mood in constructor docstrings. (#103)

* Use imperative mood in constructor docstrings.

Fixes D401 in pycodestyle 5.0.0 and flake8.

Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* Use imperative mood in docstring.

Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* Use imperative mood in docstrings.

Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>
Signed-off-by: ivan <ivanpauno@gmail.com>

* Fix misleading deprecated warnings when using launch arguments (#106)

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

* check for shutdown while waiting for a service response to avoid hang during shutdown (#104)

* check for shutdown while waiting for a service response to avoid hang during shutdown

Signed-off-by: William Woodall <william@osrfoundation.org>

* fix typo in logger call

Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: ivan <ivanpauno@gmail.com>

* Fix frontend topic remapping (#111)

* Add frontend remap test

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

* Pass data_type parameter to remap entity

This resolves an issue where frontend remaps are not parsed.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: ivan <ivanpauno@gmail.com>

Co-authored-by: Brian Marchi <brian.marchi65@gmail.com>
Co-authored-by: Grey <grey@openrobotics.org>
Co-authored-by: Steven! Ragnarök <nuclearsandwich@users.noreply.github.com>
Co-authored-by: William Woodall <william+github@osrfoundation.org>
Co-authored-by: Jacob Perron <jacob@openrobotics.org>
@ivanpauno
Copy link
Member Author

#120 reintroduced this bug.

I'm working on a fix and a regression test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working in review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants