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

[Frank Heckenbach]

I've tested the current output-sync implementation in my use case,
using "--output-sync=line --trace=dir".

I definitely need "--trace=dir" since I want to refer my compiler
errors automatically to their directories. Without this option, this
often goes wrong in parallel builds. (Which just means, I don't care
about the default for "--output-sync", since I'll need "--trace=dir"
anyway which won't be the default I understand.)

But unfortunately, even with this option, the enter/leave messages
are not quite correct as the attached example shows (test.tar.bz2,
though I admit I had to carefully construct it to exhibit the bug):

% make -j -Oline --trace=dir
make -Cd1
make -Cd2
make[1]: Entering directory 'd1'
Makefile:5: d1-1
make[1]: Entering directory 'd2'
sleep 1
make[1]: Leaving directory 'd2'
make[1]: Entering directory 'd2'
Makefile:5: d2-1
make[1]: Entering directory 'd1'
sleep 2
make[1]: Leaving directory 'd1'
Makefile:2: d1-2
make[1]: Leaving directory 'd1'
make[1]: Entering directory 'd2'
sleep 2
make[1]: Leaving directory 'd2'
Makefile:2: d2-2
make[1]: Leaving directory 'd2'

If you count the messages you'll see that the "d1-2" message appears
while 'd1' and then 'd2' is "active", so a reader (automatic or
manual) of the messages would think we're in d2, but we're actually
in d1.

Also, the "Leaving directory 'd1'" message right afterwards doesn't
pair up correctly (LIFO). The latter problem also exists, and is
more clearly visible, without "--trace=dir":

% make -j -Oline
make -Cd1
make -Cd2
make[1]: Entering directory 'd1'
Makefile:5: d1-1
make[1]: Entering directory 'd2'
sleep 1
Makefile:5: d2-1
sleep 2
Makefile:2: d1-2
make[1]: Leaving directory 'd1'
sleep 2
Makefile:2: d2-2
make[1]: Leaving directory 'd2'

That's why I had added the "spurious" enter/leave messages in
message(), error() and fatal() which you removed again. If I add
them back (misc.c.diff), these problems disappear. I understand you
do not like them by default, but perhaps you could add them
dependent on "--trace=dir" (or "--trace=dir-verbose" if you want it
even more explicit).

Some minor things (job.c.diff):

- assign_child_tempfiles(): If c->outfd is closed on error, it
  should probably reset the field to -1. I haven't traced whether
  this can cause an actual bug, but I think it's safer to not keep
  an invalid fd.

- pump_from_tmp(): Change "to_fd" to "to" in 2 comments.

- start_job_command(): "output_sync && sync_cmd" is redundant (but
  doesn't hurt) since sync_cmd is "output_sync && ..." anyway.

--- job.c.orig  2013-05-26 00:44:38.000000000 +0200
+++ job.c       2013-05-26 01:36:45.000000000 +0200
@@ -687,7 +687,10 @@
 
  error:
   if (c->outfd >= 0)
-    close (c->outfd);
+    {
+      close (c->outfd);
+      c->outfd = -1;
+    }
   output_sync = 0;
 }
 
@@ -701,7 +704,7 @@
   int prev_mode;
 
   /* from_fd is opened by open_tmpfd, which does it in binary mode, so
-     we need the mode of to_fd to match that.  */
+     we need the mode of "to" to match that.  */
   prev_mode = _setmode (fileno (to), _O_BINARY);
 #endif
 
@@ -721,7 +724,7 @@
     }
 
 #ifdef WINDOWS32
-  /* Switch to_fd back to its original mode, so that log messages by
+  /* Switch "to" back to its original mode, so that log messages by
      Make have the same EOL format as without --output-sync.  */
   _setmode (fileno (to), prev_mode);
 #endif
@@ -1753,7 +1756,7 @@
 #ifdef OUTPUT_SYNC
           /* Divert child output if output_sync in use.  Don't capture
              recursive make output unless we are synchronizing "make" mode.  */
-          if (output_sync && sync_cmd)
+          if (sync_cmd)
             {
               int outfd = fileno (stdout);
               int errfd = fileno (stderr);
@@ -1866,7 +1869,7 @@
 #ifdef OUTPUT_SYNC
           /* Divert child output if output_sync in use.  Don't capture
              recursive make output unless we are synchronizing "make" mode.  */
-          if (output_sync && sync_cmd)
+          if (sync_cmd)
             hPID = process_easy (argv, child->environment,
                                  child->outfd, child->errfd);
           else

--- misc.c.orig 2013-05-26 00:44:38.000000000 +0200
+++ misc.c      2013-05-26 04:09:23.000000000 +0200
@@ -266,6 +266,9 @@
 
   if (fmt != 0)
     {
+      if (output_sync)
+        log_working_directory (1, 1);
+
       if (prefix)
         {
           if (makelevel == 0)
@@ -277,6 +280,9 @@
       vfprintf (stdout, fmt, args);
       va_end (args);
       putchar ('\n');
+
+      if (output_sync)
+        log_working_directory (0, 1);
     }
 
   fflush (stdout);
@@ -289,7 +295,10 @@
 {
   va_list args;
 
-  log_working_directory (1, 0);
+  if (output_sync)
+    log_working_directory (1, 1);
+  else
+    log_working_directory (1, 0);
 
   if (flocp && flocp->filenm)
     fprintf (stderr, "%s:%lu: ", flocp->filenm, flocp->lineno);
@@ -304,6 +313,9 @@
 
   putc ('\n', stderr);
   fflush (stderr);
+
+  if (output_sync)
+    log_working_directory (0, 1);
 }
 
 /* Print an error message and exit.  */
@@ -313,7 +325,10 @@
 {
   va_list args;
 
-  log_working_directory (1, 0);
+  if (output_sync)
+    log_working_directory (1, 1);
+  else
+    log_working_directory (1, 0);
 
   if (flocp && flocp->filenm)
     fprintf (stderr, "%s:%lu: *** ", flocp->filenm, flocp->lineno);
@@ -328,7 +343,8 @@
 
   fputs (_(".  Stop.\n"), stderr);
 
-  log_working_directory (0, 1);
+  if (output_sync)
+    log_working_directory (0, 1);
 
   die (2);
 }

test.tar.bz2
Description: Binary data