After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 346816 - Refactor LIBDIR in libcharset Makefile
Refactor LIBDIR in libcharset Makefile
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
2.12.x
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2006-07-07 00:32 UTC by Daniel Macks
Modified: 2017-12-04 13:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
unify dependent definitions of path to charset file (891 bytes, patch)
2006-07-07 00:35 UTC, Daniel Macks
none Details | Review
Oops, forgot to create the dir (1.03 KB, patch)
2006-07-07 00:56 UTC, Daniel Macks
rejected Details | Review
Support --with-charsetalias-dir in ./configure (3.13 KB, patch)
2017-11-20 08:01 UTC, Daniel Macks
none Details | Review
Support --with-charsetalias-dir in ./configure (2.39 KB, patch)
2017-11-20 10:25 UTC, Daniel Macks
none Details | Review
Support --with-charsetalias-dir in ./configure (3.14 KB, patch)
2017-11-20 10:44 UTC, Daniel Macks
none Details | Review
Support --with-charsetalias-dir in ./configure (3.14 KB, patch)
2017-11-20 10:49 UTC, Daniel Macks
committed Details | Review
libcharset: Don't try to include configmake.h (958 bytes, patch)
2017-12-03 15:37 UTC, Nirbheek Chauhan
committed Details | Review

Description Daniel Macks 2006-07-07 00:32:34 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.
Comment 1 Daniel Macks 2006-07-07 00:35:17 UTC
Created attachment 68510 [details] [review]
unify dependent definitions of path to charset file
Comment 2 Daniel Macks 2006-07-07 00:56:03 UTC
Created attachment 68511 [details] [review]
Oops, forgot to create the dir
Comment 3 Philip Withnall 2017-11-03 20:10:36 UTC
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?
Comment 4 Philip Withnall 2017-11-03 20:11:06 UTC
Please re-open if this is still relevant and we can continue discussion.
Comment 5 Daniel Macks 2017-11-04 06:09:44 UTC
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.
Comment 6 Philip Withnall 2017-11-06 11:24:52 UTC
(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.
Comment 7 Daniel Macks 2017-11-07 06:21:04 UTC
Is this a reasonable ./configure flag:

  --with-charsetaliasdir=PATH (default=$libdir)

and propagated via AC_DEFINE(GLIB_CHARSETALIAS_DIR)?
Comment 8 Philip Withnall 2017-11-07 11:21:25 UTC
(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.
Comment 9 Daniel Macks 2017-11-07 15:04:12 UTC
(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:)
Comment 10 Daniel Macks 2017-11-20 08:01:20 UTC
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:(
Comment 11 Philip Withnall 2017-11-20 10:21:05 UTC
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.
Comment 12 Daniel Macks 2017-11-20 10:25:20 UTC
Created attachment 364034 [details] [review]
Support --with-charsetalias-dir in ./configure

oops good catch.
Comment 13 Philip Withnall 2017-11-20 10:35:08 UTC
(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?
Comment 14 Daniel Macks 2017-11-20 10:44:21 UTC
Created attachment 364036 [details] [review]
Support --with-charsetalias-dir in ./configure

Too many platforms with too many workflows....
Comment 15 Daniel Macks 2017-11-20 10:49:44 UTC
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.
Comment 16 Philip Withnall 2017-11-20 11:26:14 UTC
Review of attachment 364038 [details] [review]:

Great, thanks.
Comment 17 Nirbheek Chauhan 2017-12-03 15:34:38 UTC
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.
Comment 18 Nirbheek Chauhan 2017-12-03 15:37:21 UTC
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
Comment 19 Philip Withnall 2017-12-04 09:24:39 UTC
Review of attachment 364856 [details] [review]:

Sure.
Comment 20 Nirbheek Chauhan 2017-12-04 13:11:24 UTC
Attachment 364856 [details] pushed as c9e6270 - libcharset: Don't try to include configmake.h