Re: [PATCH 00/15] Add libunistring-optional module

[Bruno Haible]

Hi Paolo,

> This series adds a libunistring-optional module.  The purpose of the
> module is to allow using a system libunistring whenever present, while
> leaving the source code in the package too for the case when it is absent.
> 
> The obvious step would be to make this the default.  Unfortunately,
> this is hard because of two combined factors: 1) libunistring-optional
> requires changing the package (to add INCUNISTRING and LIBUNISTRING
> appropriately), and 2) via mbiter, any package that requires wchar.h
> replacements will require uniwidth too.
> 
> The first four patches implement the infrastructure, everything else is
> just search-and-replace.

Thanks for doing that! It is simpler than the solution that I had imagined
for the use in gettext, and requires nearly no additional change for the
user: He adds 'gl_LIBUNISTRING' to his configure.ac and is done with it.

Also, I appreciate that it stays the developer's decision whether he wants
to use libunistring as a library when it is present or not. When only
the uniwidth module or only some minimal UTF-8 <--> UCS-4 conversion is
needed, it'd be overkill to link against the library. But when a program
uses the entire line breaking or (planned) regular expression stuff,
linking against libunistring.so will be the preferred way of packaging.

However, I disagree with some of your patches.


1) In part 04:

>   
> AC_CONFIG_LINKS(gl_GNULIB_SOURCE_BASE[/$1.h]:gl_GNULIB_SOURCE_BASE[/$1.in.h])

This line does two things wrong:

  - It uses AC_CONFIG_LINKS. Don't use AC_CONFIG_LINKS. AC_CONFIG_LINKS has
    the big drawback that its effects persist after the user has done
    "make clean". Before reconfiguring with different parameters, users
    ought to do a "make distclean". But often they only do "make clean"
    before re-running configure. That was the reason why the creation of
    a config.cache file is no longer the default in Autoconf. Now,
    users have the habit of doing "make clean; ./configure ...other 
parameters..."
    and it works. EXCEPT for symbolic links created by AC_CONFIG_LINKS.

    It would be good if Autoconf deprecated AC_CONFIG_LINKS altogether.

  - It creates objects on the file system. Experience of the first three years
    of gnulib development has (painfully) showed that this is a bad thing to do.
    The rule is to put everything that needs to know about file names and
    directory names into Makefile.am snippets. configure.ac is the right place
    for determining platform characteristics, and Makefile.am is the right
    place for creating files.

    If you don't do this, there are multiple bad consequences:
      - "make clean" does not remove created files, mentioned above.
      - It cannot work with multiple invocations of gnulib-tool in the
        realm of the same configure.ac files. Because you attempt to
        encode gnulib's --source-base value in an autoconf macro, and
        that cannot work: During the execution of the gl_INIT macro
        the value needs to be a different one than during the execution
        of the gltests_INIT macro.
      - You end up have two mechanisms for doing the same thing,
        and have trouble maintaining the consistency betweem both.

    The absolute minimum of use of file names in configure.ac is:
      - Use of AC_LIBOBJ. It has required some pushdef/popdef hacks in
        gnulib-tool to make it work.
      - When a test program needs to #include a large piece of C code,
        like the 'getloadavg' module does, use of $gl_source_base may
        be inevitable. This too has necessitated a hack in gnulib-tool.

2) Part 01.

It attempts to encapsulate the values of gnulib-tool parameters in autoconf
macros. It cannot work, because they may be multiple gnulib-tool invocations
in the scope of a single configure.ac.

Drop this part.

3) Part 04.

The gl_LIBUNISTRING_LIBOBJ is fine. Nice invention. But it should test
the variable HAVE_LIBUNISTRING, not the variable ac_cv_libunistring.
See the documentation in m4/libunistring.m4: ac_cv_libunistring is not
documented there.

The other two macros gl_CONFIG_LINKS and gl_LIBUNISTRING_HEADER ought
to go. That belongs in Makefile.am sections of the module descriptions,
as explained above.

4) Part 04.

There is a constraint that gl_LIBUNISTRING needs to be executed before
gl_LIBUNISTRING_LIBOBJ. Therefore a line
  AC_BEFORE([$0], [gl_LIBUNISTRING_LIBOBJ])
should be added to m4/libunistring.m4.

5) In part 03 and part 04:

What's the rationale for changing the not-found message from
"no, consider installing GNU libunistring" to "no"? If the developer
has determined that his programs would benefits from an installed,
shared libunistring.so, then the message "no, consider installing
GNU libunistring" is still appropriate, even if the dependency is an
optional one.

So, IMO, there's no need to make this message customizable.

6) Part 05..15:

Should create the .h files in a Makefile.in section. Here's a template:

--------------------------- Makefile.am snippet -------------------
if INCLUDED_LIBUNISTRING
BUILT_SOURCES += unitypes.h
endif

unitypes.h: unitypes.in.h
        { echo '/* DO NOT EDIT! GENERATED AUTOMATICALLY! */'; \
          cat $(srcdir)/unitypes.in.h; \
        } > address@hidden
        mv -f address@hidden $@

MOSTLYCLEANFILES += unitypes.h unitypes.h-t
--------------------------------------------------------------------

where INCLUDED_LIBUNISTRING is an automake conditional that needs to be
defined somewhere.

This idiom makes sure that
  - symlinks with their portability problems are avoided altogether,
  - generated .h files carry a comment that you should not edit them (done
    everywhere in gnulib),
  - the .h file gets erased by "make clean", even if the user reconfigured
    with different parameters in between.

The definition of INCLUDED_LIBUNISTRING can look like this (code cribbed
from gettext/gnulib-local/m4/libxml.m4):

  AC_MSG_CHECKING([whether included libunistring is requested])
  AC_ARG_WITH([included-libunistring],
    [  --with-included-libunistring  use the libunistring parts included here],
    [gl_cv_libunistring_force_included=$withval],
    [gl_cv_libunistring_force_included=no])
  AC_MSG_RESULT([$gl_cv_libunistring_force_included])

  gl_cv_libunistring_use_included="$gl_cv_libunistring_force_included"
  LIBUNISTRING=
  LTLIBUNISTRING=
  INCUNISTRING=
  if test "$gl_cv_libunistring_use_included" != yes; then
    ... use gl_LIBUNISTRING ...
  fi
  AC_SUBST([LIBUNISTRING])
  AC_SUBST([LTLIBUNISTRING])
  AC_SUBST([INCUNISTRING])
  AC_MSG_CHECKING([whether to use the included libunistring])
  AC_MSG_RESULT([$gl_cv_libunistring_use_included])

The rest of these parts (move the source files from Makefile.am section
to configure.ac section) is fine, since it uses AC_LIBOBJ.

7) Part 05..15:

The changes to the "Files" section of modules/uni*/* files, except
modules/uni*/base files, are IMO unintentional and should be undone.
For example:
  - Part 06, modules/unicase/locale-language: Why replace
    lib/unicase/locale-languages.gperf with
    lib/unicase/locale-languages.gperf.c?? That file does not exist.
  - Part 08, modules/unictype/category-Cs: Why replace
    lib/unictype/categ_Cs.c with lib/unictype/categ_Cs?? That file does not
    exist.
There may be more like this. I haven't checked it all.


I'll comment about part 02 and part 03 separately.

Bruno