Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I18N: Set C++ locale and format sizes according to the locale #1699

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ppisar
Copy link
Contributor

@ppisar ppisar commented Sep 13, 2024

Displaying a package size, an installation size, a download speed etc. formats decimal numbers to 1-digit precision after the decimal point (49.5 KiB). However, users expect the number to be formatted according their locale. E.g. in cs_CZ.UTF-8, it is "49,5 KiB".

DNF5 formats these values with fmt::format() which utilizes C++ locale if "L" formatting option is used.

C++ locale (std::locale::global()) and C locale (setlocale(), C++-wrapped as std::setlocale()) are two different things and DNF5 only has set the C locale up to now.

This patch starts setting C++ locale, which also implicitly sets C locale. This patch also modifies
libdnf5::cli::utils::units::format_size_aligned() to use the locale-dependent decimal seperator (available since fmt-8.0.0).

I manually tested dnf5 and dnf5daemon-client and they work for me.

Though there is a risk that the new C++ locale will affect some code uknown to me, like regular expression matching, or thread-specific locales. If the affected code was unfixable, we can resort to saving the desired C++ locale into a dedicated object accessible to format_size_aligned() and pass it explicitly for fmt::format. Thorough testing is welcome.

Related: #1687

Copy link

We were not able to find or create Copr project packit/rpm-software-management-dnf5-1699 specified in the config with the following error:

Packit received HTTP 500 Internal Server Error from Copr Service. Check the Copr status page: https://copr.fedorainfracloud.org/status/stats/, or ask for help in Fedora Build System matrix channel: https://matrix.to/#/#buildsys:fedoraproject.org.

Unless the HTTP status code above is >= 500, please check your configuration for:

  1. typos in owner and project name (groups need to be prefixed with @)
  2. whether the project name doesn't contain not allowed characters (only letters, digits, underscores, dashes and dots must be used)
  3. whether the project itself exists (Packit creates projects only in its own namespace)
  4. whether Packit is allowed to build in your Copr project
  5. whether your Copr project/group is not private

Displaying a package size, an installation size, a download speed etc.
formats decimal numbers to 1-digit precision after the decimal point
(49.5 KiB). However, users expect the number to be formatted according
their locale. E.g. in cs_CZ.UTF-8, it is "49,5 KiB".

DNF5 formats these values with fmt::format() which utilizes C++ locale
if "L" formatting option is used.

C++ locale (std::locale::global()) and C locale (setlocale(),
C++-wrapped as std::setlocale()) are two different things and DNF5
only has set the C locale up to now.

This patch starts setting C++ locale, which also implicitly sets
C locale. This patch also modifies
libdnf5::cli::utils::units::format_size_aligned() to use the
locale-dependent decimal seperator (available since fmt-8.0.0).

I manually tested dnf5 and dnf5daemon-client and they work for me.

Though there is a risk that the new C++ locale will affect some code
uknown to me, like regular expression matching, or thread-specific
locales. If the affected code was unfixable, we can resort to saving
the desired C++ locale into a dedicated object accessible to
format_size_aligned() and pass it explicitly for fmt::format. Thorough
testing is welcome.

Related: rpm-software-management#1687
@jrohel
Copy link
Contributor

jrohel commented Sep 18, 2024

I wonder how many inputs, outputs and regexes this PR will break.
We already have broken parsing of some configuration values for some locales. Probably from this #1408. And it's because parsing uses functions that depend on the C locale (at least std::stod is used for log_size, bandwidth, minrate, all options using class OptionSeconds and std::stof for throttle).
This PR breaks the parsing of other numeric configuration values for some locales (OptionNumber class uses operator >> for parsing).

I'm considering whether to merge this PR now or first prepare a PR to fix the bugs I mentioned above.

@ppisar
Copy link
Contributor Author

ppisar commented Sep 18, 2024

First we need to fix the parser.

@ppisar
Copy link
Contributor Author

ppisar commented Sep 18, 2024

Some locale bugs in the parser are tracked in #1706.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants