Remove failure check of RCUTILS_CONSOLE_STDOUT_LINE_BUFFERED

[brawner]

While looking for places to increase testing coverage, I noticed that logic in logging.c required the rcutils_get_env call for RCUTILS_CONSOLE_STDOUT_LINE_BUFFERED to at least succeed. As currently implemented, it will always succeed and return NULL thus the else statement is dead code. But even if it could result in an error, the else clause doesn't seem useful since the environment variable use has been deprecated.

Importantly for foxy considerations, this will not change the API of the function, since the environment variable was unused and the rcutils_get_env call will always succeed.

colcon build --packages-up-to rcutils && colcon test --packages-select rcutils

• Linux
• Linux-aarch64
• macOS
• Windows

Signed-off-by: Stephen Brawner [email protected]

[clalancette]

While looking for places to increase testing coverage, I noticed that logic in logging.c required the rcutils_get_env call for RCUTILS_CONSOLE_STDOUT_LINE_BUFFERED to at least succeed. As currently implemented, it will always succeed and return NULL thus the else statement is dead code. But even if it could result in an error, the else clause doesn't seem useful since the environment variable use has been deprecated.

While I understand that this is hard to test from a testing point-of-view, the conditions it is checking for are different. One is the unexpected case where rcutils_get_env returned an error; if that occurs, then something very bad is happening and most likely other things are going to fail later too. In that case, it seems better to get out early and fail the program.

The other case is the one in where rcutils_get_env succeeded, but the variable was non-empty, so we want to print a warning message. This is more-or-less expected as a backwards-compatibility thing, so we should just print the warning and continue.

With that being said, I don't think this implementation is right. I think the second clause that was already there is actually providing a bit of value.

[brawner]

In the case where rcutils_get_env returns an error with the inputs that logging_initialize passes it, then it would be a programming error and not bad input or system error. What do you think of using an assert to check ret is null?

i.e.

assert(ret_str == NULL);
if (strcmp(line_buffered, "") != 0) {
      fprintf(...);
}

[clalancette]

In the case where rcutils_get_env returns an error with the inputs that logging_initialize passes it, then it would be a programming error and not bad input or system error. What do you think of using an assert to check ret is null?

Oh, I see. Yeah, for right now, you are correct. But we are looking at changing the signature of rcutils_get_env so it is more threadsafe: #242 . When we do that, rcutils_get_env will be able to return errors related to system problems (in particular, it will allocate memory, which may fail).

While I don't usually like to hold code up on something that may happen in the future, in this case it will be pretty easy to forget that we baked this knowledge in here. So in the name of defensive programming, I'd still suggest that we keep the distinction between something going wrong in the system and the thing being empty.

[brawner]

I just wanted to float this PR as an idea. I realize that a lot of repetitive checks are happening in the code base and I don't have a problem with that. This just seemed like a situation where if the API of rcutils_get_env changed and started throwing errors, then this part of the code base shouldn't really care. Either way, we get to 95% coverage without removing these lines in #258 so I'm not too concerned about this chunk.