date:20130424

Eli Zaretskii wrote:

 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).

 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. Already in
http://lists.gnu.org/archive/html/bug-make/2011-04/msg00033.html
David wrote: Yes, it's conceptually a semaphore. In fact a Windows
port might prefer to use real semaphores.

You wrote:

: 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. I
can repeat what I wrote yesterday: The only thing really necessary
is that two makes [with the same stdout/stderr] never run
sync_output() simultaneously. (Note: We're only talking about makes,
i.e. cooperating processes; we never need to synchronize the
processes run from the recipes, apart from recursive makes.)

That's really all there is to it, the rest is implementation details
(and may be totally different on Windows). Perhaps you're thinking
of it as much more complicated than is really is. It's just about
the simplest possible use case of an inter-process mutual exclusion
(all processes related, so they can pass info around; all
cooperating; just one code block to lock).

: 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, but indeed it may
display differently, also on Unix, e.g. ls --color=tty may use
colors whwn writing to a terminal and not to a file. So if you use
output-sync and your recipes contains this command, you lose colors.
I'd say you have several options:

- Live with it.

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

- If the effect is more serious than losing colors (you say it may
  look strangely), maybe you shouldn't put such a command in a make
  recipe in the first place (at least not in a Makefile that's going
  to be distributed; note that the same problems will happen if the
  user redirects the whole make output to a file which I often do).

- If the Makefile is only used in-house and you definitely want the
  output-device-dependent behaviour, just don't use output-sync.
  It's just an option.

- Provide your own work-around, perhaps write your output to a pty.

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.

: 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?

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, 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 (i.e., without CR, AIUI). 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. Or is there any special handling of
stdout/stderr WRT binary mode during inheritance or so?

I'm attaching a small patch to:

- Factor out is_same_file() into a function.

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

- Use SEEK_END rather than SEEK_CUR in fd_not_empty(), so it also
  reports true if the fd is at the beginning of a non-empty file
  (such as if the position is not shared between processes
  inheriting

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

 Date: Wed, 24 Apr 2013 05:56:49 +0300
 From: Eli Zaretskii e...@gnu.org
 Cc: f.heckenb...@fh-soft.de, bug-make@gnu.org

 I will see if locking works on console handles; if not, I will have
 to introduce a command-line argument for passing the name or the
 handle of a mutex to a sub-Make.

As I suspected, neither LockFileEx nor LockFile work on console
handles, they all fail with ERROR_INVALID_HANDLE.  The functions do
work on regular files, but since (as it turns out) file locks on
Windows are mandatory, not advisory as on Posix platforms, they are
not really suitable for this job anyway.

So file locking is out, mutex is in.

___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

 Date: Thu, 18 Apr 2013 21:57:56 +0300
 From: Eli Zaretskii e...@gnu.org
 Cc: bug-make@gnu.org, bo...@kolpackov.net

  Date: Thu, 18 Apr 2013 19:09:06 +0200
  From: Frank Heckenbach f.heckenb...@fh-soft.de

. calculation of combined_output in start_job_command will need to be
  reimplemented for Windows, since the reliance on st_dev and st_ino
  makes assumptions that are false on Windows.

  What we need is basically is_same_file (fd1, fd2) (which we
  probably could refactor into a separate function). Eli, can you try:

test file1 -ef file2

  where test is either a standalone utility or a built-in of bash or
  any other shell. If any of these works, you can see how they do it.
  Otherwise, a bit of googling turned up this page which has code (in
  Python, but it seems like a thin layer around the system calls, so
  it can probably easily be ported to C):

  http://timgolden.me.uk/python/win32_how_do_i/see_if_two_files_are_the_same_file.html

 I know about this method, but I'm not sure it does what we want here.
 The number used by this method is not guaranteed to be unique on some
 versions of Windows.  More importantly, it only works for disk files,
 I don't know whether it reports a meaningful value for the console
 device or the null device.

Testing clearly shows it doesn't: GetFileInformationByHandle simply
fails for handles open on console devices and the null device.  And we
will be comparing handles for console devices in the majority of use
cases here.

I researched this a bit, and eventually succeeded in finding a way
that is good enough for both disk files, pipes, and console devices.

  . STREAM_OK uses fcntl and F_GETFD which both don't exist on Windows.

This is to test if somehow make's stdout or stderr has been closed (not
just redirected to /dev/null, but closed).

   Right, got that; but Windows needs a different test for that, as
   there's no fcntl.

  Basically we need just any call that succees for a valid file and
  fails otherwise. fstat() might be a good candidate.

 fstat is too heavy on Windows; there are better methods.

  If that isn't available, maybe even lseek() (as used in
  FD_NOT_EMPTY) will do if we keep the || errno != EBADF test.

 lseek fails for the console as well, so it's not a good candidate.
 But this problem is not difficult, I just mentioned that the code as
 committed won't compile.

  Eli, can you do some tests to see if you find something that works
  reliably?

 Don't worry about that.  My worst problem is lack of time, not lack of
 ideas ;-)

