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

[Georgiy Tugai]

See inline responses below.

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, March 26, 2021 10:24 AM, Jose E. Marchesi <jemarch@gnu.org> wrote:

>
>
> 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+.

There was other stuff in bootstrap.conf that was also -lgpl, thus my
assumption about libpoke being LGPL.
Will fix!

> > 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.

The code in gl-libpoke/Makefile.am uses that conditional, whether it is
on Windows or not, so it must be defined.
The conditional is used to decide whether -DINSTALLDIR is bin/ or lib/,
which is used by relocatable-lib, thus my 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?

Will fix, as discussed in IRC. At the time it was the simplest strategy
to get things off the ground, since my autotools knowledge is lacking.

> > 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 =

All of the stuff above this point comes from Makefile.am as generated by
gnulib-tool without --makefile-name.

As per 
https://www.gnu.org/software/gnulib/manual/html_node/Modified-build-rules.html
I needed to initialize all of the relevant variables on Gnulib's behalf.

> > +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.

Sure.

> > +if RELOCATABLE_VIA_LD
> > +poke_LDFLAGS = `$(RELOCATABLE_LDFLAGS) $(bindir)`
> > +else
> > poke_LDFLAGS =
> >
> > -----------------------------------------------------------------------------------------------
> >
> > +endif
>
> Please indent inside the if..endif.

Will do.

> > 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.

I'll do that in v2, then.

> >      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