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

add ability to define and pass launch arguments to launch files #123

Merged
merged 26 commits into from
Sep 4, 2018

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Aug 14, 2018

This pull request is ready for review.

To try it, you can use the example files in ros2launch, e.g.:

% ros2 launch --show-args ./path/to/ros2launch/examples/example.launch.py
...

% ros2 launch ./path/to/ros2launch/examples/example.launch.py
...

% ros2 launch ./path/to/ros2launch/examples/example.launch.py node_prefix:=foo_
...

fixes #107

@wjwwood wjwwood self-assigned this Aug 14, 2018
@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Aug 14, 2018
@wjwwood
Copy link
Member Author

wjwwood commented Aug 22, 2018

@nuclearsandwich c45a877 should fix Python 3.5 support. Why it fixes it 🤷‍♂️ .

@dhood
Copy link
Member

dhood commented Aug 22, 2018

Why it fixes it 🤷‍♂️

I tracked this down once in the past: 72ae169#r195963696

@dhood
Copy link
Member

dhood commented Aug 22, 2018

edit (bad link): 72ae169#r195963696

@dhood
Copy link
Member

dhood commented Aug 22, 2018

github is changing my links, sorry for spam. this should work

@mikaelarguedas
Copy link
Member

github is changing my links, sorry for spam. this should work

Yes github now perform differently if you get the comment from specific commit. Getting the comment link from the PR page should work though: #81 (comment)

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

Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 23, 2018
@wjwwood
Copy link
Member Author

wjwwood commented Aug 23, 2018

Ok, this one is ready for review, CI:

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

Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

I mentioned this directly to you, but if you run your example launch file without the demo executables available (I removed via rm -rf), the launch executable will report an error, and then go into a state where you cannot stop it, even with multiple Ctrl-C's.

I have put the output here: https://gist.github.com/mjcarroll/65ee17100613d41ea503408ba86c183e

will be exposed as arguments when that launch description is included, e.g.
as additional parameters in the
:py:class:`launch.actions.IncludeLaunchDescription` action or as
command-line arguments when launch with ``ros2 launch ...``.
Copy link
Member

Choose a reason for hiding this comment

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

launched

Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
Copy link
Member Author

wjwwood commented Aug 30, 2018

So, I think that if you change the name of the executable that the example launch file is looking for or delete it (as you did), then you'll get the same behavior on master. So I don't really want to have to fix that on this pull request. It's already being tracked here: #112 I hope to have time to look into that tomorrow.

Can you re-review with the above in mind, only testing out the new features (passing of arguments, etc...).

@wjwwood
Copy link
Member Author

wjwwood commented Aug 30, 2018

The only difference with master is that I don't print out the traceback by default (only on debug), which I changed in this pr, but it still produces the same exception and runtime behavior.

… exception

Signed-off-by: William Woodall <william@osrfoundation.org>
The event handlers need to be setup before the other lines, but are invalid if setup does not complete successfully.
@wjwwood
Copy link
Member Author

wjwwood commented Aug 30, 2018

I looked into it and the fix was pretty simple, so I went ahead and committed it in this pr: b4969f4

Basically that just lets already running things shutdown when an unhandled exception is received. I was never able to reproduce the "cannot even ctrl-c it" scenario, even before.

I also committed f077ab6 to avoid invalid state in the execute process action if the exception originated there (basically it registers event handlers, and then an exception occurs, making the event handlers invalid).

Also, I'd appreciate feedback on 4dbd9bb which is restoring the state of master which is that exceptions are reported as a single line and the traceback is only in the debug output.

@wjwwood wjwwood merged commit aaacf5c into master Sep 4, 2018
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Sep 4, 2018
@wjwwood wjwwood deleted the feature/launch_arguments branch September 4, 2018 16:41
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

Successfully merging this pull request may close these issues.

[launch] add new concept for LaunchArgument
4 participants