Found a reasonable solution for that as well.

___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

Eli Zaretskii wrote:

  I know about this method, but I'm not sure it does what we want here.
  The number used by this method is not guaranteed to be unique on some
  versions of Windows.  More importantly, it only works for disk files,
  I don't know whether it reports a meaningful value for the console
  device or the null device.
 
 Testing clearly shows it doesn't: GetFileInformationByHandle simply
 fails for handles open on console devices and the null device.  And we
 will be comparing handles for console devices in the majority of use
 cases here.

That's right. Or perhaps pipes (I post-process make's output, to
handle the directories in messages etc. and I suppose the mentioned
Emacs mode does something similar).

 I researched this a bit, and eventually succeeded in finding a way
 that is good enough for both disk files, pipes, and console devices.

Good. (And the null device, I suppose. Though it doesn't really
matter if we write to the null device combined or separate. ;-)

   . STREAM_OK uses fcntl and F_GETFD which both don't exist on 
  Windows.
  
   Basically we need just any call that succees for a valid file and
   fails otherwise.

 Found a reasonable solution for that as well.

OK, so only one problem left.

___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

2013-04-24 Thread Tim Murphy

Some time ago when solving the same problem in a different way we used
semaphores on Windows and Linux. Compatibility might make it less
interesting but I would suggest pretending that one has semaphores first
with some nice little abstraction and then implementing them in the best
way the platform has to offer.

We didn't need to support so many platforms but you can see the idea here:
https://bitbucket.org/tnmurphy/raptor/src/fbb2e624d320e5eabc0689105e2f2b80d131ca03/util/talon/sema.h?at=default

Basically each build needs a unique id to be passed around in an
environment variable. The top-level make executable's PID should be good
enough. This is used to create the named semaphore.

Platforms without semaphores might use a file and locking or if that
doesn't work then they can't implement the feature and you configure it out.

Regards,

Tim

On 24 April 2013 18:14, Frank Heckenbach f.heckenb...@fh-soft.de wrote:

Eli Zaretskii wrote:

I know about this method, but I'm not sure it does what we want here.
The number used by this method is not guaranteed to be unique on some
versions of Windows. More importantly, it only works for disk files,
I don't know whether it reports a meaningful value for the console
device or the null device.

Testing clearly shows it doesn't: GetFileInformationByHandle simply
fails for handles open on console devices and the null device. And we
will be comparing handles for console devices in the majority of use
cases here.

That's right. Or perhaps pipes (I post-process make's output, to
handle the directories in messages etc. and I suppose the mentioned
Emacs mode does something similar).

I researched this a bit, and eventually succeeded in finding a way
that is good enough for both disk files, pipes, and console devices.

Good. (And the null device, I suppose. Though it doesn't really
matter if we write to the null device combined or separate. ;-)

. STREAM_OK uses fcntl and F_GETFD which both don't exist on
Windows.

Basically we need just any call that succees for a valid file and
fails otherwise.

Found a reasonable solution for that as well.

OK, so only one problem left.

___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

--
You could help some brave and decent people to have access to uncensored
news by making a donation at:

http://www.thezimbabwean.co.uk/friends/
___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

2013-04-24 Thread David Boyce

On Wed, Apr 24, 2013 at 10:26 AM, Tim Murphy tnmur...@gmail.com wrote:

  I would suggest pretending that one has semaphores first with some nice
 little abstraction and then implementing them in the best way the platform
 has to offer.


