ros_comm icon indicating copy to clipboard operation
ros_comm copied to clipboard

Return the exit code a required process through roslaunch

Open lucasw opened this issue 3 years ago • 5 comments

Return the exit code to the caller of a required process at least for a regular parent launch process, child and remote processes need looking at.

May solve https://github.com/ros/ros_comm/issues/919

lucasw avatar Oct 19 '20 17:10 lucasw

Made some cleanup and existing tests pass.

A new test that launches a node with required=true then forces it to exit non-zero, then inspects roslaunch exit code is the next step. Any pointers to an existing roslaunch test that would be a good template for this would be very welcome, otherwise I'll figure it out eventually.

I'll hold on to the remaining extra prints until I'm done with that, a few I think it makes sense to keep.

lucasw avatar Nov 06 '20 18:11 lucasw

Just tested this patch ported to melodic. Works for me!

I did notice that the roslaunch xml node has to be marked as required="true", which is logical. Just wanted to point that out, as it threw me for a loop for a second.

1612214530.892673: Code shutdown requested: 37
================================================================================REQUIRED process [host/exit_code_node-1] has died!
process has died [pid 24194, exit code 37, cmd /root/src/core/nodes/exit_code_node.py __name:=exit_code_node __log:=/root/.ros/log/906b1440-649f-11eb-8a83-78d00426c726/-exit_code_node-1.log].
log file: /root/.ros/log/906b1440-649f-11eb-8a83-78d00426c726/-exit_code_node-1*.log
Initiating shutdown!
================================================================================
Going to exit due to code 37
[host/exit_code_node-1] killing on exit
shutting down processing monitor...
... shutting down processing monitor complete
roslaunch runner done
ending server mode 37
exiting from main() 37
root@remote:~# echo $?
37

xkortex avatar Feb 01 '21 21:02 xkortex

would love to see this in melodic. Is there something I can do to help get this merged?

LukeAI avatar Feb 23 '21 15:02 LukeAI

Added a unit test, can run manually with

catkin run_tests
rostest test_roslaunch roslaunch.test

lucasw avatar Mar 21 '21 17:03 lucasw

Can the requires-changes label be removed if there are no more changes needed?

lucasw avatar Sep 26 '21 03:09 lucasw