add way to include other Python launch files #122
Conversation
wjwwood
force-pushed
the
feature/include_python_launch_files
branch
from
54aea08 to
fed6e3f
Compare
Signed-off-by: William Woodall <[email protected]>
Signed-off-by: William Woodall <[email protected]>
Signed-off-by: William Woodall <[email protected]> To do this, moved functions for loading Python launch files into launch. Signed-off-by: William Woodall <[email protected]>
Signed-off-by: William Woodall <[email protected]>
Signed-off-by: William Woodall <[email protected]>
Signed-off-by: William Woodall <[email protected]>
Signed-off-by: William Woodall <[email protected]>
Signed-off-by: William Woodall <[email protected]>
Signed-off-by: William Woodall <[email protected]>
wjwwood
force-pushed
the
feature/include_python_launch_files
branch
from
fed6e3f to
a0a4b53
Compare
wjwwood
added
in review
| except Exception as exc: | ||
| msg = 'Caught exception in launch (see debug for traceback): {}'.format(exc) | ||
| _logger.error(msg) | ||
| _logger.debug(traceback.format_exc()) |
There was a problem hiding this comment.
Is debug saved to disk by default? If not, it might be hard to debug launch issues that happen infrequently in a production system. Why not error or warn?
There was a problem hiding this comment.
Oops, I missed the error call right above this line. Nevermind.
There was a problem hiding this comment.
Right now debug doesn't go to a file, but perhaps it will in the future (perhaps related #104).
Do you think it's good enough to have the traceback in debug until that happens?
There was a problem hiding this comment.
Do you think it's good enough to have the traceback in debug until that happens?
I do.
| """Launch the example.launch.py launch file.""" | ||
| return LaunchDescription([ | ||
| LogInfo(msg=[ | ||
| 'Including launch file located at: ', ThisLaunchFileDir(), '/example.launch.py' |
There was a problem hiding this comment.
When is it better to use ThisLaunchFileDir() versus
os.path.abspath(os.path.dirname(__file__))?
There was a problem hiding this comment.
You should always, because when introspecting it will be the difference /abs/path/to/example.launch.py and ThisLaunchFileDir('/abs/path/to') + '/example.launch.py'. Which seems trivial, but it indicates that if you moved the launch file it would work, as opposed to it being a hard coded path.
| lc = LaunchContext() | ||
| with pytest.raises(SubstitutionFailure): | ||
| tlfp.perform(lc) | ||
| lc.extend_locals({'current_launch_file_directory': 'foo'}) |
There was a problem hiding this comment.
I half expected the current LaunchDescription and LaunchDescriptionSource to be in the context already. Sort of like inspect.currentframe().f_code.co_filename. I don't have a use case for doing that, and I'm not sure how much work it would be.
There was a problem hiding this comment.
They're not part of the context, though that might be a good thing to do at some point. This line, however, is putting the file location of the current launch file (which is based on the location member of the current LaunchDescriptionSource).
| @@ -14,6 +14,10 @@ | |||
|
|
|||
| """Tests for the IncludeLaunchDescription action class.""" | |||
|
|
|||
| import os | |||
|
|
|||
| from launch import Action | |||
There was a problem hiding this comment.
I get a flake8 warning here about an unused import
Signed-off-by: William Woodall <[email protected]>
|
CI after fixing linter issue:
|
Fixes #116
This pull request does a few things in order to make this feature possible:
LaunchDescriptionSourceto take aLaunchContextwhen sourcing the launch description from it, so that Substitutions can be expandedPythonLaunchDescriptionSourcewhich takes a path to a Python launch file (can be list of substitutions) and loads it to produce aLaunchDescriptionIncludeLaunchDescriptionros2launchthat includes another launch description next to itselfI got side tracked by debugging the issue described in #89 (seems to also be triggered when there's an exception within
launchitself), but this pr is still missing tests for the new classes/functions, which I will push soon.