How about we introduce functions called acquire_semaphore() and
release_semaphore()? Oh, wait, we did.

Platforms without semaphores might use a file and locking or if that
 doesn't work then they can't implement the feature and you configure it out.


This kind of (re)invention is not known to be needed at this time. I have
the impression Eli has the Windows port in hand, he's just skeptical about
a few details. And what's there now is already the LCD for most other
platforms as far as I know.

David
___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

 Date: Wed, 24 Apr 2013 18:50:15 +0200
 Cc: bug-make@gnu.org
 From: Frank Heckenbach f.heckenb...@fh-soft.de

  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

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

 Date: Wed, 24 Apr 2013 19:14:01 +0200
 Cc: bug-make@gnu.org
 From: Frank Heckenbach f.heckenb...@fh-soft.de

  Testing clearly shows it doesn't: GetFileInformationByHandle simply
  fails for handles open on console devices and the null device.  And we
  will be comparing handles for console devices in the majority of use
  cases here.

 That's right. Or perhaps pipes (I post-process make's output, to
 handle the directories in messages etc. and I suppose the mentioned
 Emacs mode does something similar).

GetFileInformationByHandle does work on pipes, at least on shell
pipes (as in foo | cat).

  I researched this a bit, and eventually succeeded in finding a way
  that is good enough for both disk files, pipes, and console devices.

 Good. (And the null device, I suppose. Though it doesn't really
 matter if we write to the null device combined or separate. ;-)

No, I didn't find a satisfactory solution for the null device.  But as
you point out, we don't care.

___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

 Date: Wed, 24 Apr 2013 10:34:20 -0700
 From: David Boyce david.s.bo...@gmail.com
 Cc: Frank Heckenbach f.heckenb...@fh-soft.de,
   bug-make@gnu.org bug-make@gnu.org

 On Wed, Apr 24, 2013 at 10:26 AM, Tim Murphy tnmur...@gmail.com wrote:

   I would suggest pretending that one has semaphores first with some nice
  little abstraction and then implementing them in the best way the platform
  has to offer.

 How about we introduce functions called acquire_semaphore() and
 release_semaphore()? Oh, wait, we did.

