[bug #33138] .PARLLELSYNC enhancement with patch

[Frank Heckenbach]

Follow-up Comment #5, bug #33138 (project make):

David Boyce wrote:

> This is the original author. I've become very busy at my day job in the
last
> year or two so I've lost track of this. Thanks for picking it up and
improving
> it. I haven't had time to look at your new patch yet, and not sure when I
> will, but here are a few responses to your comments:

Thanks for your reply.

> > Target vs flag
> 
> I have to agree that a flag is more "correct". It's more convenient to use
> .PARALLELSYNC because getting users to use a flag can be difficult (in fact
> the major insight of Feldman's original make was that a naive user should
be
> able to just type "make" and the right thing will happen),

If the "right thing" depends on the setting (like with
".SECONDEXPANSION" or various other flags), I definitely agree with
this reasoning.

However, in this case, it's not a question whether the build works
correctly or not, just how the output is presented to the user.
Therefore, different users may legitimately want different settings,
and this is easier to do with an argument than by modifying the
Makefile. Furthermore, this particular feature is interesting only
with parallel builds which require an option ("-j") anyway. And, as
I mentioned, passing it to recursive makes is also easier this way.

> but a flag is more
> flexible. To the best of my memory (not so good) the reasons I went with a
> special target are:
> 
> 1. Given the clever --eval flag Paul added in 3.82 any special target can
be
> used as a flag, e.g. "--eval .PARALLELSYNC:" (BTW I tried to argue that
--eval
> was worthy of a scarce single-letter alias (-E) on the grounds that it was
a
> flag-to-end-all-flags but I don't think that happened).

I wasn't aware of this feature. So it should also work this way. but
if we want to support both ways I implemented (fine and coarse) we'd
need two special targets. So I still prefer the option.

> 2. Paul has some concerns about make "becoming like GNU tar" which
apparently
> has too many options. Personally I'm always willing to trade a few more
> minutes of RTFM for more capability, and have no problem with GNU tar, but
> tastes differ.

Same for me. (So far make is quite short of the 52 available
letters. :)

What I could imagine is actually to make it the default when using
"-j" on the assumption that most users would prefer a properly
grouped output, and turn it off with an option. But it may be too
intrusive a change for a new feature, and I don't have a strong
opinion on that. (For me, it's just a one-time setup in my script
that invokes make with the right "-j" option depending on the system
configuration anyway.)

> > tmpfile vs mkstemp
> 
> IIRC the reason I used tmpfile() instead of mkstemp() is that mkstemp
doesn't
> unlink the file automatically, which leads to a number of risk factors such
as
> filling up /tmp and so on.
> 
> Possibly the best/most portable approach would be to refactor the existing
> open_tmpfile() function, which returns a FILE *, into an open_tmpfd() which
> returns a descriptor wrapped by open_tmpfile() which just converts the
> descriptor into a FILE * using fdopen(). This is already what happens, it's
> just a matter of splitting existing logic into two functions. Now
open_tmpfd()
> is logically just mkstemp-with-porting-hacks and we could enhance it with
an
> "unlink" option for this feature.

I agree this seems useful. However, since it would involve changes
to existing code which are not strictly needed for this new feature
(it works as it is), I'd rather wait for the go-ahead from one of
the maintainers before making such a change.

> > make[1]: Leaving directory 'bar'
> > make[1]: Leaving directory 'bar'
> 
> The problem with doubling these is that it becomes non-deterministic. What
if
> there was a directory structure foo/bar/bar/baz?

Actually the messages contain the absolute directory name (retrieved
by "getcwd (current_directory, GET_PATH_MAX)"); I just cut them in
my mail to avoid clutter and not expose my directories. So I don't
think there's actually a problem.

But I noticed another problem: Messages output by make itself (such
as "Makefile:42: recipe for target 'foo' failed") are not always
properly enclosed with enter/leave messages. If they mention file
names, as here "Makefile", this leads to the same issue.

Therefore I modified message(), error() and fatal() to enclose each
message with enter/leave messages, but only if parallel_sync is
active. (Otherwise, for simple uses it would clutter the output too
much.) The special case of message() with fmt==0 is left unchanged,
so the basic enter/leave messages still appear, even if no other
message are produced (the normal case with "-s" and clean builds),
as a kind of progress indicator.

Of course, when parallel_sync is used, this now results in even more
enter/leave messages, but if we want messages properly associated
with directories and don't want to mess with the messages
themselves, I see no alternative. (Again, this mode would usually be
used with machine-parsers like the Emacs directory tracker where the
number of messages won't matter so much as their reliability.)

> Additionally, would it be possible to implement the enhancement proposed in
> the email discussion? See
> http://lists.gnu.org/archive/html/bug-make/2011-04/msg00041.html for
context?
> Or was that already done? I don't remember.

You mean merging stdout and stderr if and only if they were the same
originally? That was implemented already (before my changes).

> Last, Paul has been pretty clear lately that new patches should be
accompanied
> by associated regression tests. That might be the remaining hurdle.

OK, I added some tests, basically my test case as posted, with
coarse and fine sync, and a test to check the above issue WRT
make-generated messages. (features/parallel-sync)

I also added spaces before parentheses according to the coding
standards and made sure it builds without warnings if PARALLEL_SYNC
is not used.

Frank


(file #27206)
    _______________________________________________________

Additional Item Attachment:

File name: make-sync-recursive-2.patch    Size:19 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?33138>

_______________________________________________
  Nachricht gesendet von/durch Savannah
  http://savannah.gnu.org/