Re: [PATCH] gdb/doc: use silent-rules.mk in the Makefile

[Simon Marchi <simark@simark.ca>]

On 2024-04-16 04:47, Andrew Burgess wrote:
> Andrew Burgess <aburgess@redhat.com> writes:
> 
>> Simon Marchi <simark@simark.ca> writes:
>>
>>> On 4/15/24 9:55 AM, Andrew Burgess wrote:
>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>> So my preference would be for having the above emit something like the
>>>>> below instead
>>>>>
>>>>>   TEXI2POD gdb.pod
>>>>>   POD2MAN1 gdb.1
>>>>
>>>> That one's harder.  The target information comes from make's $@
>>>> variable, so it's easy enough to do:
>>>>
>>>>   TEXI2POD gdb.1
>>>>   POD2MAN  gdb.1
>>>>
>>>> which isn't exactly what you asked for.
>>>
>>> Could you split the rule in two?  One rule generating gdb.pod and one
>>> rule generating gdb.1.
>>>
>>> I personally think the original output from Andrew's patch is fine.  In
>>> the silent mode, all I need to know is that make is currently working on
>>> getting gdb.1 generated.  The intermediary gdb.pod file is an
>>> implementation detail of the rule.  If I want to see it, then I'll use
>>> `make V=1`.
>>
>> I agree.
>>
>>> But if it makes everyone happy, I don't mind if we split the rule in
>>> two.  Smaller and simpler rules are easier to understand.
>>
>> I didn't really want to split the rule as the .pod really is an
>> intermediate step:
>>
>> 	$(SILENCE) touch $@
>> 	-$(ECHO_TEXI2POD) $(TEXI2POD) $(MANCONF) -Dgdb < $(srcdir)/gdb.texinfo > gdb.pod
>> 	-$(ECHO_TEXI2MAN) ($(POD2MAN1) gdb.pod | sed -e '/^.if n .na/d' > $@.T$$$$ && \
>> 		mv -f $@.T$$$$ $@) || (rm -f $@.T$$$$ && exit 1)
>> 	$(SILENCE) rm -f gdb.pod
>>
>> We create the .pod and then consume it, before finally deleting it.
>>
>> If the rule was split then we'd end up creating the .pod in one rule
>> before deleting it in another, which didn't seem great.  But if you're
>> happy with that change then I can split the rule.

We could maybe just not remove the intermediary pod files (but remove
them in the clean target, of course)?  They probably don't take that
much space in the build.

I think your version with the split rules is fine, it fits well the
make model.

> 
> Below is a reworked patch which splits the man page and .pod creation.
> 
> Let me know what you think.
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 1b0c9eb959212f175092da0e200811ba47f2000f
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Fri Apr 12 17:47:20 2024 +0100
> 
>     gdb/doc: use silent-rules.mk in the Makefile
>     
>     Make use of silent-rules.mk when building the GDB docs.
>     
>     During review it was requested that there be more specific rules than
>     just reusing the general 'GEN' rule everywhere in the doc/ directory,
>     so I've added:
>     
>       ECHO_DVIPS =    @echo "  DVIPS    $@";
>       ECHO_TEX =      @echo "  TEX      $@";
>       ECHO_PDFTEX =   @echo "  PDFTEX   $@";
>       ECHO_TEXI2DVI = @echo "  TEXI2DVI $@";
>       ECHO_MAKEHTML = @echo "  MAKEHTML $@";
>       ECHO_TEXI2POD = @echo "  TEXI2POD $@";
>       ECHO_TEXI2MAN = @echo "  TEXI2MAN $@";
>       ECHO_MAKEINFO = @echo "  MAKEINFO $@";
>     
>     Then I've made use of these new silent rules and added lots of uses of
>     SILENT to reduce additional clutter.
>     
>     As the man page generation is done in two phases, first the creation
>     of a .pod file, then the creation of the final man page file, I've
>     restructured the man page rules.  Previously we had one rule for each
>     of the 5 man pages.  I now have one general rule that will generate
>     all of the 5 .pod files, then I have two rules that convert the .pod
>     files into the final man pages.
>     
>     I needed two rules for the man page generation as some man pages match
>     %.1 and some match %.5.  I could combine these by using the GNU Make
>     .SECONDARYEXPANSION extension, but I'm not sure if we are OK to depend
>     on GNU only Make features, and having the two separate rules seems
>     clear enough.

