Re: AM_SILENT_RULES doesn't silence texinfo rules

[Jack Kelly]

Hi Ralf,

On Fri, Sep 25, 2009 at 5:02 AM, Ralf Wildenhues <address@hidden> wrote:
> Hi Jack,
>
> * Jack Kelly wrote on Wed, Sep 23, 2009 at 11:30:54AM CEST:
>> On Wed, Sep 23, 2009 at 2:02 PM, Ralf Wildenhues wrote:
>> > Uh, oh, more bikeshed color questions.  Let's try to find answers that
>> > follow some principle ...  ;-)
>> >
>> > -snip-
>> >
>> > What if you just give up the alignment of the target for tags longer
>> > than 6 characters?  Too ugly shed color?  Or we go to 8, that might
>> > still be tolerable, maybe let's see an example build.
>>
>> Sure. I added silent rules to my libfake437 Makefile.am and built it:
>
> [...]
>> If I drop it back to 6, it looks like this:
>
>> I personally prefer it with 8, but I'd rather get something I don't
>> like committed over a long bikeshed argument.
>
> Thanks.  8 is fine with me, too, after checking.
>
>> Revised patch attached. If you're happy with it, I'll cook up some
>> tests and we can try to get this committed soonish.
>
> Nits inline.  Nothing big.
>
>> --- a/automake.in
>> +++ b/automake.in
>> @@ -1199,11 +1199,25 @@ sub define_verbose_tagvar ($)
>>      my ($name) = @_;
>>      if (option 'silent-rules')
>>        {
>> -     define_verbose_var ($name, '@echo "  '. $name . ' ' x (6 - length 
>> ($name)) . '" $@;');
>> +     define_verbose_var ($name, '@echo "  '. $name . ' ' x (8 - length 
>> ($name)) . '" $@;');
>>       define_verbose_var ('at', '@');
>>        }
>>  }
>>
>> +# define_verbose_texinfo
>> +# ----------------------
>> +# Engage the needed `silent-rules' machinery for assorted texinfo commands.
>> +sub define_verbose_texinfo ()
>> +{
>
> Can we make the contents of this function optional to 'silent-rules'?

Done, but it doesn't make a difference. Everything it calls is a no-op
if silent-rules is off.

>
>> +  my @tagvars = ('DVIPS', 'MAKEINFO', 'INFOHTML', 'TEXI2DVI', 'TEXI2PDF');
>> +  foreach my $tag (@tagvars)
>> +    {
>> +      define_verbose_tagvar($tag);
>> +    }
>> +  define_verbose_var('texinfo', '-q');
>> +  define_verbose_var('texidevnull', '> /dev/null');
>> +}
>> +
>>  # define_verbose_libtool
>>  # ----------------------
>>  # Engage the needed `silent-rules' machinery for `libtool --silent'.
>> @@ -3248,6 +3262,8 @@ sub output_texinfo_build_rules ($$$@)
>>                                                      ? '$<' : $source),
>>                                 SOURCE_REAL      => $source,
>>                                 SOURCE_SUFFIX    => $ssfx,
>> +                                  TEXIQUIET        => 
>> verbose_flag('texinfo'),
>> +                                  TEXIDEVNULL      => 
>> verbose_flag('texidevnull'),
>>                                 );
>>    return ($dirstamp, "$dpfx.dvi", "$dpfx.pdf", "$dpfx.ps", "$dpfx.html");
>>  }
>> @@ -3551,6 +3567,7 @@ sub handle_texinfo ()
>>    reject_var 'TEXINFOS', "`TEXINFOS' is an anachronism; use 
>> `info_TEXINFOS'";
>>    # FIXME: I think this is an obsolete future feature name.
>>    reject_var 'html_TEXINFOS', "HTML generation not yet supported";
>> +  define_verbose_texinfo;
>
> Can we make this call optional to makefile files that contain texinfos?

Done.

> Otherwise, you end up with >20 unneeded lines in Makefile.in files.
> (Try adding silent-rules to the Automake package itself, ./bootstrap,
> and git diff.)

Didn't happen for me. Perhaps you're thinking of older code?

>>    my $info_texinfos = var ('info_TEXINFOS');
>>    my ($mostlyclean, $clean, $maintclean) = ('', '', '');
>> @@ -3567,7 +3584,8 @@ sub handle_texinfo ()
>>                                  MOSTLYCLEAN   => $mostlyclean,
>>                                  TEXICLEAN     => $clean,
>>                                  MAINTCLEAN    => $maintclean,
>> -                                'LOCAL-TEXIS' => !!$info_texinfos);
>> +                                'LOCAL-TEXIS' => !!$info_texinfos,
>> +                                   TEXIQUIET     => 
>> verbose_flag('texinfo'));
>>  }
>
>> --- a/lib/am/texibuild.am
>> +++ b/lib/am/texibuild.am
>> @@ -32,7 +32,8 @@
>>  ##    to fail, the info files are not removed.  (They are needed by the
>>  ##    developer while he writes documentation.)
>>  ## *.iNN files are used on DJGPP.  See the comments in install-info-am
>> -     restore=: && backupdir="$(am__leading_dot)am$$$$" && \
>> +     $(AM_V_MAKEINFO)
>> +     $(AM_V_at)restore=: && backupdir="$(am__leading_dot)am$$$$" && \
>
> Two issues here (several instances below):
>
> Completely empty lines in make rules are not portable (they provoke a
> warning from some make implementation), which is what $(AM_V_MAKEINFO)
> will expand to if silent-rules is disabled or V=1.  Instead, you can
> replace the first AM_V_at with AM_V_MAKEINFO and remove the preceding line
> (likewise, in the other instances, replace the first AM_V_at in a recipe).
>
> Second, so far I have been a bit keen on keeping Makefile.in files
> unchanged when 'silent-rules' was not used at all.  Arguably, this
> was mostly important for depend2.am, which can get expanded very
> often in large packages, leading to visibly larger file size.  I'm
> not so sure whether that is all that important for texinfo rules
> which are used less often, and after all, AM_V_* is in our namespace,
> so we don't need to fear that it's used for other purposes already.
>
> Anyway, the way to avoid this would be to replace $(AM_V_MAKEINFO)
> with %VERBOSE%, and any remaining instances of $(AM_V_at) with %SILENT%,
> and substituting those two in the respective %transform in automake.in,
> only if option 'silent-rules' is used, empty otherwise.

Done.

>>  ?INSRC?      am__cwd=`pwd` && $(am__cd) $(srcdir) && \
>>       rm -rf $$backupdir && mkdir $$backupdir && \
>>  ## If makeinfo is not installed we must not backup the files so
>> @@ -62,23 +63,29 @@ INFO_DEPS += %DEST_INFO_PREFIX%%DEST_SUFFIX%
>>
>>  ?GENERIC?%SOURCE_SUFFIX%.dvi:
>>  ?!GENERIC?%DEST_PREFIX%.dvi: %SOURCE% %DEPS% %DIRSTAMP%
>> -     TEXINPUTS="$(am__TEXINFO_TEX_DIR)$(PATH_SEPARATOR)$$TEXINPUTS" \
>> +     $(AM_V_TEXI2DVI)
>> +     
>> $(AM_V_at)TEXINPUTS="$(am__TEXINFO_TEX_DIR)$(PATH_SEPARATOR)$$TEXINPUTS" \
>>  ## Must set MAKEINFO like this so that version.texi will be found even
>>  ## if it is in srcdir (-I $(srcdir) is set in %MAKEINFOFLAGS%).
>>       MAKEINFO='$(MAKEINFO) $(AM_MAKEINFOFLAGS) $(MAKEINFOFLAGS) 
>> %MAKEINFOFLAGS%' \
>>  ## Do not use `-o' unless necessary: it is only supported since Texinfo 4.1.
>> -?GENERIC?    $(TEXI2DVI) %SOURCE%
>> -?!GENERIC?   $(TEXI2DVI) -o $@ `test -f '%SOURCE%' || echo 
>> '$(srcdir)/'`%SOURCE%
>> +## texi2dvi doesn't silence everything with -q, redirect to /dev/null 
>> instead.
>> +## We still want -q (%TEXIQUIET%) because it turns on batch mode.
>> +?GENERIC?    $(TEXI2DVI) %TEXIQUIET% %SOURCE% %TEXIDEVNULL%
>> +?!GENERIC?   $(TEXI2DVI) %TEXIQUIET% -o $@ `test -f '%SOURCE%' || echo 
>> '$(srcdir)/'`%SOURCE% %TEXIDEVNULL%
>
> This leads to trailing spaces in the non-silent-rules case.  Can we add
> a leading space to var('texidevnull') and remove the one before
> %TEXIDEVNULL% here (again, several instances)?
> (Don't bother with it if the solution is harder than that.)

Done.

>> -?GENERIC?    $(TEXI2PDF) %SOURCE%
>> -?!GENERIC?   $(TEXI2PDF) -o $@ `test -f '%SOURCE%' || echo 
>> '$(srcdir)/'`%SOURCE%
>> +## texi2pdf doesn't silence everything with -q, redirect to /dev/null 
>> instead.
>> +## We still want -q (%TEXIQUIET%) because it turns on batch mode.
>
> Thanks for documenting this!
>
>> +?GENERIC?    $(TEXI2PDF) %TEXIQUIET% %SOURCE% %TEXIDEVNULL%
>> +?!GENERIC?   $(TEXI2PDF) %TEXIQUIET% -o $@ `test -f '%SOURCE%' || echo 
>> '$(srcdir)/'`%SOURCE% %TEXIDEVNULL%
>>
>>  ?GENERIC?%SOURCE_SUFFIX%.html:
>>  ?!GENERIC?%DEST_PREFIX%.html: %SOURCE% %DEPS% %DIRSTAMP%
>
> Thanks,
> Ralf
>

I have also added a test: tests/silent8.test. Lastly, I changed the
tagline for the html target to MAKEINFO not INFOHTML, because MAKEINFO
is the name of the program. Besides, seeing `MAKEINFO foo.html' makes
it fairly clear what is going on. I can change this if you want.

-- Jack

0001-Add-silent-rules-support-for-texinfo-outputs.patch
Description: Text Data