Re: [PATCH 3/5] Allow Poke to be built relocatable

[Jose E. Marchesi]

Hi Georgiy.

Thanks for the patch.  Please see some comments below.

        (libpoke_modules): add relocatable-lib-lgpl

Why importing relocatable-lib-lgpl instead of relocatable-lib?  libpoke
is GPLv3+.

> diff --git a/configure.ac b/configure.ac
> index 7ca26abb..2b19683d 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -173,6 +173,10 @@ dnl following macro is also supposed to work when 
> cross-compiling.
>
>  AC_C_BIGENDIAN
>
> +dnl Gnulib libpoke relocatable-lib-lgpl needs this
> +AM_CONDITIONAL([SHLIBS_IN_BINDIR],
> +               [case "$host_os" in mingw* | cygwin*) true;; *) false;; esac])
> +

I think it is Windows that really needs that setting right, rather than
relocatable-lib, right?  I would make it clear in the comment.

>  dnl The following M4 macro from gnulib sets HOST_CPU_C_ABI_32BIT to
>  dnl 'yes' if the C language ABI is a 32-bit one, to 'no' if it is
>  dnl 64-bit, or to 'unknown'.
> @@ -270,6 +274,7 @@ AC_CONFIG_FILES(Makefile
>                  testsuite/poke.mi-json/Makefile)
>  AC_CONFIG_FILES([run],
>                  [chmod +x,-w run])
> +AC_CONFIG_SUBDIRS([gl-libpoke])

Hmm why not just adding gl-libpoke/Makefile to AC_CONFIG_FILES?

> diff --git a/gl-libpoke/Makefile.am b/gl-libpoke/Makefile.am
> new file mode 100644
> index 00000000..df8bc0a1
> --- /dev/null
> +++ b/gl-libpoke/Makefile.am
> @@ -0,0 +1,27 @@
> +AUTOMAKE_OPTIONS = 1.11 gnits

Why gnits?

> +SUBDIRS =
> +noinst_HEADERS =
> +noinst_LIBRARIES =
> +noinst_LTLIBRARIES =
> +EXTRA_DIST =
> +BUILT_SOURCES =
> +SUFFIXES =
> +MOSTLYCLEANFILES = core *.stackdump

Is the configure or build supposed to dump a core? :D

> +MOSTLYCLEANDIRS =
> +CLEANFILES =
> +DISTCLEANFILES =
> +MAINTAINERCLEANFILES =
> +
> +AM_CFLAGS =
> +AM_CPPFLAGS =
> +
> +AM_CPPFLAGS += -DIN_LIBRARY -DENABLE_COSTLY_RELOCATABLE
> +
> +if SHLIBS_IN_BINDIR
> +AM_CPPFLAGS += -DINSTALLDIR=\"$(bindir)\"
> +else
> +AM_CPPFLAGS += -DINSTALLDIR=\"$(libdir)\"
> +endif
> +

> diff --git a/poke/Makefile.am b/poke/Makefile.am
> index 594a6885..c468ad85 100644
> --- a/poke/Makefile.am
> +++ b/poke/Makefile.am
> @@ -26,6 +26,8 @@ BUILT_SOURCES =
>
>  EXTRA_DIST =
>
> +RELOCATABLE_LIBRARY_PATH = $(libdir)
> +
>  dist_pkgdata_DATA = pk-cmd.pk pk-dump.pk pk-save.pk pk-copy.pk \
>                      pk-extract.pk pk-scrabble.pk poke.pk
>
> @@ -50,14 +52,18 @@ poke_CPPFLAGS = -I$(top_builddir)/gl -I$(top_srcdir)/gl \
>                  -DJITTER_VERSION=\"$(JITTER_VERSION)\" \
>                  -DPKGDATADIR=\"$(pkgdatadir)\" \
>                  -DPKGINFODIR=\"$(infodir)\" \
> -                -DLOCALEDIR=\"$(localedir)\"
> +                -DLOCALEDIR=\"$(localedir)\" \
> +                -DINSTALLDIR=\"$(bindir)\"
>  poke_CFLAGS = -Wall
>  poke_LDADD = $(top_builddir)/gl/libgnu.la \
>               $(top_builddir)/libpoke/libpoke.la \
>               $(LTLIBREADLINE) \
>               $(LTLIBTEXTSTYLE)

Please leave an empty line here.

> +if RELOCATABLE_VIA_LD
> +poke_LDFLAGS = `$(RELOCATABLE_LDFLAGS) $(bindir)`
> +else
>  poke_LDFLAGS =
> -
> +endif

Please indent inside the if..endif.

>  AM_LFLAGS = -d
>  # The Automake generated .l.c rule is broken: When executed in a VPATH build,
> diff --git a/poke/pk-map.c b/poke/pk-map.c
> index 67bb6056..e960ea62 100644
> --- a/poke/pk-map.c
> +++ b/poke/pk-map.c
> @@ -17,6 +17,7 @@
>   */
>
>  #include <config.h>
> +#include <relocatable.h>
>
>  #include <stdio.h>
>  #include <inttypes.h>
> @@ -678,7 +679,7 @@ pk_map_resolve_map (const char *mapname, int filename_p)
>      const char *s, *e;
>
>      char *fixed_load_path
> -      = pk_str_replace (map_load_path, "%DATADIR%", PKGDATADIR);
> +      = pk_str_replace (map_load_path, "%DATADIR%", relocate (PKGDATADIR));


This is not directly related to this patch, but I think it would be good
to use the global poke_datadir here instead of PKGDATADIR, and include
relocate.h only in poke.c.

>
>      for (s = fixed_load_path, e = s; *e; s = e + 1)
>        {
> diff --git a/poke/pk-term.c b/poke/pk-term.c
> index 6a89697c..f799ec6b 100644
> --- a/poke/pk-term.c
> +++ b/poke/pk-term.c
> @@ -17,6 +17,7 @@
>   */
>
>  #include <config.h>
> +#include <relocatable.h>
>
>  #include <stdlib.h> /* For exit.  */
>  #include <assert.h> /* For assert. */
> @@ -202,7 +203,7 @@ pk_term_init (int argc, char *argv[])
>        || color_mode == color_html)
>      {
>        /* Find the style file.  */
> -      style_file_prepare ("POKE_STYLE", "POKESTYLESDIR", PKGDATADIR,
> +      style_file_prepare ("POKE_STYLE", "POKESTYLESDIR", relocate 
> (PKGDATADIR),
>                            "poke-default.css");

Same here.

>      }
>    else