We do require GNU make already:

  https://sourceware.org/gdb/current/onlinedocs/gdb.html/Requirements.html#Requirements

But I have to admit that a using things like .SECONDARYEXPANSION is
above my make skill level :).

>     
>     I've also added a new SILENT_QUIET_FLAG to silent-rules.mk, this is
>     like SILENT_FLAG, but is set to '-q' when in silent mode, this can be
>     used with the 'dvips' and 'texi2dvi' commands, both of which use '-q'
>     to mean: only report errors.

Maybe name this one SILENT_Q_FLAG?  SILENT_QUIET_FLAG would be for
"--quiet" or "-quiet".

>     As with the rest of the GDB makefiles, I've only converted the
>     "generation" rules to use silent-rules.mk, the install / uninstall
>     rules are left unchanged.
>     
>     There are still a few "generation" targets that produce output, there
>     seems to be no flag to silence the 'tex' and 'pdftex' commands which
>     some recipes use, I've not worried about these for now, e.g. the
>     refcard.dvi and refcard.pdf targets still produce some output.

Not a big deal IMO.  If we really want to, we could pipe stdout to
/dev/null I suppose.

>     Luckily, when doing a 'make all' in the gdb/ directory, we only build
>     the info docs by default, and those rules are now nice and silent, so
>     a complete GDB build is now looking nice and quiet by default.
> 
> diff --git a/gdb/doc/Makefile.in b/gdb/doc/Makefile.in
> index 8007f6373d4..ed8f7be5227 100644
> --- a/gdb/doc/Makefile.in
> +++ b/gdb/doc/Makefile.in
> @@ -33,6 +33,8 @@ man5dir = $(mandir)/man5
>  
>  transform = @program_transform_name@
>  
> +include $(srcdir)/../silent-rules.mk
> +
>  SHELL = @SHELL@
>  
>  LN_S = @LN_S@
> @@ -238,7 +240,7 @@ Doxyfile-gdbserver:	$(srcdir)/Doxyfile-gdbserver.in
>  
>  all-doc: info dvi ps pdf
>  diststuff: info man
> -	rm -f gdb-cfg.texi
> +	$(SILENCE) rm -f gdb-cfg.texi

This makes this rule completely silent, that's maybe too much?  In the
clean target, we don't silence the `rm`s, so I don't think we should
here either.

> -gdb-add-index.1: $(GDB_DOC_FILES)
> -	touch $@
> -	-$(TEXI2POD) $(MANCONF) -Dgdb-add-index < $(srcdir)/gdb.texinfo > gdb-add-index.pod
> -	-($(POD2MAN1) gdb-add-index.pod | sed -e '/^.if n .na/d' > $@.T$$$$ && \
> +$(MAN1S) : %.1 : %.pod $(GDB_DOC_FILES)
> +	$(SILENCE) touch $@
> +	$(ECHO_TEXI2MAN) ($(POD2MAN1) $*.pod | sed -e '/^.if n .na/d' > $@.T$$$$ && \
>  		mv -f $@.T$$$$ $@) || (rm -f $@.T$$$$ && exit 1)
> -	rm -f gdb-add-index.pod
> +	$(SILENCE) rm -f $*.pod

So as I said above, I would just not remove the pod file.

A question about the pre-existing code: do you know why the `touch @` is
needed?  It just seems dangerous to me.  If make is interrupted after
the `touch` but before the next command, the destination file will exist
but be empty.  A subsequent "make" will not rebuild it, saying the file
is already up-to-date (when it's not).

Simon