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

[Eli Zaretskii]

> Date: Wed, 24 Apr 2013 18:50:15 +0200
> Cc: address@hidden
> From: Frank Heckenbach <address@hidden>
> 
> > That's one way.  Another one is to discuss the design more thoroughly
> > before the patches are accepted.
> 
> I think it was discussed quite extensively. Also in retrospect, I
> don't see how the design could have been much different (see below).

Fine, then this discussion should probably be terminated.  The last
thing I want is to annoy people.  If you think that everything is and
was like it should have, then I will have to live with that.  It's not
the first time, either, and Make is not the first project where I saw
this happening.

> > I tried to turn the discussion that
> > way when the issue was first brought up, but my comments were largely
> > ignored (if I compare what was suggested then with what was eventually
> > committed), and most of the discussions were about the details of the
> > Unix implementation anyway.
> 
> I just reread the whole discussion, and as far as I can see, most of
> your questions were answered.

"Answered", yes.  But the real reason behind my questions was not
really to have them answered.  It was to affect the design and the
implementation of the feature.  In that, I failed.  You might argue
that I failed because there was nothing to change anyway, but I beg to
differ.

> : Yes, but a few words about how is this semaphore supposed to get job
> : done, and in fact what kind of "synchronization" will this bring to
> : Make, would be appreciated.  I don't think you described the feature
> : too much in your original post.
> 
> Just like David, I really don't know what more you want described.

Nothing.  I already understood what's there to understand, thanks.
I'm half way into implementing this on Windows, which wouldn't have
been possible _unless_ I understood this completely.  The problems,
whatever they were, and the reasons for my rant are history now,
i.e. immutable and uncorrectable.  We can only behave differently in
the future.  Or not.

> : Btw, there will be other side effects, at least on non-Posix
> : platforms, due to the fact that stuff that was supposed to go to the
> : screen is redirected to a file instead.  Some programs sense that and
> : behave differently, e.g. with binary non-printable characters or with
> : special character sequences.  By redirecting the output to files, then
> : displaying it on the screen, the output may look strangely, or have
> : some more grave effect, like terminating output prematurely.
> 
> I don't see why it would terminate prematurely

It was long ago, but I presume I thought about the ^Z character that
some programs write or interpret to signal end of file.  Writing that
to the console stops output, AFAIR.

> - Change the command (in the ls example, use "--color=always").

This will only work on a Posix console.  The Windows console doesn't
support the SGR sequences, and will display them as ugly binary
garbage.  (When 'ls' or 'grep' run on Windows and their stdout is
connected to a console, they use special console output commands to
produce color, but those commands cannot be recorded in a file and
"replayed", as you can with SGR sequences.)

> What I'm saying is the circumstances where this is a real problem
> seem rather exotic, and noone's forcing you to use the option. If we
> were talking about changing make's default behaviour, the concern
> may be justified, but we aren't.

I just mentioned some caveats that people might find surprising,
that's all.  Perhaps those caveats should be mentioned in the
documentation.  No other intentions.

> : We should consider these potential problems, and in any case the
> : simple loop that reads from temporary files and writes to
> : stdout/stderr may need to become much more complicated, at least
> : on DOS/Windows.  (I believe similar, though less serious, problems
> : could happen on Unix as well.)
> 
> Apart from setting O_BINARY, I don't see why. Can you explain?

You just explained it yourself.  Sure, if we don't want to do anything
about those issues, then no complications are needed.

> WRT O_BINARY, you wrote:
> 
> :  . pump_from_tmp_fd will need to switch to_fd to binary mode, since
> :    this is how tmpfile opens the temporary file; we need output mode
> :    match the input mode.
> 
> I was about to add it

Please don't.  I'm actively working on this, will test it, and hope to
commit the changes in a few days.  Making such changes in parallel
will just make my merging harder.

> but now I wonder if that's really needed: If the temp file is opened
> in binary mode, and this temp file is passed as stdout/stderr to the
> shell commands, those will write their output in binary mode

No, they won't.  The text/binary mode is per process (and per file
descriptor in the process), it is not global and not per resource.
That's because the text mode is implemented in the C library and uses
data private to each process.

Therefore, even if the handle is inherited by child processes, those
processes will not magically start using binary I/O on these handles,
they will use whatever mode was coded in their own code.

The result is that Make will _read_ from the temporary file in binary
mode, and will get all the CR-LF EOLs verbatim, without the CRs
stripped.  It therefore must write in binary mode, to faithfully
reproduce what would have happened without going through the temporary
files.

> Later
> pump_from_tmp_fd() will read it back in binary and dump it to the
> original stdout/stderr in whatever mode it was (usually non-binary),
> so the CRs get added there.

Right, so we will get two CRs in a row, and we will stop on the first
^Z character.  This cannot be allowed.

> Or is there any special handling of stdout/stderr WRT binary mode
> during inheritance or so?

It's not specific to stdout/stderr, what I described above will happen
with any inherited file descriptor.

> - Factor out is_same_file() into a function.

I already wrote this in my branch, please wait until I commit.

> - Turn file_ok() and fd_not_empty() into functions instead of
>   macros. (No real need for macros, and functions may be easier to
>   port.)

Fixed that as well here.

> To sum, what's missing for Windows is:
> 
> - implement file_ok() and is_same_file() (both take only stdout and
>   stderr as arguments, in case this helps)

Already have that done.

> - imeplement acquire_semaphore()/release_semaphore(), probably using
>   a real mutex instead of file locking, and add code to pass down
>   the mutex name or handle to sub-makes.

Almost done, modulo testing.

There's one issue that perhaps needs discussing.  A mutex is
identified by a handle, which on Windows is actually a pointer to an
opaque object (maintained by the kernel).  As such, using just 'int'
for sync_handle is not wide enough, certainly not in 64-bit builds.
Is it OK to use intptr_t instead?  Doing this cleanly might require to
have a macro (I called it FD_FROM_HANDLE) to extract a file descriptor
that is passed to a Posix fcntl; on Windows that macro will be a
no-op, and on Posix platforms it's a simple cast.

Is this OK?

> >From my view, all of this has been established several mails ago, so
> I really wonder what's the problem now.

I didn't say there was a problem, not one standing in my way of
getting this done.  Apologies if I somehow left people wondering about
all this.

> +/* Test whether a file contains any data. */
> +static int
> +fd_not_empty (int fd)
> +{
> +  return fd >= 0 && lseek (fd, 0, SEEK_END) > 0;
> +}

Isn't this unnecessarily expensive (with large output volumes)?  Why
not use fstat?