Use pthread_once to call bindtextdomain only once

[sebastinas]

Calling bindtextdomain everytime in discid_new incurs a little overhead. If
pthread is available, we can ensure with pthread_once that bindtextdomain is
called exactly once.

The autoconf function AX_PTHREADS is available from autoconf-archive. In cmake
FIND_LIBRARY(Threads) is used to find pthread.

[JonnyJD]

Starting to use threading just for that? Not really sure if this is worth the additional complexity. It is a performance issue as far as I understand and probably no bottleneck (compared to reading the TOC from a disc).

However, if it works fine on all platforms without (much) additional work then this is possibly fine. I'll do some testing.

The CI test fails:
http://ci.musicbrainz.org/job/libdiscid_PR/build_system=autotools/33/console
(that one was started manually, the automatic one had to be aborted for some reason)

  autoconf
configure.ac:43: error: possibly undefined macro: AC_DEFINE
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.

This totally misleading error message is fixed by installing autoconf-archive on my machine, but this is yet another thing that isn't installed on the CI server and an additional dev build dependency.

Not a complete showstopper, but I don't like it and it obviously won't be merged until this works on the CI server (over which I don't have full control myself).

[sebastinas]

On 2013-07-23 16:35:24, Johannes Dewender wrote:

Starting to use threading just for that? Not really sure if this is worth the additional complexity. It is a performance issue as far as I understand and probably no bottleneck (compared to reading the TOC from a disc).

Does linking against pthread introduce threading? We're not creating threads or
anything, so no.

However, if it works fine on all platforms without (much) additional work then this is possibly fine. I'll do some testing.

The CI test fails:
http://ci.musicbrainz.org/job/libdiscid_PR/build_system=autotools/33/console
(that one was started manually, the automatic one had to be aborted for some reason)

  autoconf
configure.ac:43: error: possibly undefined macro: AC_DEFINE
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.

This totally misleading error message is fixed by installing autoconf-archive on my machine, but this is yet another thing that isn't installed on the CI server and an additional dev build dependency.

As usual with autoconf macros, one can just drop the macro in m4 (or any
other directory specified in AC_CONFIG_MACRO_DIR) and there is no
additional dependency.

If you prefer that, please let me know. Otherwise it should be easy
enough to write a test that tries to build and run a program using
pthread_once and -pthread.

If you don't like the pthread solution, we can also do one of the following:

• Move the bindtextdomain call into a discid_init and let applications that
  want translated error messages call it.
• Using attribute((constructor)) (a gcc-ism) the same thing could be
  achieved presumably. Haven't tested that.
• Use the equivalent function from glib.

[JonnyJD]

Does linking against pthread introduce threading? We're not creating threads or anything, so no.

We are not using multi-threading but we are using platform dependent threading mechanisms. Which would be meaningless if this would be a linux-only library, but raises maintenance effort getting this to work on every supported platform.

As usual with autoconf macros, one can just drop the macro in m4 (or any other directory specified in AC_CONFIG_MACRO_DIR) and there is no additional dependency.

That would be worse, because then we have to maintain a 300 line m4 script in our code. (Yes, there is a possibility we never have to change anything at all, but also one having weird bugs in that part for a platform)
Getting autoconf-archive installed on dev machines would be the better option.

Currently I'd say this is the best solution, but I am not sure if this is a big enough problem for this solution.
I'll see if I can get somebody to install autoconf-archive on the CI machine anyways. That won't hurt.

There is no rush though, I still need to figure out how i18n would work for libdiscid bundled for a Windows or Mac OS X program (MusicBrainz Picard does that). I have a rough idea, but nothing tested or implemented.

[JonnyJD]

jenkins: retest please

EDIT: starting by comment still doesn't work, but a manual start failed:
http://ci.musicbrainz.org/job/libdiscid_PR/build_system=autotools/40/
(not sure what went wrong, autoconf-archive is supposed to be installed now)

[JonnyJD]

Meh. autoconf-archive is now installed on the build server, but the oldstable version (http://packages.debian.org/de/squeeze/autoconf-archive) doesn't include ax_pthread.m4 macro. And this time, there isn't even a backport available.

Still no final decisions (I'll see how complicated i18n is when everything else works, maybe this is a minor issue compared to other i18n related issues), but right now this doesn't look like something I would implement without actual need. (no current impact/breakage performance problem)

[sebastinas]

On 2013-07-29 01:15:17, Johannes Dewender wrote:

Does linking against pthread introduce threading? We're not creating threads or anything, so no.

We are not using multi-threading but we are using platform dependent threading mechanisms. Which would be meaningless if this would be a linux-only library, but raises maintenance effort getting this to work on every supported platform.

pthread is supported on every major platform. Supposedly there is a
pthread-win32 for Windows. There is even a equivalent function to pthread_once
provided by the kernel from Vista onwards.

As usual with autoconf macros, one can just drop the macro in m4 (or any other directory specified in AC_CONFIG_MACRO_DIR) and there is no additional dependency.

That would be worse, because then we have to maintain a 300 line m4 script in our code. (Yes, there is a possibility we never have to change anything at all, but also one having weird bugs in that part for a platform)
Getting autoconf-archive installed on dev machines would be the better option.

There is no need to maintain the file at all. Running aclocal --install --force
from time to time is more than enough to get the latest version from
autoconf-archive included.

Apparently the thousand of lines dropped in m4 by gettext and libtool are less

of a concern than the m4 file for pthread, so I give up.

Sebastian Ramacher

[JonnyJD]

Sebastian Ramacher wrote at Monday 29 July 2013 09:00:11:

On 2013-07-29 01:15:17, Johannes Dewender wrote:

Does linking against pthread introduce threading? We're not
creating threads or anything, so no.>
We are not using multi-threading but we are using platform
dependent threading mechanisms. Which would be meaningless if this
would be a linux-only library, but raises maintenance effort
getting this to work on every supported platform.
pthread is supported on every major platform. Supposedly there is a
pthread-win32 for Windows. There is even a equivalent function to
pthread_once provided by the kernel from Vista onwards.

Good to know. I didn't even check Mac and Windows yet since that is meaningless unless we get i18n to work on these.
So the only problem left is the autoconf macro when building the source distribution from the repository.

As usual with autoconf macros, one can just drop the macro in m4
(or any other directory specified in AC_CONFIG_MACRO_DIR) and
there is no additional dependency.>
That would be worse, because then we have to maintain a 300 line m4
script in our code. (Yes, there is a possibility we never have to
change anything at all, but also one having weird bugs in that part
for a platform) Getting autoconf-archive installed on dev machines
would be the better option.
There is no need to maintain the file at all. Running aclocal
--install --force from time to time is more than enough to get the
latest version from autoconf-archive included.

I think we are talking about different things.
There are 3 main ways to get a working libdiscid:

1. using a libdiscid binary from somewhere
2. compiling from a released source distribution (.tar.gz)
3. compiling from the git repository (creating the source distribution)

1 and 2 don't have to care about autoconf-archive and additional m4 macros at all. They are either included in the distribution or "already dealt with". This also includes debian packaging. That is possibly also the reason you don't see a problem at all.

3 needs to get the pthread autoconf macro somehow with the proposed changes.
This isn't a problem on my dev machine since autoconf-archive is available. However, it is a problem for everybody (or every machine) building directly from the git repository when no current autoconf-archive is available.

Currently these are people willing to test pre-release versions of libdiscid and the continuous integration build server of musicbrainz.
Additionally I build the binary windows and Mac distributions also directly from the git repository. Including using autoconf/automake to build a full source distribution. (Yes, that currently works fine)

The build server, Mac OS X and Windows don't seem to have an easy way to get a current autoconf-archive.
Depending on how big of an issue this is, there are ways to solve this, but it does break current workflows and does make things more complex. Fine for important things, but not for things that are just "nice to have".

Apparently the thousand of lines dropped in m4 by gettext and libtool
are less of a concern than the m4 file for pthread, so I give up.

"Äpfel und Birnen.."

The macros for gettext and libtool are quite big and make the source distribution (2.) a lot bigger. This is a general autotools problem/feature, but fine. These are macros provided by vanilla autoconf, without additional autoconf-archive needed. All (source distribution) build systems that need autotools (see 3.) have these installed.
In contrast, the pthread macros are not part of plain autoconf so they are not available everywhere.
So I took

As usual with autoconf macros, one can just drop the macro in m4

as "I should include these in the git repository". Which is a valid option in general, but different from adding m4 macros to the source distribution (automatically).

Additionally, both have an actual usage besides performance enhancements (not sure yet if there is an actual performance impact).

Again:
I would like to get i18n working on the supported platforms first and only afterwards possibly do things like including m4 files in the actual repository for this.
I don't like having code that isn't really specific to libdiscid in the repository, unless necessary or at least very convenient. Lots of bigger projects have these, but we are not "bigger projects". I am not saying it is an impossible thing to do, though. Especially when we start adding m4 files to the repository for other reasons it is a small step to add more of these..

I still want to thank you for your contribution. It doesn't make everything work out of the box and isn't what's needed most atm, but it is non-trivial code and it is an improvement (when we fix 3. somehow).