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

Projects
None yet
4 participants
@wjwwood
Copy link
Member

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 label Aug 14, 2018

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Aug 22, 2018

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

@dhood

This comment has been minimized.

Copy link
Member

dhood commented Aug 22, 2018

Why it fixes it 🤷‍♂️

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

@dhood

This comment has been minimized.

Copy link
Member

dhood commented Aug 22, 2018

edit (bad link): 72ae169#r195963696

@dhood

This comment has been minimized.

Copy link
Member

dhood commented Aug 22, 2018

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

@mikaelarguedas

This comment has been minimized.

Copy link
Contributor

mikaelarguedas commented Aug 22, 2018

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)

@wjwwood wjwwood force-pushed the feature/launch_arguments branch from 3540e2a to e01ed64 Aug 23, 2018

wjwwood added some commits Aug 14, 2018

add accessor for the Condition of an Action
Signed-off-by: William Woodall <william@osrfoundation.org>
do not automatically push/pop configs when including
Signed-off-by: William Woodall <william@osrfoundation.org>
small refactor to get the launch file location
Signed-off-by: William Woodall <william@osrfoundation.org>
improve ability to detect declared arguments across launch file includes
Signed-off-by: William Woodall <william@osrfoundation.org>
add tests for the new DeclareLaunchArgument action
Signed-off-by: William Woodall <william@osrfoundation.org>
test new features of IncludeLaunchDescription
Signed-off-by: William Woodall <william@osrfoundation.org>
test new features of LaunchDescription class
Signed-off-by: William Woodall <william@osrfoundation.org>
remove unused imports
Signed-off-by: William Woodall <william@osrfoundation.org>
improve output when showing arguments of a launch file
Signed-off-by: William Woodall <william@osrfoundation.org>
fix the return type of LaunchService.run()
Signed-off-by: William Woodall <william@osrfoundation.org>
fix the checking for the asyncio event loop for the case where it is …
…not set

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

@wjwwood wjwwood force-pushed the feature/launch_arguments branch from e01ed64 to 43cb119 Aug 23, 2018

@wjwwood wjwwood added in review and removed in progress labels Aug 23, 2018

@wjwwood

This comment has been minimized.

Copy link
Member

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
@mjcarroll
Copy link
Contributor

mjcarroll left a comment

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 ...``.

This comment has been minimized.

@mjcarroll

mjcarroll Aug 30, 2018

Contributor

launched

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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.

wjwwood added some commits Aug 30, 2018

restart event loop to allow proper shutdown when there's an unhandled…
… exception

Signed-off-by: William Woodall <william@osrfoundation.org>
ExecuteProcess: unregister event handlers if rest of setup fails
The event handlers need to be setup before the other lines, but are invalid if setup does not complete successfully.
@wjwwood

This comment has been minimized.

Copy link
Member

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 label Sep 4, 2018

@wjwwood wjwwood deleted the feature/launch_arguments branch Sep 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment