Hi Eddie, > Here's a patch for what I've worked on. Just to reiterate this patch > does the following to distccmon-text: > > 1) Add a timestamp to every line from the distccmon-text output > > 2) Only print the newline after a series of distcc calls have been > displayed > > And here's the patch. Any chance this could make it into the official > tree? (I tried submitting using tla, but apparently I don't have > access. :) )
Both improvements sound very good to me, even if I am not a regular user of distccmon-text. I however have several objections to your implementation, and some more general comments about patch submission. > --- orig/src/mon-text.c > +++ mod/src/mon-text.c > @@ -36,7 +36,7 @@ > #include "exitcode.h" > #include "snprintf.h" > #include "mon.h" > - > +#include "time.h" I can't find any file named time.h in the distcc project. Don't you really mean <time.h>? > @@ -85,25 +85,38 @@ > * other program, so make sure we're always line buffered. */ > setvbuf (stdout, NULL, _IOLBF, BUFSIZ); > > + char currentTimeAsString[9]; Older C compilers will not like variables being declared in the middle of a block. Also, please change the name of that variable. The distcc project names variables this_way, not thatWay. Same applies to all variables declared in your patch. > -#if 1 > + > + havePrinted = 1; > + > if (i->curr_phase == DCC_PHASE_DONE) > continue; > -#endif I wouldn't drop #if/#endif, even if I don't know why it is there. I guess Martin has good reasons. > + strftime(currentTimeAsString, 9, "%H:%M:%S", > localtime(¤tTime)); Note that your mailer breaks long lines. I had to edit your patch manually before I could apply it. Either configure your client differently, or submit your patches as attachements instead of inline. > /* Assume 80 cols = */ > - printf("%6ld %-10.10s %-30.30s %24.24s[%d]\n", > + printf("[%8s] %6ld %-10.10s %-30.30s %24.24s[%d]\n", > + currentTimeAsString, > (long) i->cpid, > dcc_get_phase_name(i->curr_phase), > i->file, i->host, i->slot); This doesn't fit in 80 columns anymore, while this was very convenient. Also notice that you are computing the time string for each file/host line, while it will always be (almost) the same time. I would suggest that you compute the time string *outside* of the for loop, and print it only *once*, before the loop. This would result in something like: [12:59:26] 7181 Compile mon-text.c arrakis[0] 7177 Compile mon.c localhost[0] 7157 Compile popt.c mahadeva[0] instead of: [12:59:26] 7181 Compile mon-text.c arrakis[0] [12:59:26] 7177 Compile mon.c localhost[0] [12:59:26] 7157 Compile popt.c mahadeva[0] This has the following advantages: 1* Still fits on 80 columns. 2* Less computation. If you insist to have a the time on each line, please at least change the widths so that the lines will fit on 80 columns again. > - printf("\n"); > + if(havePrinted) > + printf("\n"); Same can be done without the additional variable, see attached patch. And a more general comment: I'd suggest that you do not include several improvements in a single patch. One change per patch makes reviewing and merging way easier. Care to redo a patch implementing (1) only (since mine implements (2) more efficiently, or so I think), and addressing the points I listed above? Martin, like my patch? Thanks, -- Jean Delvare
--- distcc-2.18.3/src/mon-text.c.orig 2004-10-24 07:05:49.000000000 +0200 +++ distcc-2.18.3/src/mon-text.c 2005-02-20 13:04:19.000000000 +0100 @@ -103,7 +103,9 @@ i->file, i->host, i->slot); } - printf("\n"); + /* Scroll only if something is actually happening */ + if (i != list) + printf("\n"); /* XXX: usleep() is probably not very portable */ usleep(delay * 1000000);
__ distcc mailing list http://distcc.samba.org/ To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/distcc