asrob-uc3m / robotDevastation

A new-generation shooter with augmented reality and real robots. You can play online with other users with your PC, moving robots in championships and campaigns: all 24/7!
http://asrob-uc3m.github.io/workgroups/2017-05-28-robot-devastation.html
GNU Lesser General Public License v2.1
9 stars 4 forks source link

Synchronization of color debug macros in AppVeyor logs #103

Closed PeterBowman closed 7 years ago

PeterBowman commented 7 years ago

There is a noticeable delay in message logs that are redirected to stderr, that is, errors and warnings. It can be easily spotted while running tests on AppVeyor. Apart from that, there is a mismatch between stdout/stderr on RD_SUCCESS and RD_DEBUG.

David-Estevez commented 7 years ago

I agree, RD_SUCCESS and RD_DEBUG should output stuff to stdout instead of stderr.

Redirecting errors to stderr seems correct to me, it is the expected behavior for most programs. We could argue about warnings, since they shouldn't be fatal, but they can be considered a different kind of error (so ok with stderr for them).

PeterBowman commented 7 years ago

Done at d75fdce. Note that lines following RD_SUCCESS and RD_DEBUG output appeared kind of contaminated by their presence as a consequence of c0d09fe, e.g. YARP connection notices printed right next to a debug line were coloured blue. This effect could not be observed on Travis CI logs. Fixed at 5cf9629 along with the AppVeyor synchronization issue.

jgvictores commented 7 years ago

@PeterBowman AFAIK, fflush is not required as long as there is a \n. In my code, I usually put an fflush explicitly only when I do not want to put an `\n' (example).

  1. Do you agree with me that explicit fflush may be overkill in most cases (considering prints to screen and possibly flushing are slow operations)?
  2. If so, do you think we could come up with some mechanism to actually check through the code and detect situations where an \n would be good but someone simply forgot?

PS: AFAIK, macros like ROS_INFO always append a \n. I avoided this because of the example above, and also because I wanted to keep syntax as close to printf as possible.

PeterBowman commented 7 years ago

@jgvictores agreed (although aren't we hitting POITROAE?). However, I think it makes no difference whether we include a newline character according to the current implementation:

printf(GREEN); // (1)
printf("[success] %s:%d %s(): ", __REL_FILE__, __LINE__, __func__); // (2)
printf(__VA_ARGS__); // (3)
printf(RESET); // (4)

What's the point of ensuring the presence of \n at (3) if the actual block isn't closed until (4)? In fact, I stumbled on such problem after c0d09fe precisely because of the closing format macro not being in sync with the log output.

There are other mechanisms we could explore, e.g. setbuf. See also http://stackoverflow.com/q/33775622.

jgvictores commented 7 years ago

What's the point of ensuring the presence of \n at (3) if the actual block isn't closed until (4)?

Correct, I hadn't noticed that the final RESET will never be flushed even in the presence of a \n! Given this is the case, I'm happy with 5cf96296fe13a96e118fc65db6a8af880889bc01. Proceeding to close issue, please reopen again if any second-thoughts arise. 😄