Disable rosout.log by using environment variable

[yli-cpr]

rosout is usually started together with roscore. There may be race condition so that it cannot connect to the master during initialization. This change lets rosout wait at most 1 second for the master to be online, so that it can read the ROS parameter.

[mikepurvis]

Rather than blocking startup of the node, can we do something like check on each message received until either we get an answer or 10s have elapsed, and then stop checking?

[yli-cpr]

That may leave a rosout.log file on the disk. When the first message arrives the master must have been online, I guess? What about opening the file upon the first received message?

Another idea is to make it runtime parameter, so we check the paramter every time when we are going to log a message.

What do you think?

I don't like environment variable solution because, when roslaunch spawns rosout process, it seems not to use the environment variables defined in the launch file.

[yli-cpr]

Using environment variable will be a clean change though

[yli-cpr]

The test failure seems unrelated? 12:06:45 Caused by: com.fasterxml.jackson.core.JsonParseException: Numeric value (5048810355) out of range of int

[mikepurvis]

This is new to me. Other than the allocation, is there any advantage to this over just instantiating a throwaway std::string and comparing that directly?

[yli-cpr]

std::string cannot be constructed with nullptr. So anyway, checking for null is required. I first tried std::string and got an exception complaining about nullptr

[mikepurvis]

gotcha, makes sense.

[dirk-thomas]

The case insensitive comparison to true seems to be rather restrictive. I would suggest the opposite and if the string is empty or 0 (maybe "false", "off"?) consider the value to be false, otherwise true.

[mikepurvis]

LGTM, but you'll need to add the boost dependency to this package's package.xml:

https://github.com/ros/ros_comm/blob/melodic-devel/tools/rosout/package.xml#L15-L19

IMO a good opportunity to consolidate the build/run deps and switch this one to format="2" as well.

The wiki page describing the parameter will need to be updated to describe the envvar, once this is merged: http://wiki.ros.org/rosout#rosout.log

Will wait on @dirk-thomas for any final thoughts.

[yli-cpr]

Thanks. Updated the package.xml. Also submitted a change to the wiki page.

[yli-cpr]

The wiki page has been updated. Ping @dirk-thomas
@mikepurvis we will need this in lunar-edge branch

[jliviero]

You may need to add Boost to CMake as well:

• http://wiki.ros.org/catkin/CMakeLists.txt#Boost
• https://github.com/ros/ros_comm/search?l=CMake&q=boost

You could also avoid needing to pull in the Boost dependency altogether by doing something like the following (and let's avoid the nasty true/True discrepancy we've seen before at the same time):

const char* disable_logging_env = getenv("ROSOUT_DISABLE_FILE_LOGGING"); 
std::string disable_file_logging = disable_logging_env ? disable_logging_env : "false";
std::transform(disable_file_logging.begin(), disable_file_logging.end(), disable_file_logging.begin(), ::tolower);

if (disable_file_logging == "true")
{
   // ...etc
}

[yli-cpr]

Thanks for the suggestions.

find_package(Boost REQUIRED COMPONENTS thread)

This is not needed for boost algorithm because that lib is headers only. I remember it gave an error when I tried that with algorithm.

I could use find_package(Boost) and add the boost include directories. However, it is not mentioned in the catkin wiki page, and the fact it builds probably means the boost include directories is already included in catkin_INCLUDE_DIRS.

Opinions?

[yli-cpr]

Added Boost_INCLUDE_DIRS explicitly

[yli-cpr]

The test failure is not related.

[dirk-thomas]

Just as a note: this patch revert the logic added in #1381.

It also looks like that the documentation has already been updated to reflect the proposed change: http://wiki.ros.org/action/info/rosout?action=diff&rev2=53&rev1=52 Please do not update the wiki with not yet merged changes since that feature is not yet usable by users.

[yli-cpr]

Yes, lesson learned. Will do the change.

[dirk-thomas]

I am just wondering if the case insensitive comparison if worth the boost dependency? Each condition could be duplicates to check for lower case as well as upper case (not allowing mixed case)...

[jliviero]

You don't need boost for case-insensitivity. Just convert the input value to lower case before comparing:

#1425 (comment)

[dirk-thomas]

Thank you for the patch and for iterating on it.

[yli-cpr]

Thanks for the cleanup!