Hi Frank, thanks for the thorough testing! This feature will be much better for all your efforts.
*sigh* If it weren't for the enter/leave messaging, the output-sync feature would have been quite straightforward! :-/ :-). On Mon, 2013-09-16 at 12:18 +0200, Frank Heckenbach wrote: > 1. > > % cat Makefile > include foo > all: > foo: > touch foo > > % rm -f foo; make -w > make: Entering directory 'foo' > Makefile:1: foo: No such file or directory > touch foo > make: Entering directory 'foo' > make: Nothing to be done for 'all'. > make: Leaving directory 'foo' > > Twice "Entering", once "Leaving". I think there's an output_close() > missing before reexec (which might also lose other messages in other > situations): I didn't fix it this way. Instead I used the existing MAKE_RESTART environment variable to communicate from the current make to the restarted make that the enter message was already printed (if it was) so it wouldn't be printed again. This ensures we don't get a stream of enter/leave statements as we re-exec. This works now (in my repo). > 2. > > % cat Makefile > all: t1 t2 > t1: > $(MAKE) -Cd1 > t2: > $(MAKE) -Cd2 > % cat d1/Makefile > $(info d1) > all: > % cat d2/Makefile > $(info d2) > all: > > % make -j -Oline # or -Otarget > make -Cd1 > make -Cd2 > make[1]: Entering directory 'foo/d1' > d1 > make[1]: Entering directory 'foo/d2' > d2 > make[1]: Nothing to be done for 'all'. > make[1]: Leaving directory 'foo/d1' > make[1]: Nothing to be done for 'all'. > make[1]: Leaving directory 'foo/d2' > > So the $(info) output is now inside the appropriate Enter/Leave > messages, but those are nested improperly because they are not > synced. > > Of course, not syncing them seems right at first glance, when I > request syncing on a target level, and no (non-recursive) targets > are being run, but the improper nesting doesn't look nice (and might > confuse tools that try to parse the messages). > > As I understand it, you addressed this kind of problem for the > -Orecurse case with the "output_sync == OUTPUT_SYNC_RECURSE" > condition in output_start(), but unfortunately not for > -Otarget/-Oline. I'm not sure I agree with that assessment. I spent a bit of time considering how "recurse" should work, and it's very different than line/target. This is because in "recurse" mode we know that our parent make has redirected all our output to a temp file, so it won't get dumped until the entire make is complete, while in the other modes we do not redirect output from recursive command lines so any output generated by them goes directly to stdout. As a result, when it comes to enter/leave the "recurse" mode is actually the same as non-sync mode (or even non-parallel mode): we only need to write enter/leave at the beginning and end of the entire make process, not around every recipe. The individual recipes are still sync'd, of course, but we don't need enter/leave around each one because they all get written to a single temp file and that temp file will have a global enter/leave when it is dumped. I did think about the fact that output generated while the makefile was being parsed would have this problem. I agree the only solution is to have output generated while the makefile is being parsed be synchronized, and enter/leave managed, just like other output, when in line/target sync mode. As you say it shouldn't be hard. I just didn't want to add a new set of enter/leave statements, but maybe there's no good way around it. > To fix it for all cases, I'm afraid the only way I can see is to set > up an extra temp file in this situation and dump it before a child > is run. At least this seems easier to do now with your new > infrastructure. > > As a side benefit, this would simplify the logic in output_start(); > AFAICS, it could just say "if (!output_sync)" then; and > correspondingly remove the "output_sync != OUTPUT_SYNC_RECURSE" > condition in output_dump(), i.e. those messages are always produced > by output_dump() if syncing and by output_start()/output_close() > otherwise, which would incidentally also fix the next problem: I don't think this is true, but I'll have to look into it. > 3. > > % cat Makefile > $(shell echo foo >&2) > > all: > echo bar > > % make -s -w -Onone # or -Orecurse > make: Entering directory 'foo' > foo > bar > make: Leaving directory 'foo' > % make -s -w -Oline # or -Otarget > make: Entering directory 'foo' > foo > make: Entering directory 'foo' > bar > make: Leaving directory 'foo' > make: Leaving directory 'foo' > > So with -Otarget or -Oline, there's double Enter/Leave messages. > (I should note that I personally don't mind this so much, seeing as > I had implemented "excessive" messages before. :) Anyway, I think > that's due to both of these being executed here: > > stdio_traced = log_working_directory (NULL, 1); > > traced = log_working_directory (output_context, 1); > > If this is not fixed together with 2., a solution might be to just > suppress the 2nd one if the 1st one was already done, but I haven't > checked this carefully. Hm. I suppose you mean that this will "bundle" the output from makefile parsing along with the output of the first synchronized output (line or target). I don't think that will work well, since the makefile output is not synchronized the enter message could be a long way from the first target or line output. I think synchronizing the makefile parsing will yield better results. > 4. > > % cat Makefile > all: > $(shell echo foo >&2) > > % make -s -w -Onone # or -Orecurse > make: Entering directory 'foo' > foo > make: Leaving directory 'foo' > % make -s -w -Oline # or -Otarget > foo > > Here, with -Otarget or -Oline there's no Enter/Leave messages if > there's no other output. Not sure if you consider this an actual > problem. The solution seems to be to redirect stderr (only) before > $(shell) executions just like we do for recipe jobs. Hm. This is pretty contrived. I have a hard time imagining a real makefile wanting to do this for a good reason. However, it does seem that the solution may be simple enough. > 5. > > ISTM when output_dump() is called, output_context always ought to > be NULL (the call is either outside of any OUTPUT_SET/OUTPUT_UNSET > section, or (job.c:1274) child->output.syncout is NULL), is that > right? > > If so, the use of output_context might be slightly irritating > (though not wrong) -- at first I wondered where the > log_working_directory() output after the pump_from_tmp() calls was > going to and whether it didn't need pumping too if it was going to > the temp file, but apparently this never happens. > > In fact, if that's so, it seems you could eliminate > the out parameter to log_working_directory() entirely. I'll have to read this again and look more carefully; offhand I'm not convinced of this. But, I did have other things going on here which I eventually decided were not needed and it's possible this complexity is remaining from that. > 6. > > output.c:379: /* We've entered the "critical section" during which a lock is > held. ... */ > > You might want to move this comment a few lines up since the > previous if-statement is already within the critical section. OK. > 7. > > job.c:1258: OUTPUT_UNSET(); > > Just wondering, is this necessary? (It's before OUTPUT_SET().) It is, because if you look above you'll see there's a label next_command: and later in the function we goto that label, perhaps with OUTPUT_SET(). So for example at 1268 we run OUTPUT_SET(), then at 1314 and 1326 we goto next_command. We may return from the goto (at line 1259) before we run OUTPUT_UNSET() at line 1642 or 1648. PS. I know this use of goto is nightmarish: I didn't write this code :-) _______________________________________________ Bug-make mailing list Bug-make@gnu.org https://lists.gnu.org/mailman/listinfo/bug-make
