(In reply to comment #0)
> Created an attachment (id=241933) [details] [review]
> add --with-incomplete-locales configure switch
> 
> Hi.
> 
> There are several ifdef WITH_INCOMPLETE_LOCALES in the code but these
> could never be reached since there was no way to define it.

That code is most likely from gdm, which had that configure switch. I'd rather remove that dead code than carry it around.

> I need it on at least OpenBSD, would that be alright to push?

Why is it needed on OpenBSD?

Hi Bastien.

The code comes indeed from GDM but it got removed there when the code got moved to gnome-desktop.
It is needed on OpenBSD to display the list of languages, otherwise we end up with an empty list.
See also: https://bugzilla.gnome.org/show_bug.cgi?id=698384

(In reply to comment #2)
> It is needed on OpenBSD to display the list of languages, otherwise we end up
> with an empty list.

Again, why? What makes OpenBSD locales be considered incomplete?

(In reply to comment #3)
> (In reply to comment #2)
> > It is needed on OpenBSD to display the list of languages, otherwise we end up
> > with an empty list.
> 
> Again, why? What makes OpenBSD locales be considered incomplete?

Because they are.
OpenBSD base system has no support for anything besides LC_CTYPE. gnome-desktop does a check for INCOMPLETE_LOCALES and use LC_CTYPE instead of LC_MESSAGES.

Hi Bastien.

Is there any more information that you need?
Thank you.

ping...

(In reply to comment #6)
> ping...

This isn't going to help. Removing the NEEDINFO when you've answered the question will show the patch again on patch reports.

is the problem really LC_CTYPE versus LC_MESSAGES, or it just that iso-codes isn't installed?

can you explain comment 4 in more detail?

(In reply to comment #9)
> can you explain comment 4 in more detail?

Sure I'll try ;-)

OpenBSD only fully supports LC_CTYPE.
$ ls /usr/share/locale/en_US.UTF-8/                                                                                             
LC_CTYPE

For the rest of the LC_ definitions, OpenBSD implementation currently only supports "C" and "POSIX".
That means basically that the following will fail:
setlocale (LC_MESSAGES, "en_US.UTF-8")
while this will succeed:
setlocale (LC_CTYPES, "en_US.UTF-8")

language_name_is_valid() will use LC_CTYPES if WITH_INCOMPLETE_LOCALES is defined instead of LC_MESSAGES. The result is that gnome-cc will properly display supported languages (from /usr/share/locale/*) instead of an empty list... which allows me to run GNOME in French...

I hope this clarifies things.

weird. So, if you:

1) delete all the

 #ifdef WITH_INCOMPLETE_LOCALES
 ...
 #endif

code

2) clean up the #ifndef WITH_INCOMPLETE_LOCALES code (since it will always be undefined)
3) globally replace all occurences of LC_MESSAGES with LC_CTYPE

does it also work?

if the experience in comment 11 pans out, i'm thinking it would probably be better to AC_DEFINE some LOCALE_MESSAGES_CATEGORY in configure.ac based on platform and change all occurences of LC_MESSAGES to LOCALE_MESSAGES_CATEGORY

*experiment

(In reply to comment #11)
> weird. So, if you:
> 
> 1) delete all the
> 
>  #ifdef WITH_INCOMPLETE_LOCALES
>  ...
>  #endif
> 
> code
> 
> 2) clean up the #ifndef WITH_INCOMPLETE_LOCALES code (since it will always be
> undefined)
> 3) globally replace all occurences of LC_MESSAGES with LC_CTYPE
> 
> does it also work?

It doesn't: "No Language Found".

But I don't think we want to change all occurrences of LC_MESSAGES with LC_CTYPE because gnome-desktop also specifically looks under PREFIX/share/locale/*/LC_MESSAGES/ if I read the code correctly.

I forgot to mention we also lack support for the locale-archive file (and IIRC also do other BSDs).
If it can give you guys more clue, this was the original commit introducing the incomplete locale support:
https://git.gnome.org/browse/gdm/commit/?id=d3009cfe5835a3348ba50ddcb447beb029eed849

Am I missing something obvious that you guys requested or could this go in at some point?
Thanks.

I'm still missing how it works I think.  basically, i'm pretty sure that ifdef isn't 100% correct, and while on the surface it may fix your issue, I want to understand why, so we can do a good job instead of a "just happens to make it work" job.

Can you get the debug output leading up to the "No Languages Found" error, and also post the patch you alluded to in comment 14?

> Can you get the debug output leading up to the "No Languages Found" error, and
> also post the patch you alluded to in comment 14?

Actually just using LC_CTYPES for language_name_is_valid() seems to be enough here. The issue in my previous comment was because all LC_CTYPES were turned into LC_MESSAGES.
New patch included.

Created attachment 245667 [details] [review]
drop unused WITH_INCOMPLETE_LOCALES definition

Review of attachment 245667 [details] [review]:

That's not quite good enough, as that will remove all the locale variants. A diff between the before/after outputs on Linux shows:
-be_BY.utf8 == Bielorusso (Bielo-Rússia) -- Bielo-Rússia (Bielorusso)
-be_BY.utf8@latin == Bielorusso (Bielo-Rússia) -- Bielo-Rússia (Bielorusso)
+be_BY.utf8 == Bielorusso -- Bielo-Rússia
-gez_ER.utf8@abegede == Geez (Eritréia) -- Eritréia (Geez)
-gez_ET.utf8@abegede == Geez (Etiópia) -- Etiópia (Geez)
-ks_IN.utf8 == Caxemira (Índia) -- Índia (Caxemira)
-ks_IN.utf8@devanagari == Caxemira (Índia) -- Índia (Caxemira)
+ks_IN.utf8 == Caxemira -- Índia (Caxemira)
-sr_RS.utf8@latin == Sérvio (Sérvia) -- Sérvia (Sérvio)
-sr_RS.utf8 == Sérvio (Sérvia) -- Sérvia (Sérvio)
+sr_RS.utf8 == Sérvio (Sérvia) -- Sérvia
-tt_RU.utf8@iqtelif == Tártaro (Federação Russa) -- Federação Russa (Tártaro)
-tt_RU.utf8 == Tártaro (Federação Russa) -- Federação Russa (Tártaro)
+tt_RU.utf8 == Tártaro -- Federação Russa (Tártaro)
-uz_UZ.utf8@cyrillic == Uzbeque -- Uzbequistão

That won't do...

GNOME currently searches locales supported by the system in two ways:
  - locale archive file (apparently Linux-specific)
  - names of directories in /usr/share/locale (platform-specific)

Neither of these is portable. The fact that names of directories in /usr/share/locale match names of locales on many systems is a conincidence. There is no spec that defines what the contents of this directory must be.

OpenBSD's layout of /usr/share/locale was changed in June this year and since then GNOME has been unable to detect supported locales.

There is a POSIX-compliant way of getting the names of available locale names, namely reading the output of the 'locale -a' command. I would suggest reading
'locale -a' output in addition to the above, to ensure that GNOME can function correctly on systems where the other approaches don't work.

This approach works on OpenBSD, where the other two approaches don't work, and should also work on any other POSIX-compliant system that ships with a locale utility.

Created attachment 253364 [details] [review]
Patch that implements above 'locale -a' suggestion.

I'm not sure if the above patch fully complies with GNOME coding guidelines. I'm happy to make any necessary adjustments in case the approach is considered viable.

With the above patch, GNOME always runs 'locale -a' in addition to search locales in other ways. Perhaps 'locale -a' could be run only as a fallback in case no locales could be found by other means, because on most systems running 'locale -a' might repeat work already done by GNOME itself. Or you might choose to run drop the platform-specific code and always run 'locale -a', if that is acceptable.

Note also that the patch I attached contains the changes from
https://bugzilla.gnome.org/attachment.cgi?id=245667, including the switch from LC_MESSAGES to LC_CTYPE of lc_type_id. I'm not sure whether this still breaks detection of locale variants on Linux, but if so, the patch may need further adjustments.

i'd say we should drop the archive stuff and use locale -a instead, not in addition. One of the glibc guys recently told me he didn't like that we were doing that.

Can you update your patch accordingly?

( see https://bugzilla.redhat.com/show_bug.cgi?id=956993 )

Created attachment 253394 [details] [review]
updated patch for locale detection via 'locale -a'

This patch replaces parsing of the locale archive file with reading locale -a output via popen(). If there are any glib APIs this patch should use instead of standard C, it would be nice if someone could point me to the relevant API docs.

The patch also removes all #ifdef WITH_INCOMPLETE_LOCALES code. But unlike the previous patch it leaves lc_type_id in language_name_is_valid() unchanged (doesn't change it from LC_MESSAGES to LC_CTYPE), since that is a separate issue and can also be addressed by changing OpenBSD's libc accordingly. So this patch should cause no regression on Linux.

locarchive.h can be removed after applying this patch. The removal of that file isn't part of the patch.

(In reply to comment #27)
> This patch replaces parsing of the locale archive file with reading locale -a
> output via popen(). If there are any glib APIs this patch should use instead of
> standard C, it would be nice if someone could point me to the relevant API
> docs.
g_spawn_async_with_pipes and g_spawn_sync are too functions that serve this role

Created attachment 253410 [details] [review]
glib-based patch for locale search with locale -a

Updated patch that uses g_spawn_sync and other glib APIs.

Also removes mapped_file_new_allow_noent() which I missed in the last patch. It was used only for reading locale archive files.

Review of attachment 253410 [details] [review]:

Can you make this a "git format-patch" patch please?

https://live.gnome.org/GnomeLove/SubmittingPatches

The patch at the surface makes sense to me, although I wonder how expensive it is to run the locale binary; maybe long term we need some system component caching this?  Or possibly that's just overkill and /usr/bin/locale should just be reading from some mmapped binary internally.

Created attachment 253414 [details] [review]
same patch as above, with log message, formatted with git format-patch

FWIW with the recent locale fixes in OpenBSD and this patch, everything now works perfectly; thank you Stefan.

Colin, Bastien, any feedback still? I would love to push this along with the locarchive.h removal (which I can provide as another patch once you guys agree on Stephan's patch).
Cheers!

Review of attachment 253414 [details] [review]:

This patch looks good.  Excellent commit message too.  I tested it, and it works for me; I just needed to add one tiny fix on top:

-        uint32_t locrec_offset;
+        guint32 locrec_offset;

Since at least on my system we're not pulling in stdint.  This code, like most of GNOME, prefers the GLib aliases, so I'll just push with that fix on top.