Sorry, but this is not funny.  There are, indeed, such functions, but
that's about all there is about modularity in the implementation.  The
rest is code scattered all over the place, that _knows_ all too well
that it is dealing with file descriptors, not with some abstract
semaphores or mutexes.  Here's a good example:

  if (output_sync)
{
  static int combined_output;
  /* If output_sync is turned on, find a resource to
  synchronize on. This block is traversed only once. */
  if (sync_handle == -1)
{
  if (STREAM_OK (stdout))
{
  struct stat stbuf_o, stbuf_e;

  sync_handle = fileno (stdout);
  combined_output =
fstat (fileno (stdout), stbuf_o) == 0 
fstat (fileno (stderr), stbuf_e) == 0 
stbuf_o.st_dev == stbuf_e.st_dev 
stbuf_o.st_ino == stbuf_e.st_ino;
}
  else if (STREAM_OK (stderr))
sync_handle = fileno (stderr);
  else
{
  perror_with_name (output-sync suppressed: , stderr);
  output_sync = 0;
}
}

There are 2 separate things intermixed here: determination of
combined_output and determination of the resource to use as a mutex.
They are mixed because they both deal with file descriptors, and it
was just convenient, by sheer luck (or maybe because of something
else) to save a few if's and do them together.

But this is exactly where abstractions leak and break.

It will come as small surprise, I'm sure, that the corresponding code
in the Windows branch looks different, because there the two decisions
are uncoupled.

 I have the impression Eli has the Windows port in hand

Almost, yes.  I needed to invest some time into researching how to do
is_same_file, STREAM_OK, CLOSE_ON_EXEC, etc.  But I'm getting there.

 he's just skeptical about a few details.

Not anymore, thanks.

___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

On Wed, 2013-04-24 at 21:17 +0300, Eli Zaretskii wrote:
 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?

I think the lock or lock handle or whatever can be encapsulated into
a typedef, which will be different between the different platforms.
That sounds like a good abstraction to me.

We can even generalize the way in which we communicate the handle to
sub-makes; for example by calling a function with the handle that
returns a char*: if that value is non-NULL it's added to the command
line (maybe as a third element to the current jobs-fd argument).  If
it's null, nothing is added.  Then the submake can parse it out and hand
it back to a function that returns a handle again
(serialize/deserialize).  I'm not sure if this is necessary; it depends
on the details of the Windows model.

  +/* 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?

This lseek() doesn't actually move the file reference: SEEK_END plus an
offset of 0 is a no-op so it doesn't matter how large the file is.  This
is just seeing if the position has moved since we opened the file (still
at 0 or not); it just returns the current position in the file, which is
known to the system directly without having to go ask anyone (it has to
be so, since each file descriptor has its own position).

I would be greatly surprised if fstat(), which has to go to the
directory (probably) to look up all the information on the file such as
ownership, permissions, etc., is faster.


___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

Eli Zaretskii wrote:

  : 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.

Oh, that's still done? When I last used Dos in the 1990s it was
already obsolete and few programs did it. OK, so you'll have to
strip them. Fortunately, that's not too hard, I assume you have
already implemented it.

  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.

OK, we can add a few sentences in the docs. Have you already written
something (just to avoid further dupliation of work)?

  - 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.

OK, so Paul now has two versions to choose from. :-)

Really, I just tried to be helpful. Since you were constantly
complaining about the design, I did the (small) things I saw to
improve about it. Other than that I didn't (and still don't) know
what's so bad about the design.

 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).

And the pointer is the same between different processes?

 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?

I can't follow you here. On POSIX, we don't need to pass a fd
because it's always stdout/stderr. Or do mean something else?

  +/* 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?

On POSIX both lseek and fstat are not very expensive. On Windows,
you said fstat was very expensive, didn't you? Or is lseek even
worse?

  How about we introduce functions called acquire_semaphore() and
  release_semaphore()? Oh, wait, we did.
 
 Sorry, but this is not funny.  There are, indeed, such functions, but
 that's about all there is about modularity in the implementation.  The
 rest is code scattered all over the place, that _knows_ all too well
 that it is dealing with file descriptors, not with some abstract
 semaphores or mutexes.  Here's a good example:

 [...]

 There are 2 separate things intermixed here: determination of
 combined_output and determination of the resource to use as a mutex.
 They are mixed because they both deal with file descriptors, and it
 was just convenient, by sheer luck (or maybe because of something
 else) to save a few if's and do them together.

I talked about this yesterday or so. I still don't see why you can't
keep this code as it. You can use sync_handle as a flag to make sure
the code is executed only once and ignore it for the locking
purpose.

Of course, you can rewrite the code to separate the two things, but
I still don't see that it's necessary. The checks to be done
(file_ok, is_same_file) are the same anyway, so I don't see it as a
design flaw.

Paul Smith wrote:

   +/* 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?
 
 This lseek() doesn't actually move the file reference: SEEK_END plus an
 offset of 0 is a no-op so it doesn't matter how large the file is.  This
 is just seeing if the position has moved since we opened the file (still
 at 0 or not); it just returns the current position in the file, which is
 known to the system directly without having to go ask anyone (it has to
 be so,

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

I'm fully prepared to accept the blame for not doing the best job
getting buy-in etc. on this effort.  Can we leave the discussion on the
process behind?  I'd prefer that, unless there are real constructive
comments on how to do better next time rather than rehashing what was
done wrong.  I think we have discussed that enough, and I'd prefer to
avoid going further along that road.

Thanks.


On Wed, 2013-04-24 at 20:46 +0200, Frank Heckenbach wrote:
 That's true about SEEK_CUR which was there originally. I actually
 changed it to SEEK_END, which does move the position to the end.

Oh right.  My head cold is keeping me foggy.  What was the reason to
change to SEEK_END, again?

I'm not so sure fstat() is that cheap.  struct stat contains a lot of
information.  Although I guess since we are only ever talking about temp
files, not NFS files or something, it's probably not too bad.


___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

 From: Paul Smith psm...@gnu.org
 Cc: e...@gnu.org, bug-make@gnu.org
 Date: Wed, 24 Apr 2013 15:07:21 -0400

 I'm not so sure fstat() is that cheap.  struct stat contains a lot of
 information.  Although I guess since we are only ever talking about temp
 files, not NFS files or something, it's probably not too bad.

We could time it if we are afraid of the cost, but I'd be surprised if
'fstat' wasn't extremely fast on Posix platforms.  Most of the
information you get in struct stat is already available when the file
is open.  In particular, the OS tracks the file's size as it is being
written.

(On Windows, we will have to use a different method.)

___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

 Date: Wed, 24 Apr 2013 20:46:14 +0200
 Cc: p...@mad-scientist.net, bug-make@gnu.org
 From: Frank Heckenbach f.heckenb...@fh-soft.de

   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.

 Oh, that's still done? When I last used Dos in the 1990s it was
 already obsolete and few programs did it. OK, so you'll have to
 strip them. Fortunately, that's not too hard, I assume you have
 already implemented it.

No need: if the file is read and written in binary mode, ^Z is just a
character like any other.

  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.

 OK, we can add a few sentences in the docs. Have you already written
 something (just to avoid further dupliation of work)?

No.  Go ahead, if you have time.

 Really, I just tried to be helpful.

I realize, and I am grateful.  The discussion was very helpful.

  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).

 And the pointer is the same between different processes?

I arrange for it to be inherited, therefore yes.  This way, I don't
have to invent a unique name and don't risk a slim chance that some
stale process left a mutex by the same name around.  (Still need to
make sure that nameless mutexes will do the job in this case, though.)

  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?

 I can't follow you here. On POSIX, we don't need to pass a fd
 because it's always stdout/stderr. Or do mean something else?

I meant the file descriptor passed to fcntl to put a lock on it.  The
value of that descriptor is stored in sync_handle, whose type is
'int'.

   +/* 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?

 On POSIX both lseek and fstat are not very expensive.

But fstat should be cheaper, as it already tracks the size of the
file, which is what you want here.

 On Windows, you said fstat was very expensive, didn't you? Or is
 lseek even worse?

I think anything that potentially moves the file pointer can be
sometimes expensive and is best avoided.  (On Windows, I'd use
GetFileInformationByHandle.)

  There are 2 separate things intermixed here: determination of
  combined_output and determination of the resource to use as a mutex.
  They are mixed because they both deal with file descriptors, and it
  was just convenient, by sheer luck (or maybe because of something
  else) to save a few if's and do them together.

 I talked about this yesterday or so. I still don't see why you can't
 keep this code as it.

Because the Windows implementation doesn't care whether stdout or
stderr (or both) are OK.  It doesn't use their file descriptor for
locking, so it doesn't care.  It just needs to know, for the purposes
of combined_output, whether they point to the same file/device.  But
for determining what to use for sync_handle, this is irrelevant.  So
the logic looks different.

 You can use sync_handle as a flag to make sure the code is executed
 only once and ignore it for the locking purpose.

I need to store the handle for the mutex somewhere, and sync_handle
sounds just like the right place.

 Nothing is actually read by lseek (and even if it were, it would
 only need to look at the first and last part of the file, not read
 all the content, if that was the worry).

Are you sure?  How can lseek jump to the last byte of the file, if
the file is not contiguous on disk, except by reading some of it?

Anyway, we should time this instead of arguing.

Or maybe we should abandon this optimization and take the lock
regardless.  How bad can that be?

___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

On Wed, 2013-04-24 at 22:25 +0300, Eli Zaretskii wrote:
  From: Paul Smith psm...@gnu.org
  Cc: e...@gnu.org, bug-make@gnu.org
  Date: Wed, 24 Apr 2013 15:07:21 -0400

  I'm not so sure fstat() is that cheap.  struct stat contains a lot of
  information.  Although I guess since we are only ever talking about temp
  files, not NFS files or something, it's probably not too bad.

 We could time it if we are afraid of the cost, but I'd be surprised if
 'fstat' wasn't extremely fast on Posix platforms.  Most of the
 information you get in struct stat is already available when the file
 is open.  In particular, the OS tracks the file's size as it is being
 written.

True.  It's probably all right there and not measurably different if you
have a file descriptor already.

___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

On Wed, 2013-04-24 at 22:39 +0300, Eli Zaretskii wrote:
  Nothing is actually read by lseek (and even if it were, it would
  only need to look at the first and last part of the file, not read
  all the content, if that was the worry).
 
 Are you sure?  How can lseek jump to the last byte of the file, if
 the file is not contiguous on disk, except by reading some of it?

If fstat() can get the size from an internal structure then lseek() can
do the same, then just update the file descriptor's position.  I don't
think there's more to it than setting that value, but it could be.
Certainly at the filesystem layer we don't know, and we don't care,
about things like whether the file is kept contiguously at the block
layer.

As you say, we should just measure.

 Or maybe we should abandon this optimization and take the lock
 regardless.  How bad can that be?

Well, we want to know if the file has any content anyway: for example we
don't want to output the enter/leave notifications if there's nothing to
print.  So there's no extra cost to avoiding the lock here.



___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

2013-04-24 Thread Tim Murphy

why not use a named semaphore wherever possible (windows and linux) and
lock a file where not instead of trying to pass kernel object handles
around (seems a bit nasty to me)?


On 24 April 2013 21:19, Paul Smith psm...@gnu.org wrote:

 On Wed, 2013-04-24 at 22:39 +0300, Eli Zaretskii wrote:
   Nothing is actually read by lseek (and even if it were, it would
   only need to look at the first and last part of the file, not read
   all the content, if that was the worry).
 
  Are you sure?  How can lseek jump to the last byte of the file, if
  the file is not contiguous on disk, except by reading some of it?

 If fstat() can get the size from an internal structure then lseek() can
 do the same, then just update the file descriptor's position.  I don't
 think there's more to it than setting that value, but it could be.
 Certainly at the filesystem layer we don't know, and we don't care,
 about things like whether the file is kept contiguously at the block
 layer.

 As you say, we should just measure.

  Or maybe we should abandon this optimization and take the lock
  regardless.  How bad can that be?

 Well, we want to know if the file has any content anyway: for example we
 don't want to output the enter/leave notifications if there's nothing to
 print.  So there's no extra cost to avoiding the lock here.



 ___
 Bug-make mailing list
 Bug-make@gnu.org
 https://lists.gnu.org/mailman/listinfo/bug-make




-- 
You could help some brave and decent people to have access to uncensored
news by making a donation at:

http://www.thezimbabwean.co.uk/friends/
___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

On Wed, 2013-04-24 at 22:55 +0100, Tim Murphy wrote:
 why not use a named semaphore wherever possible (windows and linux)
 and lock a file where not instead of trying to pass kernel object
 handles around (seems a bit nasty to me)?

Hi Tim; I think you're late to the party :-).  Let me summarize a lot of
discussion then we can use this as a reference for other questions.

Named semaphores on POSIX are kind of sucky.  They don't get
automatically cleaned up when the process exits, for one thing.  And my
suspicion is that they're not very portable across different variations
of UNIX especially older ones.

File locking, on the other hand, is very portable, very simple, very
fast, and any held locks are automatically released when the descriptor
is closed.  No muss, no fuss.

If you agree file locking is the right way to go on a POSIX system, then
we just have to decide what to lock.  It can be anything as long as all
children of a given top-level make can find it.  We could use a
temporary file, sure.  If we did that we'd have to pass around either
the name of the file or, more likely, use tmpfile() and send along the
file descriptor, handle close-on-exec, etc. like we do with the
jobserver pipe.  Which we could certainly do: we do it today with
jobserver.

However we already have a descriptor available to us: stdout.  If we use
that we don't have to pass around anything because our children are
already inheriting it and it's on a well-known FD.  We don't have to
worry about using + before sub-make rules like we do with jobserver.

There's one other advantage of using stdout: if a sub-make redirects its
output to a separate file, that magically starts a new lock domain and
that sub-make and its children don't contend with the other sub-makes
and their children.  This is not a big deal and if we didn't get it for
free, we'd never go to the effort of implementing it.  But it's nice.


Eli is working on the Windows port; I have no idea how he's decided to
implement this there yet.


___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

Paul Smith wrote:

 On Wed, 2013-04-24 at 20:46 +0200, Frank Heckenbach wrote:
  That's true about SEEK_CUR which was there originally. I actually
  changed it to SEEK_END, which does move the position to the end.
 
 Oh right.  My head cold is keeping me foggy.  What was the reason to
 change to SEEK_END, again?

lseek (SEEK_END, 0) seeks to the end and then returns the current
position, i.e. the file size.

lseek (SEEK_CUR, 0) stays at the current position and returns it. So
if the fd (in the parent) remains at the start while the child wrote
something, it would return 0, whereas the former wouldn't.

AFAIK, lseek (SEEK_END, 0) is one of two canonical ways (besides
fstat) to get the size of a file.

Eli Zaretskii wrote:

   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.
  
  OK, we can add a few sentences in the docs. Have you already written
  something (just to avoid further dupliation of work)?
 
 No.  Go ahead, if you have time.

Attached.

   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?
  
  I can't follow you here. On POSIX, we don't need to pass a fd
  because it's always stdout/stderr. Or do mean something else?
 
 I meant the file descriptor passed to fcntl to put a lock on it.  The
 value of that descriptor is stored in sync_handle, whose type is
 'int'.

So you want to overload sync_handle to contain an fd on POSIX and a
pointer on Windows? I'm not sure I'd like this. Since it's used in
separate code branches (ifdef'd or perhaps in different source files
in compat) anyway, why not define separate variables of the
appropriate types (preferably with different names, to reduce
confusion)?

  On Windows, you said fstat was very expensive, didn't you? Or is
  lseek even worse?
 
 I think anything that potentially moves the file pointer can be
 sometimes expensive and is best avoided.  (On Windows, I'd use
 GetFileInformationByHandle.)

OK, if that's so, do that. But I don't think that's true on POSIX.

  Nothing is actually read by lseek (and even if it were, it would
  only need to look at the first and last part of the file, not read
  all the content, if that was the worry).
 
 Are you sure?  How can lseek jump to the last byte of the file, if
 the file is not contiguous on disk, except by reading some of it?

lseek doesn't need to read any data. It just sets the current offset
of the FD to the given position, so the next read (which in this
case never happens before seeking to the beginning) knows where to
read. Even in the case of SEEK_END, all it has to do is add the
given offset (here: 0) to the current file size.

Instead of testing, I just looked at the implementation (Linux
3.2.2). The following is really the whole relevant code. As you see,
nothing's read from the disk, it only handles in-memory data. (Also
the file size is in memory for open files; even it were not, it
would be a constant-time access to the inode and wouldn't need to
touch any data blocks.)

loff_t
generic_file_llseek_size(struct file *file, loff_t offset, int origin,
loff_t maxsize)
{
struct inode *inode = file-f_mapping-host;

switch (origin) {
case SEEK_END:
offset += i_size_read(inode);
break;
// skipped other cases
}

return lseek_execute(file, inode, offset, maxsize);
}

static loff_t lseek_execute(struct file *file, struct inode *inode,
loff_t offset, loff_t maxsize)
{
if (offset  0  !unsigned_offsets(file))
return -EINVAL;
if (offset  maxsize)
return -EINVAL;

if (offset != file-f_pos) {
file-f_pos = offset;
file-f_version = 0;
}
return offset;
}

static inline int unsigned_offsets(struct file *file)
{
return file-f_mode  FMODE_UNSIGNED_OFFSET;
}

static inline loff_t i_size_read(const struct inode *inode)
{
// skipped code to deal with preemption and synchronized access in SMP
return inode-i_size;
}
--- doc/make.texi.orig	2013-04-24 16:30:05.0 +0200
+++ doc/make.texi	2013-04-25 02:08:58.0 +0200
@@ -8639,6 +8639,17 @@
 complex parallel build in the background and checking its output
 afterwards.
 
+Note that with this option, each job's output is redirected to a
+temporary file first. Some commands can behave differently depending
+on the type of their standard output or standard error. E.g.,
+@code{ls --color=tty} might display a colored directory listing when
+standard output is connected to a terminal.

findstring/MAKEFLAGS example is not robust

2013-04-24 Thread Peter Eisentraut

The manual contains this example:

 archive.a: ...
 ifneq (,$(findstring t,$(MAKEFLAGS)))
 +touch archive.a
 +ranlib -t archive.a
 else
 ranlib archive.a
 endif

But this would also trigger if you ran make with any other option that
contains a t, for example --no-print-directory.

Is there a better way to write this?



___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

 From: Paul Smith p...@mad-scientist.net
 Cc: Frank Heckenbach f.heckenb...@fh-soft.de, bug-make@gnu.org
 Date: Wed, 24 Apr 2013 16:03:56 -0400

  Or maybe we should abandon this optimization and take the lock
  regardless.  How bad can that be?

 Well, we want to know if the file has any content anyway: for example we
 don't want to output the enter/leave notifications if there's nothing to
 print.

Why not?

___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

Eli Zaretskii wrote:

  From: Paul Smith p...@mad-scientist.net
  Cc: Frank Heckenbach f.heckenb...@fh-soft.de, bug-make@gnu.org
  Date: Wed, 24 Apr 2013 16:03:56 -0400

   Or maybe we should abandon this optimization and take the lock
   regardless.  How bad can that be?

  Well, we want to know if the file has any content anyway: for example we
  don't want to output the enter/leave notifications if there's nothing to
  print.

 Why not?

To avoid clutter. There are probably still too many of these
messages anyway, as discussed before.

Note, we still give the regular enter/leave messages as we do
without output-sync, so no information is lost. We just avoid
additional enter/leave messages with nothing in between.

___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

 Date: Wed, 24 Apr 2013 22:55:59 +0100
 From: Tim Murphy tnmur...@gmail.com
 Cc: bug-make@gnu.org bug-make@gnu.org

 trying to pass kernel object handles around (seems a bit nasty to
 me)?

Why is that nasty?  The handle is returned by a documented interface,
and communicating it to another process is an officially documented
technique.  How is that different from passing file descriptors, which
are indices into a kernel-maintained array?

___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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

 Date: Thu, 25 Apr 2013 02:16:33 +0200
 Cc: e...@gnu.org, bug-make@gnu.org
 From: Frank Heckenbach f.heckenb...@fh-soft.de

   I can't follow you here. On POSIX, we don't need to pass a fd
   because it's always stdout/stderr. Or do mean something else?

  I meant the file descriptor passed to fcntl to put a lock on it.  The
  value of that descriptor is stored in sync_handle, whose type is
  'int'.

 So you want to overload sync_handle to contain an fd on POSIX and a
 pointer on Windows?

I don't see it as an overloading.  'sync_handle' is already opaque,
even its name says so.  It just so happens that on Unix it is actually
a file descriptor.  And it just so happens that on Windows it will be
a handle to a mutex.

 I'm not sure I'd like this. Since it's used in separate code
 branches (ifdef'd or perhaps in different source files in compat)
 anyway, why not define separate variables of the appropriate types
 (preferably with different names, to reduce confusion)?

I don't see any confusion (how probable is it that someone who is
interested in Unix will ever look at the details of the Windows
implementation, and vice versa?).  And we never do that in Make, we
generally try to use the same variables.  If nothing else, it keeps
the number of ifdef's down.
 --- doc/make.texi.orig2013-04-24 16:30:05.0 +0200
 +++ doc/make.texi 2013-04-25 02:08:58.0 +0200
 @@ -8639,6 +8639,17 @@
  complex parallel build in the background and checking its output
  afterwards.

 +Note that with this option, each job's output is redirected to a
 +temporary file first. Some commands can behave differently depending
 +on the type of their standard output or standard error. E.g.,
 +@code{ls --color=tty} might display a colored directory listing when
 +standard output is connected to a terminal. When using this option,
 +colors would be disabled because the output of the command goes to a
 +file. Note, however, that it is generally best to avoid such
 +commands in makefiles, independent of this option, since the
 +different behavior would also be triggered when users redirect the
 +whole output of @code{make}, e.g. to a log file.

Thanks.  I think you need two spaces between sentences (that's GNU
Coding Standards' requirement), and command lines people type should
be in @kbd, not @code.  Also, e.g. needs either a comma or a @:
after it, to signal to TeX it's not an end of a sentence.

___
Bug-make mailing list
Bug-make@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-make

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