Re: Support --with-pkgversion and --with-bugurl

[Roland McGrath <roland at hack dot frob dot com>]

The thrust is fine, but there are some nits.

Seeing all the boilerplate changes to argp-using tools makes it clear
that we really ought to factor out some of the common stuff into a
shared place.  I think doing that before this change would be nice,
and its success would be demonstrated by the new version of this
change touching many fewer files.  So that's my strong preference.
But it's up to you whether you want to do that separate cleanup first.

> +/* Additional package description.  */
> +#undef PKGVERSION

It's not really "additional" if it replaces "GNU libc".

> diff --git a/elf/pldd.c b/elf/pldd.c
> index 39095c9..a8e2e5c 100644
> --- a/elf/pldd.c
> +++ b/elf/pldd.c
> @@ -52,10 +52,8 @@ extern void *xrealloc (void *o, size_t n)
>  static void print_version (FILE *stream, struct argp_state *state);
>  void (*argp_program_version_hook) (FILE *, struct argp_state *) = print_version;
>  
> -/* Bug report address.  */
> -const char *argp_program_bug_address = N_("\
> -For bug reporting instructions, please see:\n\
> -<http://www.gnu.org/software/libc/bugs.html>.\n");
> +/* Function to print some extra text in the help message.  */
> +static char *more_help (int key, const char *text, void *input);

Why is it necessary to change this from argp_program_bug_address to
more_help?  What's the difference in output between the two?

> diff --git a/locale/programs/localedef.c b/locale/programs/localedef.c
> index 5dc35ca..68a2377 100644
> --- a/locale/programs/localedef.c
> +++ b/locale/programs/localedef.c
> @@ -354,19 +354,22 @@ static char *
>  more_help (int key, const char *text, void *input)
>  {
>    char *cp;
> +  char *tp;
>  
>    switch (key)
>      {
>      case ARGP_KEY_HELP_EXTRA:
>        /* We print some extra information.  */
> +      if (asprintf (&tp, gettext ("\
> +For bug reporting instructions, please see:\n\
> +%s.\n"), REPORT_BUGS_TO) < 0)
> +	return NULL;
>        if (asprintf (&cp, gettext ("\
>  System's directory for character maps : %s\n\
>  		       repertoire maps: %s\n\
>  		       locale path    : %s\n\
>  %s"),
> -		    CHARMAP_PATH, REPERTOIREMAP_PATH, LOCALE_PATH, gettext ("\
> -For bug reporting instructions, please see:\n\
> -<http://www.gnu.org/software/libc/bugs.html>.\n")) < 0)
> +		    CHARMAP_PATH, REPERTOIREMAP_PATH, LOCALE_PATH, tp) < 0)
>  	return NULL;
>        return cp;

Leaks TP.

> --- a/login/programs/pt_chown.c
> +++ b/login/programs/pt_chown.c

Same here.

> --- a/manual/Makefile
> +++ b/manual/Makefile
> @@ -113,6 +113,14 @@ $(objpfx)stamp-libm-err: libm-err-tab.pl $(wildcard $(foreach dir,$(sysdirs),\
>  	$(move-if-change) $(objpfx)libm-err-tmp $(objpfx)libm-err.texi
>  	touch $@
>  
> +# Package version and bug reporting URL.
> +$(objpfx)pkgvers.texi: $(objpfx)stamp-pkgvers

Needs a ; for empty commands.

> +$(objpfx)stamp-pkgvers:
> +	echo "@set PKGVERSION $(PKGVERSION_TEXI)" > $(objpfx)pkgvers-tmp
> +	echo "@set REPORT_BUGS_TO $(REPORT_BUGS_TEXI)" >> $(objpfx)pkgvers-tmp
> +	$(move-if-change) $(objpfx)pkgvers-tmp $(objpfx)pkgvers.texi
> +	touch $@

Depend on config.make.

> --- a/manual/install.texi
> +++ b/manual/install.texi
> @@ -2,6 +2,9 @@
>  @c Makeinfo ignores it when processing the file from the include.
>  @setfilename INSTALL
>  @include macros.texi
> +@ifclear REPORT_BUGS_TO
> +@set REPORT_BUGS_TO @uref{http://www.gnu.org/software/libc/bugs.html}
> +@end ifclear

Why bake a default in here?  I presume it's just for the INSTALL
formatting.  It would be better to use pkgvers.texi here too, with
whatever macro fiddles are easiest to either avoid or make harmless
the multiple inclusion.

>  It is a good idea to verify that the problem has not already been
>  reported.  Bugs are documented in two places: The file @file{BUGS}
> -describes a number of well known bugs and the bug tracking system has a
> +describes a number of well known bugs and the upstream @glibcadj{}
> +bug tracking system has a

"upstream" is not an appropriate term to use within canonical GNU
documentation.  If you need an adjective, you can use "central" or
something like that.

> @@ -36,7 +38,8 @@ This file documents @theglibc{}.
>  This is
>  @c Disabled (printed editions, see above).
>  @c Edition @value{EDITION} of
> -@cite{The GNU C Library Reference Manual}, for version @value{VERSION}.
> +@cite{The GNU C Library Reference Manual}, for version @value{VERSION}
> +@value{PKGVERSION}.

In default configuration this is going to say, "for version 2.17
(GNU libc) ".  That's not what we want.  All the instances in the
manual should be omitted entirely if it's the default setting.

> --- a/sunrpc/rpc_main.c
> +++ b/sunrpc/rpc_main.c
> @@ -1439,15 +1439,15 @@ options_usage (FILE *stream, int status)
>    f_print (stream, _("-T\t\tgenerate code to support RPC dispatch tables\n"));
>    f_print (stream, _("-Y path\t\tdirectory name to find C preprocessor (cpp)\n"));
>  
> -  f_print (stream, "\n%s", _("\
> +  f_print (stream, "\n\
>  For bug reporting instructions, please see:\n\
> -<http://www.gnu.org/software/libc/bugs.html>.\n"));
> +%s.\n", REPORT_BUGS_TO);
>    exit (status);
>  }

Why are you dropping the translation of this string?

> --- a/timezone/Makefile
> +++ b/timezone/Makefile
> @@ -107,6 +107,8 @@ $(testdata)/Asia/Tokyo: asia $(zic-deps)
>  
>  $(objpfx)tzselect: tzselect.ksh $(common-objpfx)config.make
>  	sed -e 's%@KSH@%$(KSH)%g' \
> -	    -e 's%@TZDIR@%$(zonedir)%g' < $< > $@.new
> +	    -e 's%@TZDIR@%$(zonedir)%g' \
> +	    -e 's%@PKGVERSION@%$(PKGVERSION)%g' \
> +	    -e 's%@REPORT_BUGS_TO@%$(REPORT_BUGS_TO)%g' < $< > $@.new

We don't know what characters people might include in these strings.
% doesn't seem all that unlikely.  We should be consistent in all
the sed invocations so they exclude only one particular character.
| is used elsewhere and that seems like the best choice off hand.

> --- a/timezone/tzselect.ksh
> +++ b/timezone/tzselect.ksh

This script is barely changed from tzcode.
We should try to reharmonize before we diverge further.


Thanks,
Roland