GNOME Bugzilla – Bug 346816
Refactor LIBDIR in libcharset Makefile
Last modified: 2017-12-04 13:11:31 UTC
glib/libcharset/Makefile.am handles installation and manipulation of the charset.alias file and also passing of that file's path via a -D flag. The path for installation, the path for a tempfile for manipulation, and the path passed as -DLIBDIR are each declared independently. These need to stay in sync, so it would be much easier for anyone who wants to change them (and clearer in general) if they all used a common variable.
Created attachment 68510 [details] [review] unify dependent definitions of path to charset file
Created attachment 68511 [details] [review] Oops, forgot to create the dir
Review of attachment 68511 [details] [review]: I don’t get the point in this. Do you want to install the charset files to a different location from the $(libdir) used for the rest of GLib?
Please re-open if this is still relevant and we can continue discussion.
Yes, I exactly need to install the charset files somewhere other than $(libdir). Several packages that use the localcharset module of gnulib all seem to try to install a $(libdir)/charset.alias file, which makes my package-manager software cry.
(In reply to Daniel Macks from comment #5) > Yes, I exactly need to install the charset files somewhere other than > $(libdir). Several packages that use the localcharset module of gnulib all > seem to try to install a $(libdir)/charset.alias file, which makes my > package-manager software cry. OK, seems reasonable to me. If you can provide an updated (and git-formatted) version of the patch, I’ll review and apply it. Thanks.
Is this a reasonable ./configure flag: --with-charsetaliasdir=PATH (default=$libdir) and propagated via AC_DEFINE(GLIB_CHARSETALIAS_DIR)?
(In reply to Daniel Macks from comment #7) > Is this a reasonable ./configure flag: > > --with-charsetaliasdir=PATH (default=$libdir) Looks fine to me. > and propagated via AC_DEFINE(GLIB_CHARSETALIAS_DIR)? You shouldn’t need to use AC_DEFINE, since it’s not going into the C code. AC_SUBST should be sufficient. --- Note that you should also add it to the Meson build system, since that will eventually replace the automake one, and it would be good to not have regressions when that time comes. Thanks.
(In reply to Philip Withnall from comment #8) > (In reply to Daniel Macks from comment #7) > > Is this a reasonable ./configure flag: > > > > --with-charsetaliasdir=PATH (default=$libdir) > > Looks fine to me. > > > and propagated via AC_DEFINE(GLIB_CHARSETALIAS_DIR)? > > You shouldn’t need to use AC_DEFINE, since it’s not going into the C code. > AC_SUBST should be sufficient. It goes in localcharset.c as well as glib/libcharset/Makefile.am. I chose the flagname based on the env var that .c uses to override it at runtime. AC_SUBST, with propagation to the compiler via -D (as is done now) seems fine to me. > Note that you should also add it to the Meson build system, since that will > eventually replace the automake one, and it would be good to not have > regressions when that time comes. Thanks. I have no experience with Meson, but looks like I'm about to learn. Thanks for the motivation:)
Created attachment 364027 [details] [review] Support --with-charsetalias-dir in ./configure The meson build system is not yet fully usable or equivalent to autotools on OS X and my voodoo-hacking on meson.build hasn't gotten me very far:(
Review of attachment 364027 [details] [review]: (In reply to Daniel Macks from comment #10) > The meson build system is not yet fully usable or equivalent to autotools on > OS X and my voodoo-hacking on meson.build hasn't gotten me very far:( For the moment we could skip the Meson support then. The fallback #define in localcharset.c should be sufficient, modulo the fix needed below. ::: glib/libcharset/localcharset.c @@ +70,2 @@ # include "configmake.h" +# define CHARSETALIASDIR LIBDIR In Makefile.am you’ve defined this as GLIB_CHARSETALIAS_DIR, not CHARSETALIASDIR.
Created attachment 364034 [details] [review] Support --with-charsetalias-dir in ./configure oops good catch.
(In reply to Daniel Macks from comment #12) > Created attachment 364034 [details] [review] [review] > Support --with-charsetalias-dir in ./configure > > oops good catch. In git format please?
Created attachment 364036 [details] [review] Support --with-charsetalias-dir in ./configure Too many platforms with too many workflows....
Created attachment 364038 [details] [review] Support --with-charsetalias-dir in ./configure May as well get the comment correct too. Anything else, I'll come back tomorrow.
Review of attachment 364038 [details] [review]: Great, thanks.
The meson build regressed because of this, and I fixed that with c603ba301d, but it seems it also broke the vcxproj files: https://mail.gnome.org/archives/gtk-devel-list/2017-December/msg00000.html That section of code seems to be cargo-culted from 2006 for no reason. `configmake.h` is an internal gnulib thing and is not available anywhere in glib: https://github.com/GerHobbelt/libiconv/commit/8a923955c19ebf60f7fc607e76c100e4d9e8cd31 I'm going to attach a patch that removes this and falls back to LIBDIR if GLIB_CHARSETALIAS_DIR is not defined.
Created attachment 364856 [details] [review] libcharset: Don't try to include configmake.h It's an internal gnulib thing which will never be available while building glib. Always expect LIBDIR and fallback to using that for compatibility with the existing MSVC projects: https://mail.gnome.org/archives/gtk-devel-list/2017-December/msg00000.html
Review of attachment 364856 [details] [review]: Sure.
Attachment 364856 [details] pushed as c9e6270 - libcharset: Don't try to include configmake.h