[roslaunch] Added condition to skip empty ('') <include> paths#882
[roslaunch] Added condition to skip empty ('') <include> paths#882jliviero wants to merge 3 commits intoros:kinetic-develfrom
Conversation
|
Can you explain your use case a little bit more? And please consider adding test cases to make sure it behaves in the expected ways. From your example it looks like the modified code path will never be hit. |
|
Certainly. This covers a case where we want to configure a set of parameters in a Gazebo simulation at launch time. In our specific case, this constitutes a series of additions to the environment (which is why I chose "some_mesh" - the actual arg is arbitrary). However, we don't need or want these changes on every launch, and we want to be able to quickly swap between configurations of the additions (pose, etc.) to test different solutions. edited to add: Our internal build system runs roslaunch-check against the relevant package, which will fail when it encounters
|
|
I just modified an existing launch file and added an optional Can you please clarify your use case and describe why you need the modification in this PR to run the launch file with and without. |
Ah, I see where I haven't been clear, thank you. This allows rolaunch-check to pass; it does not affect the behaviour of roslaunch. Without this change, a file with the modifications you've made based on my example will fail: With my change, the output of roslaunch is: While the failing test could probably be ignored, it is causing our internal build tests to fail, and this seems like a reasonable enough use-case to submit a fix. Editing my above posts accordingly. |
| if sub_pkg is None: | ||
| print("ERROR: cannot determine package for [%s]"%sub_launch_file, file=sys.stderr) | ||
|
|
||
| elif sub_launch_file not in file_deps[launch_file].includes: |
There was a problem hiding this comment.
Why should this be changed from if to elif?
There was a problem hiding this comment.
It shouldn't; this was an error, thanks for catching. Will correct.
|
To avoid the huge diff can you please avoid the change of indentation and instead use |
Done. Still some whitespace/line length fixes in the block, but it's otherwise unchanged. |
|
Any unnecessary whitespace changes just make it more difficult to backport changes between different branches. Therefore I have cherry-picked your patch (without the whitespace changes) to the kinetic-devel branch: b323b8d Thank you for the PR. |
|
Fair enough re: whitespace. Thanks. |
Our internal build system runs roslaunch-check against relevant packages, which will fail when it encounters
<include file='' />. This change allows roslaunch-check to pass, but prints a warning when such an include is encountered.This allows a file path to be passed in as an arg, and left empty by default, and still have roslaunch-check pass (but with a warning). For example: