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/ _______________________________________________ Bug-make mailing list Bug-make@gnu.org https://lists.gnu.org/mailman/listinfo/bug-make