Re: [PATCH] Make own set of gnulib modules for libutils

[Tim Rühsen]

See my comments inline.

On 05.05.20 13:38, Jose E. Marchesi wrote:
> 
> Hi Tim.
> 
> I like the approach, but see a couple of comments below.
> 
>     2020-05-05  Tim Rühsen  <address@hidden>
>     
>             * Makefile.am: Add libutils/gl to SUBDIRS.
>             * bootstrap.conf: Invoke gnulib-tool to generate libutils/gl.
>             * configure.ac: Add libutils/gl/m4 to AC_CONFIG_MACRO_DIRS.
>             Add libutils_EARLY.
>             Add libutils/gl/Makefile to AC_CONFIG_FILES.
>             * libutils/Makefile.am: Use gl/libgnu.la in libutils_la_LIBADD.
>     ---
>      ChangeLog            |  9 +++++++++
>      Makefile.am          |  2 +-
>      bootstrap.conf       | 16 +++++++++++++++-
>      configure.ac         |  4 +++-
>      libutils/Makefile.am |  2 +-
>      5 files changed, 29 insertions(+), 4 deletions(-)
>     
>     diff --git a/Makefile.am b/Makefile.am
>     index a59b5ef1..1cc2ff9c 100644
>     --- a/Makefile.am
>     +++ b/Makefile.am
>     @@ -1,5 +1,5 @@
>      ACLOCAL_AMFLAGS = -I m4
>     -SUBDIRS = jitter gl pickles libutils libpoke poke doc man testsuite etc 
> po
>     +SUBDIRS = jitter gl pickles libutils/gl libutils libpoke poke doc man 
> testsuite etc po
>     
> I would prefer to use the same strategy than for the "main" gnulib,
> i.e. to have it in the top-level.  We can put it in a directory
> gl-libutils/.

Sure, it's your project.
My reason for putting it into libutils/gl (and libutils/gl/m4) is
self-containment. Not spreading the things that only libutils/ is using
somewhere around (if possible).

> 
>      noinst_SCRIPTS = run
>     
>     diff --git a/bootstrap.conf b/bootstrap.conf
>     index fdf0183d..c1768a26 100644
>     --- a/bootstrap.conf
>     +++ b/bootstrap.conf
>     @@ -16,7 +16,7 @@
>      # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>     
>     
>     -# gnulib modules used by this package.
>     +# gnulib modules used for this project and in poke/.
> 
> 
> Thats a bit confusing.  I would say something like:
> "gnulib modules used by libpoke and poke."

I think modules like update-copyright are in their own class - that's
why I mentioned 'this project'. Not sure how we can split that up in a
better way.

> 
>      gnulib_modules="
>        accept
>        array-list
>     @@ -57,6 +57,17 @@ gnulib_modules="
>        xstrndup
>        "
>     
>     +# gnulib modules used in libutils/.
>     +libutils_modules="
>     +  gettext-h
>     +  snprintf
>     +  stat
>     +  stdarg
>     +  stddef
>     +  strerror
>     +  strstr
>     +  "
>     +
>      # TODO: Remove this after the 1.0 release. Until then, this helps 
> developers
>      # not have to type --skip-po on every bootstrap call
>      SKIP_PO=t
>     @@ -122,6 +133,9 @@ makeinfo    6.0
>      # Poke's configure.ac requires the Autoconf macros to be copied from 
> Jitter.
>      bootstrap_post_import_hook ()
>      {
>     +  # create
> 
> Incomplete comment?

Ups.

> 
>     +  ${GNULIB_SRCDIR}/gnulib-tool --import --lib=libgnu 
> --source-base=libutils/gl --m4-base=libutils/gl/m4 --doc-base=doc 
> --aux-dir=build-aux --lgpl=3 --no-conditional-dependencies --libtool 
> --without-tests --macro-prefix=libutils ${libutils_modules}
>     +
> 
> Why not using the main m4/ directory for libutil?  There is only one
> configure.ac script in the project.

Also self-containment and bit of being unsure if that is technically the
same. But yeah, I will do.

> 
>        echo 'Updating the Jitter submodule'
>        git submodule update --init -- ./jitter
>     
>     diff --git a/configure.ac b/configure.ac
>     index 5a135eec..8db0ba5c 100644
>     --- a/configure.ac
>     +++ b/configure.ac
>     @@ -24,7 +24,7 @@ AC_INIT([GNU poke], [0.1-beta], [address@hidden], 
> [poke],
>      AC_CONFIG_AUX_DIR([build-aux])
>      AM_INIT_AUTOMAKE
>      AC_CONFIG_HEADERS(poke/config.h)
>     -AC_CONFIG_MACRO_DIR([m4])
>     +AC_CONFIG_MACRO_DIRS([m4 libutils/gl/m4])
>     
> Ditto, regarding the m4 directory.
> 
>      # Include the Autoconf macros from Jitter.
>      m4_include([m4/jitter.m4])
>     @@ -35,6 +35,7 @@ dnl AB_INIT
>     
>      AC_PROG_CC
>      gl_EARLY
>     +libutils_EARLY
>     
>      LT_INIT
>      AC_PROG_CC_C99
>     @@ -128,6 +129,7 @@ fi
>      dnl Generate output files
>      AC_CONFIG_FILES(Makefile
>                      gl/Makefile
>     +                libutils/gl/Makefile
>                      libutils/Makefile
>                      libpoke/Makefile
>                      poke/Makefile
>     diff --git a/libutils/Makefile.am b/libutils/Makefile.am
>     index 675025b4..434e9729 100644
>     --- a/libutils/Makefile.am
>     +++ b/libutils/Makefile.am
>     @@ -26,7 +26,7 @@ libutils_la_SOURCES = pk-utils.c pk-utils.h
>      libutils_la_CFLAGS = -Wall -Wextra
>      libutils_la_CPPFLAGS = -D_GNU_SOURCE \
>                             -I$(top_builddir)/gl -I$(top_srcdir)/gl
>     -libutils_la_LIBADD = ../gl/libgnu.la
>     +libutils_la_LIBADD = gl/libgnu.la
>      libutils_la_LDFLAGS =
>     
>      # End of Makefile.am
>     --
>     2.26.2
>

signature.asc
Description: OpenPGP digital signature