Re: [bug #33138] .PARLLELSYNC enhancement with patch

[Paul Smith]

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 :-)