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

roslaunch option to log to both: screen and logfile #413

Closed
wants to merge 3 commits into from

Conversation

eggerdo
Copy link

@eggerdo eggerdo commented May 16, 2014

Fix for issue #409

I added a new ouput value both which sends stdout and stderr to both screen and file. it uses subprocess.pipe and threading.

Also includes the following fix:

on http://wiki.ros.org/roslaunch/XML/node#Attributes it says that:

If 'log', the stdout/stderr output will be sent to a log file in $ROS_HOME/log, and stderr will continue to be sent to screen.

but stderr was only sent to screen, not to logfile. now stderr will be sent to both screen and log, while stdout only goes to log.

@dirk-thomas
Copy link
Member

First any PR should be against the latest development branch - currently indigo-devel. After they have been merged there they may be considered for backporting.

I will add a number of comments to your commits inline.

if not self.output in ['log', 'screen', None]:
raise ValueError("output must be one of 'log', 'screen'")
if not self.output in ['log', 'screen', 'both', None]:
raise ValueError("output must be one of 'log', 'screen' or 'both'")
Copy link
Member

Choose a reason for hiding this comment

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

Line 437 (docblock) needs to be updated:

:param output: where to log output to, 'screen', 'log' or 'both', ``str``

@dirk-thomas dirk-thomas changed the title Hydro devel roslaunch option to log to both: screen and logfile May 21, 2014
@eggerdo
Copy link
Author

eggerdo commented May 23, 2014

I adjusted the code based on your changes, so once we agree on the solution for the stdbuf I can issue a new PR. Since you mentioned it should be done on indigo-devel branch I will do it there, but as my system is not set up for indigo but for hydro I can't directly test it. Do you think the changes between indigo and hydro in respect to roslaunch are big or is it enough if I test it on hydro?

@dirk-thomas
Copy link
Member

You should be able to compile the indigo-devel branch just the same way as the hydro-devel branch together with all the other hydro packages. It could be the case that you need to checkout another repos indigo-devel branch but CMake should give you a warning about wrong versions in that case / or missing packages.

eggerdo pushed a commit to eggerdo/ros_comm that referenced this pull request May 27, 2014
…creen and file. it uses subprocess.pipe and threading. Includes corrections from PR ros#413
@eggerdo eggerdo closed this May 27, 2014
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.

2 participants