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

[Frank Heckenbach]

: I've tested your changes and so far "-O job" works for my use cases.

I shouldn't have written that. :-( Shortly afterwards, I found a bug
or perhaps two:

foo:
        @echo foo
        address@hidden bar

(a)
% make -Ojob
foo
bar
foo

(b)
% make -Otarget
bar
foo

As you see, (a) "-Ojob" writes "foo" twice and (b) "-Otarget" writes
the messages in the wrong order. "-Omake" is correct.

I debugged (a) and found that sync_output() is called twice without
an assign_child_tempfiles() in between which would ftruncate() the
temp files. The second one does not call it because
flags & COMMANDS_RECURSE is set. This led me to this ChangeLog
entry:

: Fri Sep 17 00:37:18 1993  Roland McGrath  (address@hidden)
:
:       * job.c (start_job_command): Set COMMANDS_RECURSE in FLAGS if we
:       see a `+' at the beginning of the command line.

Well, I haven't often found an answer I was looking for in a
20 years old ChangeLog entry. :) However, that's kind of bad news
since I'd written my changes under the assumption that
COMMANDS_RECURSE means, well, to recurse.

(a), which is clearly a bug, can be fixed if we make sync_output()
ftruncate() the temp files (the original patch closed them at this
point, and let the next job open new ones). The patch below does
this. I think it also adds some clarity and resilience WRT future
changes, since it makes sure that the contents are purged as soon as
they were output.

In (b), since make thinks the second line is recursive, and the
first one not, it "syncs" the second one first, then outputs the
first one normally. In fact, you can see that "+" lines are not
synced at all with "-Otarget":

test: foo bar

foo:
        address@hidden foo1
        +sleep 1
        address@hidden foo2

bar:
        address@hidden bar1
        +sleep 1
        address@hidden bar2

% make -Otarget -j
foo1
bar1
sleep 1
sleep 1
foo2
bar2

Maybe you consider this correct if "+" lines are indeed considered
recursive and this isn't just an implementation artifact. If so,
alright (it doesn't affect me personally, since I use "-Ojob"). But
if not, this might require slightly bigger changes such as splitting
COMMANDS_RECURSE into two flags. Paul?

PS: I wrote:

: The following patch makes these changes and fixes a precedence bug
: that had crept in during the introduction of child_out (although I
: can't actually test it on VMS :).
: -  if (exit_code & 1 != 0)
: +  if ((exit_code & 1) != 0)

Strictly speaking, it's not a bug, since "exit_code & 1 != 0" parses
as "exit_code & (1 != 0)" which is the same as "exit_code & 1" and
thus "(exit_code & 1) != 0", but I doubt that was the intention. ;)

--- job.c.orig  2013-05-04 04:08:09.000000000 +0200
+++ job.c       2013-05-04 08:32:43.000000000 +0200
@@ -617,25 +617,7 @@
               CLOSE_ON_EXEC (c->errfd);
             }
         }
-
-      return 1;
     }
-
-  /* We already have a temp file.  On per-job output, truncate and reset.  */
-  if (output_sync == OUTPUT_SYNC_JOB)
-    {
-      if (c->outfd >= 0)
-        {
-          lseek(c->outfd, 0, SEEK_SET);
-          ftruncate(c->outfd, 0);
-        }
-      if (c->errfd >= 0 && c->errfd != c->outfd)
-        {
-          lseek(c->errfd, 0, SEEK_SET);
-          ftruncate(c->outfd, 0);
-        }
-    }
-
   return 1;
 }
 
@@ -679,6 +661,11 @@
       }
     }
 
+  /* Truncate and reset input file to avoid duplicating data
+     in case it will be used again.  */
+  lseek (from_fd, 0, SEEK_SET);
+  ftruncate (from_fd, 0);
+
  finished:
 
 #ifdef WINDOWS32