Bug#990511: unblock: kodi/2:19.1+dfsg2-2
Hi Sebastian, One more (technical) thing to mention: the best way to centralize ToLower and ToUpper (omitting collations for now, for that there is a libicu PR for v20 targeting Android) is to declare an overloaded 'StringUtils::ToLower(char c, bool useCLocale)' function and make all Kodi use it. However, there are two instances of StringUtils.h and the second one is part of Kodi API SDK. Fixing Kodi API SDK will be immediately rejected by upstream for v19 because it triggers an API bump and rebuild of all addons (including minimum 12 addons of 49 in Debian during the full freeze!). Since the workaround does not break movie search / collation / addons, let's follow the upstream path and have it merged. For v20 development cycle, we can either refactor lower/upper/collation things or use libicu to perform similar comparisons. If you have a better approach to solve this issue, please let me know on https://github.com/xbmc/xbmc/issue/19883 Cheers! -- Vasyl Gello == Certified SolidWorks Expert Mob.:+380 (98) 465 66 77 E-Mail: vasek.ge...@gmail.com Skype: vasek.gello == 호랑이는 죽어서 가죽을 남기고 사람은 죽어서 이름을 남긴다
Bug#990511: unblock: kodi/2:19.1+dfsg2-2
>C++ has an overloaded version of tolower that takes a locale: >https://en.cppreference.com/w/cpp/locale/tolower Good find, thanks! Still, what do you think about this concern: > but to separate "culture-dependent" and "ordinal" comparisons, one needs to > overhaul half of Kodi. > And this would make the fix unfit for 19.x branch as the regressions would be > just as severe as they could be with CDateTime PR. I dont think spending several weeks to isolate all places that need C-locale comparison from those implementing language-specific processing (amd testing all possible regressions!) is worth for 19.x. -- Vasyl Gello == Certified SolidWorks Expert Mob.:+380 (98) 465 66 77 E-Mail: vasek.ge...@gmail.com Skype: vasek.gello == 호랑이는 죽어서 가죽을 남기고 사람은 죽어서 이름을 남긴다
Bug#990511: unblock: kodi/2:19.1+dfsg2-2
On 2021-07-02 08:48:10 +, Vasyl Gello wrote: > Also I don't see std::tolower_l as part of C++ standard. Looks like tolower_l > is POSIX standard and Kodi uses std:: stuff there :( C++ has an overloaded version of tolower that takes a locale: https://en.cppreference.com/w/cpp/locale/tolower Cheers > -- > Vasyl Gello > == > Certified SolidWorks Expert > > Mob.:+380 (98) 465 66 77 > > E-Mail: vasek.ge...@gmail.com > > Skype: vasek.gello > == > 호랑이는 죽어서 가죽을 남기고 사람은 죽어서 이름을 남긴다 -- Sebastian Ramacher signature.asc Description: PGP signature
Bug#990511: unblock: kodi/2:19.1+dfsg2-2
Also I don't see std::tolower_l as part of C++ standard. Looks like tolower_l is POSIX standard and Kodi uses std:: stuff there :( -- Vasyl Gello == Certified SolidWorks Expert Mob.:+380 (98) 465 66 77 E-Mail: vasek.ge...@gmail.com Skype: vasek.gello == 호랑이는 죽어서 가죽을 남기고 사람은 죽어서 이름을 남긴다
Bug#990511: unblock: kodi/2:19.1+dfsg2-2
Hi Sebastian!
> So what you're telling me is that this tolower call should actually be
> call to tolower_l with an appropriate locale as kodi only expects
> tolower("...") == "includes.xml" under the C locale.
Very likely it is the more "proper" solution, but to separate
"culture-dependent" and "ordinal" comparisons, one needs to overhaul half of
Kodi.
And this would make the fix unfit for 19.x branch as the regressions would be
just as severe as they could be with CDateTime PR.
I consider this patch as a quick workaround that works on all languages so far
(to my surprise, as I tested that in loop with ~40 languages using API calls &
scrot). As I mentioned in the upstream issue, if this workaround would not have
worked, I'd have either to use tolower_t, toupper_t locale-wise, use
"uselocale(8)" or even switch to ICU. But this worked and upstream approved
PR#19905. Having said that, I think it is good to go.
--
Vasyl Gello
==
Certified SolidWorks Expert
Mob.:+380 (98) 465 66 77
E-Mail: vasek.ge...@gmail.com
Skype: vasek.gello
==
호랑이는 죽어서 가죽을 남기고 사람은 죽어서 이름을 남긴다
Bug#990511: unblock: kodi/2:19.1+dfsg2-2
Hi Vasyl
On 2021-07-02 08:14:34 +, Vasyl Gello wrote:
> Hi Sebastian!
>
> > I'm not sure why the second patch is needed. If I understand the first
> > patch correctly, in the case where LC_CTYPE is set to a Turkish locale,
> > the locale will be reset to C.UTF-8 (not sure that this is sane,
> > though). The second patch makes it even worse since it resets it to
> > C.UTF-8 for everyone else as well.
>
> This is intended. Kodi internally expects to start with classic C locale and
> after language pack is loaded, Kodi's global locale is set to the desired
> locale.
> The root cause of the bug is incorrect comparison of skin's includes.xml
> involving lowercase translation of file name (see upstream issue #19883,
> linked)
> So there are two scenarios possible when the bug can manifest:
>
> 1. LC_TYPE is set to a locale where "Turkish I" is effective (tr_*, az_AZ)
> and user chooses Kodi language other than Turkish / Azeri.
> In this case, the locale is set to C + overrides from LC_CTYPE / LANG /
> LC_ALL, so it becomes C + CTYPE("tr_TR.UTF-8"). The setlocale() call enforces
> this "franken-locale" and the tolower("Includes.xml") produces "Includes.xml"
> as per Turkish CTYPE rules, and not "inclides.xml" as expected.
So what you're telling me is that this tolower call should actually be
call to tolower_l with an appropriate locale as kodi only expects
tolower("...") == "includes.xml" under the C locale.
Cheers
>
> 2. LC_* and LANG is set to arbitrary locale and user chooses Turkish / Azeri
> language.
> In this case, the locale is set to whatever the environment variables declare
> first, but then "CLangInfo::CRegion::SetGlobalLocale" sets global locale to
> "tr_TR.UTF-8" and Kodi interface gets completely broken due to failed XML
> substitutions etc.
>
> That's why the first patch is not enough and I had to add the second one to
> enforce Kodi's internal expectation to start with POSIX locale.
>
> Mattia asked the same question, BTW :) The resolution looks odd at the first
> glance as experienced users might expect Kodi to display UI in language
> controlled by LANG or LC_ALL environment variable, but with Kodi, the
> language is controlled by Settings -> Skin - Region -> Language and only by
> that means, because unlike coreutils, language packs are shipped in form of
> Kodi addons from central Kodi repository (https://mirrors.kodi.tv) and not as
> a collection of PO files.
>
> --
> Vasyl Gello
> ==
> Certified SolidWorks Expert
>
> Mob.:+380 (98) 465 66 77
>
> E-Mail: vasek.ge...@gmail.com
>
> Skype: vasek.gello
> ==
> 호랑이는 죽어서 가죽을 남기고 사람은 죽어서 이름을 남긴다
--
Sebastian Ramacher
Bug#990511: unblock: kodi/2:19.1+dfsg2-2
Hi Sebastian!
> I'm not sure why the second patch is needed. If I understand the first
> patch correctly, in the case where LC_CTYPE is set to a Turkish locale,
> the locale will be reset to C.UTF-8 (not sure that this is sane,
> though). The second patch makes it even worse since it resets it to
> C.UTF-8 for everyone else as well.
This is intended. Kodi internally expects to start with classic C locale and
after language pack is loaded, Kodi's global locale is set to the desired
locale.
The root cause of the bug is incorrect comparison of skin's includes.xml
involving lowercase translation of file name (see upstream issue #19883, linked)
So there are two scenarios possible when the bug can manifest:
1. LC_TYPE is set to a locale where "Turkish I" is effective (tr_*, az_AZ) and
user chooses Kodi language other than Turkish / Azeri.
In this case, the locale is set to C + overrides from LC_CTYPE / LANG / LC_ALL,
so it becomes C + CTYPE("tr_TR.UTF-8"). The setlocale() call enforces this
"franken-locale" and the tolower("Includes.xml") produces "Includes.xml" as per
Turkish CTYPE rules, and not "inclides.xml" as expected.
2. LC_* and LANG is set to arbitrary locale and user chooses Turkish / Azeri
language.
In this case, the locale is set to whatever the environment variables declare
first, but then "CLangInfo::CRegion::SetGlobalLocale" sets global locale to
"tr_TR.UTF-8" and Kodi interface gets completely broken due to failed XML
substitutions etc.
That's why the first patch is not enough and I had to add the second one to
enforce Kodi's internal expectation to start with POSIX locale.
Mattia asked the same question, BTW :) The resolution looks odd at the first
glance as experienced users might expect Kodi to display UI in language
controlled by LANG or LC_ALL environment variable, but with Kodi, the language
is controlled by Settings -> Skin - Region -> Language and only by that means,
because unlike coreutils, language packs are shipped in form of Kodi addons
from central Kodi repository (https://mirrors.kodi.tv) and not as a collection
of PO files.
--
Vasyl Gello
==
Certified SolidWorks Expert
Mob.:+380 (98) 465 66 77
E-Mail: vasek.ge...@gmail.com
Skype: vasek.gello
==
호랑이는 죽어서 가죽을 남기고 사람은 죽어서 이름을 남긴다
Bug#990511: unblock: kodi/2:19.1+dfsg2-2
Control: tags -1 moreinfo
On 2021-07-01 03:47:05 +, Vasyl Gello wrote:
> Package: release.debian.org
> Severity: normal
> User: release.debian@packages.debian.org
> Usertags: unblock
> X-Debbugs-Cc: mat...@debian.org
>
> Please unblock package kodi
>
> [ Reason ]
>
> Targeted bug fix for #989814
>
> [ Impact ]
>
> Turkish users get Kodi unusable without it
>
> [ Tests ]
>
> See related Debian bug and https://github.com/xbmc/xbmc/issues/19883
>
> [ Risks ]
>
> Change is trivial and approved by upstream
>
> [ Checklist ]
> [x] all changes are documented in the d/changelog
> [x] I reviewed all changes and I approve them
> [x] attach debdiff against the package in testing
>
> [ Other info ]
>
> unblock kodi/2:19.1+dfsg2-2
> diff -Nru kodi-19.1+dfsg2/debian/changelog kodi-19.1+dfsg2/debian/changelog
> --- kodi-19.1+dfsg2/debian/changelog 2021-06-07 14:42:08.0 +
> +++ kodi-19.1+dfsg2/debian/changelog 2021-06-24 20:44:30.0 +
> @@ -1,3 +1,9 @@
> +kodi (2:19.1+dfsg2-2) unstable; urgency=medium
> +
> + * Add runtime locale test and fallback (Closes: #989814)
> +
> + -- Vasyl Gello Thu, 24 Jun 2021 20:44:30 +
> +
> kodi (2:19.1+dfsg2-1) unstable; urgency=medium
>
>* New upstream version 19.1+dfsg2
> diff -Nru kodi-19.1+dfsg2/debian/patches/kodi/0022-Workaround-989814.patch
> kodi-19.1+dfsg2/debian/patches/kodi/0022-Workaround-989814.patch
> --- kodi-19.1+dfsg2/debian/patches/kodi/0022-Workaround-989814.patch
> 1970-01-01 00:00:00.0 +
> +++ kodi-19.1+dfsg2/debian/patches/kodi/0022-Workaround-989814.patch
> 2021-06-24 20:44:30.0 +
> @@ -0,0 +1,67 @@
> +From 8b8e97dbec5c6268d1b81eb7799cfc945ca9520e Mon Sep 17 00:00:00 2001
> +From: Vasyl Gello
> +Date: Fri, 25 Jun 2021 01:37:02 +
> +Subject: [PATCH 1/2] Check if applied locale correctly lowers chars and
> + fallback
> +
> +.. to default region if it does not.
> +
> +Fixes #19883.
> +
> +Signed-off-by: Vasyl Gello
> +---
> + xbmc/LangInfo.cpp | 10 ++
> + 1 file changed, 10 insertions(+)
> +
> +diff --git a/xbmc/LangInfo.cpp b/xbmc/LangInfo.cpp
> +index 24f0419cfe..ace72e1ffe 100644
> +--- a/xbmc/LangInfo.cpp
> b/xbmc/LangInfo.cpp
> +@@ -981,6 +981,16 @@ void CLangInfo::SetCurrentRegion(const std::string&
> strName)
> +
> + m_currentRegion->SetGlobalLocale();
> +
> ++ // Check if locale is not affected by #19883
> ++ int test19883 = std::tolower('i') - std::tolower('I');
> ++ if (test19883 != 0)
> ++ {
> ++CLog::Log(LOGWARNING, "region '{}' is affected by #19883 - falling back
> to default region '{}'",
> ++ m_currentRegion->m_strName, m_defaultRegion.m_strName);
> ++m_currentRegion = _defaultRegion;
> ++m_currentRegion->SetGlobalLocale();
> ++ }
> ++
> + const std::shared_ptr settings =
> CServiceBroker::GetSettingsComponent()->GetSettings();
> + if (settings->GetString(CSettings::SETTING_LOCALE_SHORTDATEFORMAT) ==
> SETTING_REGIONAL_DEFAULT)
> + SetShortDateFormat(m_currentRegion->m_strDateFormatShort);
> +--
> +2.32.0.rc0
> +
> +
> +From 114ee13138389c96a759d6e5b73717093dd4030d Mon Sep 17 00:00:00 2001
> +From: Vasyl Gello
> +Date: Sun, 27 Jun 2021 19:31:39 +
> +Subject: [PATCH 2/2] kodi.sh.in: Unset LC_{ALL,CTYPE}, LANG
> +
> +Signed-off-by: Vasyl Gello
> +---
> + tools/Linux/kodi.sh.in | 3 +++
> + 1 file changed, 3 insertions(+)
> +
> +diff --git a/tools/Linux/kodi.sh.in b/tools/Linux/kodi.sh.in
> +index 108c0b007b..29d17d2c0f 100644
> +--- a/tools/Linux/kodi.sh.in
> b/tools/Linux/kodi.sh.in
> +@@ -171,6 +171,9 @@ if command_exists gdb; then
> + fi
> + fi
> +
> ++# Unset CTYPE, LANG and ALL - see issue #19883
> ++unset LC_CTYPE LC_ALL LANG
I'm not sure why the second patch is needed. If I understand the first
patch correctly, in the case where LC_CTYPE is set to a Turkish locale,
the locale will be reset to C.UTF-8 (not sure that this is sane,
though). The second patch makes it even worse since it resets it to
C.UTF-8 for everyone else as well.
Cheers
> ++
> + LOOP=1
> + while [ $(( $LOOP )) = "1" ]
> + do
> +--
> +2.32.0.rc0
> +
> diff -Nru kodi-19.1+dfsg2/debian/patches/series
> kodi-19.1+dfsg2/debian/patches/series
> --- kodi-19.1+dfsg2/debian/patches/series 2021-06-07 14:42:08.0
> +
> +++ kodi-19.1+dfsg2/debian/patches/series 2021-06-24 20:44:30.0
> +
> @@ -19,6 +19,7 @@
> kodi/0019-Disable-GetCPUFrequency-test.patch
> kodi/0020-Fix-C++-example-includes.patch
> kodi/0021-Detect-and-honor-big-endian-arch.patch
> +kodi/0022-Workaround-989814.patch
> libdvdnav/0001-xbmc-dvdnav-allow-get-set-vm-state.patch
> libdvdnav/0002-xbmc-dvdnav-expose-dvdnav_get_vm-dvdnav_get_button_i.patch
> libdvdnav/0003-xbmc-dvdnav-detection-of-dvd-name.patch
--
Sebastian Ramacher
signature.asc
Description: PGP signature
Bug#990511: unblock: kodi/2:19.1+dfsg2-2
Package: release.debian.org
Severity: normal
User: release.debian@packages.debian.org
Usertags: unblock
X-Debbugs-Cc: mat...@debian.org
Please unblock package kodi
[ Reason ]
Targeted bug fix for #989814
[ Impact ]
Turkish users get Kodi unusable without it
[ Tests ]
See related Debian bug and https://github.com/xbmc/xbmc/issues/19883
[ Risks ]
Change is trivial and approved by upstream
[ Checklist ]
[x] all changes are documented in the d/changelog
[x] I reviewed all changes and I approve them
[x] attach debdiff against the package in testing
[ Other info ]
unblock kodi/2:19.1+dfsg2-2
diff -Nru kodi-19.1+dfsg2/debian/changelog kodi-19.1+dfsg2/debian/changelog
--- kodi-19.1+dfsg2/debian/changelog2021-06-07 14:42:08.0 +
+++ kodi-19.1+dfsg2/debian/changelog2021-06-24 20:44:30.0 +
@@ -1,3 +1,9 @@
+kodi (2:19.1+dfsg2-2) unstable; urgency=medium
+
+ * Add runtime locale test and fallback (Closes: #989814)
+
+ -- Vasyl Gello Thu, 24 Jun 2021 20:44:30 +
+
kodi (2:19.1+dfsg2-1) unstable; urgency=medium
* New upstream version 19.1+dfsg2
diff -Nru kodi-19.1+dfsg2/debian/patches/kodi/0022-Workaround-989814.patch
kodi-19.1+dfsg2/debian/patches/kodi/0022-Workaround-989814.patch
--- kodi-19.1+dfsg2/debian/patches/kodi/0022-Workaround-989814.patch
1970-01-01 00:00:00.0 +
+++ kodi-19.1+dfsg2/debian/patches/kodi/0022-Workaround-989814.patch
2021-06-24 20:44:30.0 +
@@ -0,0 +1,67 @@
+From 8b8e97dbec5c6268d1b81eb7799cfc945ca9520e Mon Sep 17 00:00:00 2001
+From: Vasyl Gello
+Date: Fri, 25 Jun 2021 01:37:02 +
+Subject: [PATCH 1/2] Check if applied locale correctly lowers chars and
+ fallback
+
+.. to default region if it does not.
+
+Fixes #19883.
+
+Signed-off-by: Vasyl Gello
+---
+ xbmc/LangInfo.cpp | 10 ++
+ 1 file changed, 10 insertions(+)
+
+diff --git a/xbmc/LangInfo.cpp b/xbmc/LangInfo.cpp
+index 24f0419cfe..ace72e1ffe 100644
+--- a/xbmc/LangInfo.cpp
b/xbmc/LangInfo.cpp
+@@ -981,6 +981,16 @@ void CLangInfo::SetCurrentRegion(const std::string&
strName)
+
+ m_currentRegion->SetGlobalLocale();
+
++ // Check if locale is not affected by #19883
++ int test19883 = std::tolower('i') - std::tolower('I');
++ if (test19883 != 0)
++ {
++CLog::Log(LOGWARNING, "region '{}' is affected by #19883 - falling back
to default region '{}'",
++ m_currentRegion->m_strName, m_defaultRegion.m_strName);
++m_currentRegion = _defaultRegion;
++m_currentRegion->SetGlobalLocale();
++ }
++
+ const std::shared_ptr settings =
CServiceBroker::GetSettingsComponent()->GetSettings();
+ if (settings->GetString(CSettings::SETTING_LOCALE_SHORTDATEFORMAT) ==
SETTING_REGIONAL_DEFAULT)
+ SetShortDateFormat(m_currentRegion->m_strDateFormatShort);
+--
+2.32.0.rc0
+
+
+From 114ee13138389c96a759d6e5b73717093dd4030d Mon Sep 17 00:00:00 2001
+From: Vasyl Gello
+Date: Sun, 27 Jun 2021 19:31:39 +
+Subject: [PATCH 2/2] kodi.sh.in: Unset LC_{ALL,CTYPE}, LANG
+
+Signed-off-by: Vasyl Gello
+---
+ tools/Linux/kodi.sh.in | 3 +++
+ 1 file changed, 3 insertions(+)
+
+diff --git a/tools/Linux/kodi.sh.in b/tools/Linux/kodi.sh.in
+index 108c0b007b..29d17d2c0f 100644
+--- a/tools/Linux/kodi.sh.in
b/tools/Linux/kodi.sh.in
+@@ -171,6 +171,9 @@ if command_exists gdb; then
+ fi
+ fi
+
++# Unset CTYPE, LANG and ALL - see issue #19883
++unset LC_CTYPE LC_ALL LANG
++
+ LOOP=1
+ while [ $(( $LOOP )) = "1" ]
+ do
+--
+2.32.0.rc0
+
diff -Nru kodi-19.1+dfsg2/debian/patches/series
kodi-19.1+dfsg2/debian/patches/series
--- kodi-19.1+dfsg2/debian/patches/series 2021-06-07 14:42:08.0
+
+++ kodi-19.1+dfsg2/debian/patches/series 2021-06-24 20:44:30.0
+
@@ -19,6 +19,7 @@
kodi/0019-Disable-GetCPUFrequency-test.patch
kodi/0020-Fix-C++-example-includes.patch
kodi/0021-Detect-and-honor-big-endian-arch.patch
+kodi/0022-Workaround-989814.patch
libdvdnav/0001-xbmc-dvdnav-allow-get-set-vm-state.patch
libdvdnav/0002-xbmc-dvdnav-expose-dvdnav_get_vm-dvdnav_get_button_i.patch
libdvdnav/0003-xbmc-dvdnav-detection-of-dvd-name.patch
