[bug #33138] .PARLLELSYNC enhancement with patch

[Frank Heckenbach]

Follow-up Comment #3, bug #33138 (project make):

The current patch doesn't work well with recursive makes. The test
case (sync-recursive-demo.tar.gz) demonstrates this. Its output with
"make -j" is the following:

make -C foo
make -C bar
[2s delay]
make[1]: Entering directory 'foo'
foo: start
foo: end
make[1]: Leaving directory 'foo'
[2s delay]
make[1]: Entering directory 'bar'
bar: start
baz: start
bar: end
baz: end
make[1]: Leaving directory 'bar'

This shows two potential problems:

a) The output of bar and baz is still mixed up. This is clearly a
   bug, as far as the purpose of the patch is concerned.

b) The output of bar is not shown until baz is also finished which
   is just an inconvenience, but may be a significant one if the
   recursive jobs take long (think of several directories with large
   builds coordinated by a small top-level Makefile). While some
   amount of output delay is inevitable when syncing, as discussed
   in the original thread, in this case it might be preferable to
   sync the output of the individual recipes of the recursive makes.

The problems occur because the patch doesn't handle recursive jobs
specially, so they will be synced like any other command, as a
whole. Fortunately, I think the issues are rather easy to solve:

1. Do not activate parallel_sync for the recursive jobs themselves.
   With an external solution using SHELL, it might be tricky to
   detect recursive jobs, but with this internal solution, the
   information is readily available by checking
   "!(flags & COMMANDS_RECURSE)" before the assign_child_tempfiles()
   call.

   If the call is not made, we must also set "outfd = errfd = -1" in
   the child. This doesn't hurt anyway and I'd prefer to do it in
   new_job(), right after the allocation of a new child for clarity
   (as is it now, these fields are left unintialized in this case,
   so their validity depends on a global flag which can be confusing
   and error-prone).

   The other places where parallel_sync is checked do not need any
   changes because they do nothing if outfd/errfd are < 0.

2. Pass the parallel sync option to the sub makes. If it is turned
   into a command-line option, as I suggested in my previous
   comment, that can happen automatically via MAKEFLAGS. This
   involves removing the change in read.c and inserting in an entry
   in struct command_switch switches[].

Since someone might prefer the current behaviour of b), I actually
made parallel_sync a tri-state (none, PARALLEL_SYNC_FINE,
PARALLEL_SYNC_COARSE, which also seems easier to do with a
command-line option than with special targets) and modified the
check in 1. accordingly. Note that in the PARALLEL_SYNC_COARSE case,
the various recipes in each recursive jobs will sync against each
other on their stdout/stderr which is in fact the temp file created
by the higher-level parallel sync. So it still fixes the bug of a).

As a detail, the statement
"parallel_sync = assign_child_tempfiles(...);"
won't work this way with a tri-state. I've modified it to preserve
the value if assign_child_tempfiles() returns non-zero.

With those changes I now get this output with PARALLEL_SYNC_FINE
(which I activate with option "-P"):

make -C foo
make -C bar
make[1]: Entering directory 'foo'
make[1]: Entering directory 'bar'
[1s delay]
bar: start
bar: end
[1s delay]
foo: start
foo: end
make[1]: Leaving directory 'foo'
[2s delay]
baz: start
baz: end
make[1]: Leaving directory 'bar'

With PARALLEL_SYNC_COARSE ("-P2") I get:

make -C foo
make -C bar
[2s delay]
make[1]: Entering directory 'foo'
foo: start
foo: end
make[1]: Leaving directory 'foo'
[2s delay]
make[1]: Entering directory 'bar'
bar: start
bar: end
baz: start
baz: end
make[1]: Leaving directory 'bar'

This shows that a) is fixed, and b) is for PARALLEL_SYNC_FINE as it
should.

However, it also shows another problem:

c) In the "PARALLEL_SYNC_FINE" case, the "Entering/Leaving
   directory" messages do not properly relate to the messages. When
   one wants to use them to interpret the messages (such as with the
   "directory change tracking" changes to Emacs's compilation
   commands), this is misleading.

To fix that, I surround each recipe's output with enter/leave
messages in sync_output(). Notes:

- I had to add a 2nd parameter "force" to log_working_directory(),
  otherwise the new messages wouldn't appear since make thinks it
  has already shown them (which it has, but perhaps not recently
  enough). Since I didn't want to mess with the other message
  reporting places, this seems the least intrusive way. Of course,
  this can lead to redundant messages (as seen in the test output
  below), but since those messages in this case are mostly meant to
  be read by machines (e.g. Emacs), it shouldn't hurt too much. It's
  more important that the messages are correct.

- I check if the temporary files are empty before copying them,
  surrounded by the messages. Since usually (at least for me) most
  individual recipes produce no output at all, this avoids many of
  those redundant messages. Also (independent from the new
  messages), it avoids acquiring the semaphore if there is nothing
  to write so it might (slighty) increase throughput. (To do this
  properly, I had to move the close() call from pump_from_tmp_fd()
  to sync_output().)

- The change shows an asymmetry between stdout and stderr. The
  messages only apply to the former since log_working_directory()
  writes them to stdout. But if stdout and stderr are merged (the
  usual case), this is alright, since the copying of stdout handles
  the merged temp file. If they are not merged, there's nothing we
  can do easily, since stderr does not get enter/leave messages
  anyway.

I now get with "-P":

make -C foo
make -C bar
make[1]: Entering directory 'foo'
make[1]: Entering directory 'bar'
[1s delay]
make[1]: Entering directory 'bar'
bar: start
bar: end
make[1]: Leaving directory 'bar'
[1s delay]
make[1]: Entering directory 'foo'
foo: start
foo: end
make[1]: Leaving directory 'foo'
make[1]: Leaving directory 'foo'
[2s delay]
make[1]: Entering directory 'bar'
baz: start
baz: end
make[1]: Leaving directory 'bar'
make[1]: Leaving directory 'bar'

and with "-P2":

make -C foo
make -C bar
[2s delay]
make[1]: Entering directory 'foo'
make[1]: Entering directory 'foo'
foo: start
foo: end
make[1]: Leaving directory 'foo'
make[1]: Leaving directory 'foo'
[2s delay]
make[1]: Entering directory 'bar'
make[1]: Entering directory 'bar'
bar: start
bar: end
make[1]: Leaving directory 'bar'
make[1]: Entering directory 'bar'
baz: start
baz: end
make[1]: Leaving directory 'bar'
make[1]: Leaving directory 'bar'

This looks good to me, so I hope the patch is now ready for
inclusion if no new issues are found.

I've added a modified patch (make-sync-recursive.patch) including
the above-mentioned changes and corresponding documentation changes.
It also fixes the two bugs mentioned in my previous comment, though
it does not change anything WRT the first two points mentioned there
which are not bugs, rather matters of preference.


(file #27203, file #27204)
    _______________________________________________________

Additional Item Attachment:

File name: sync-recursive-demo.tar.gz     Size:0 KB
File name: make-sync-recursive.patch      Size:14 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?33138>

_______________________________________________
  Nachricht gesendet von/durch Savannah
  http://savannah.gnu.org/