Re: [Groff] [groff] 01/01: New groff preprocessor for Perl parts in groff files.

[Ingo Schwarze]

Hi Bernd,

sorry for noticing this late, see one comment inline...

Bernd Warken wrote on Wed, Feb 26, 2014 at 01:33:31AM +0000:

> bwarken pushed a commit to branch master
> in repository groff.
> 
> commit 51476bee112778bd314d1a4c7a27fcd670434167
> Author: Bernd Warken <address@hidden>
> Date:   Wed Feb 26 02:33:05 2014 +0100
> 
>     New groff preprocessor for Perl parts in groff files.
> ---
>  ChangeLog                  |    4 +
>  Makefile.in                |   27 ++--
>  contrib/gperl/ChangeLog    |   24 ++++
>  contrib/gperl/Makefile.sub |   64 +++++++++
>  contrib/gperl/gperl.man    |  310 
> ++++++++++++++++++++++++++++++++++++++++++++
>  contrib/gperl/gperl.pl     |  222 +++++++++++++++++++++++++++++++
>  6 files changed, 638 insertions(+), 13 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 4f26218..75dc435 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,7 @@
> +2014-02-25  Bernd Warken  <address@hidden>
> +
> +     * contrib/gperl: New preprocessor for Perl parts in groff files.
> +
>  2014-02-15  Ingo Schwarze  <address@hidden>
>  
>       * tmac/groff_mdoc.man: Improve the manual page template.
> diff --git a/Makefile.in b/Makefile.in
> index a7f1b64..763516a 100644
> --- a/Makefile.in
> +++ b/Makefile.in
> @@ -2,7 +2,7 @@
>  #   Free Software Foundation, Inc.
>  #      Written by James Clark (address@hidden)
>  #
> -# Last update: 30 Apr 2013
> +# Last update: 25 Feb 2014
>  # 
>  # This file is part of groff.
>  # 
> @@ -624,25 +624,26 @@ ALLTTYDEVDIRS=\
>  # `doc' must be processed before `contrib/pdfmark',
>  # pdf stuff must be processed before `contrib/mom
>  OTHERDIRS=\
> -  man \
> -  tmac \
> -  src/utils/afmtodit \
> -  src/roff/grog \
> -  src/roff/nroff \
> -  doc \
> -  contrib/groff_filenames \
> -  contrib/mm \
>    contrib/chem \
> -  contrib/pic2graph \
>    contrib/eqn2graph \
> +  contrib/gdiffmk \
> +  contrib/glilypond \
> +  contrib/gperl \
>    contrib/grap2graph \
> +  contrib/groff_filenames \
>    contrib/groffer \
> -  contrib/glilypond \
>    contrib/hdtbl \
> +  contrib/mm \
>    contrib/pdfmark \
> -  font/devpdf \
> +  contrib/pic2graph \
>    contrib/mom \
> -  contrib/gdiffmk
> +  doc \
> +  font/devpdf \
> +  man \
> +  src/utils/afmtodit \
> +  src/roff/grog \
> +  src/roff/nroff \
> +  tmac
>  
>  # OTHERDIRS is handled specially in the `$(TARGETS)' rule to avoid
>  # dependency problems with parallel builds.

I suspect this reordering might be a bad idea.  I realize you are
trying to make this nicer by ordering it alphabetically, but i don't
think the order is arbitrary.  I certainly remember dependency and
ordering issues when porting groff in the past.

In particular, the comment right at the top of your chunk says:

  # `doc' must be processed before `contrib/pdfmark',
  # pdf stuff must be processed before `contrib/mom

Your patch breaks both of those requirements.  Can you re-check
that this doesn't introduce ordering and dependency issues,
also in parallel builds, and that the build still works even
without groff installed?  If it turns out this works reliably,
which would surprise me a bit, then the comment should probably
be corrected.

Yours,
  Ingo