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

lib/command_duration: avoid relying on a specific locale #2271

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

akinomyoga
Copy link
Contributor

@akinomyoga akinomyoga commented Nov 18, 2024

Description

This fixes #2206 (and a duplicate #2269) as described in comments #2269 (comment) and #2206 (comment).

Describe your changes in detail---The original code in the master branch used the locale en_US.UTF-8 to ensure that the decimal point of EPOCHREALTIME is the period. However, the specific locale en_US.UTF-8 is not ensured to exist in the system. More specifically, the locale en_US.UTF-8 is the locale for English used in the United States. In other countries and regions, the locale installed in the system is probably the region's local language, and en_US.UTF-8 is not installed.

The standard solution in this kind of situation is to specify the locale C or POSIX. The locale C is ensured by the C and POSIX standards to be available in any environment conforming to the standards, and all the systems support C to the best of my knowledge. The existence of the locale POSIX is also ensured by the POSIX standard, but some environments do not support it (though most environments are supported). Also, there is essentially no difference from the locale C, so there is no reason to use the locale POSIX instead of C. With the locale C, the decimal point of the number in EPOCHREALTIME is ensured to be the period (ASCII \x2e) as desired.

I also added local to explicitly confine the scope of the temporary value of LC_ALL. Since the function _shell_duration_en is currently only used in a subshell, it doesn't affect the caller's context. However, if this function is called normally, it would break the locale of the global context. It is generally a good practice to design a function so that it doesn't break the environment even with unexpected usage.

Motivation and Context

Why is this change required?---This change is required to fix the locale error reported in #2206 and #2269. The locale error needs to be solved because it may cause unexpected behavior in the result of the measured command duration, and also the error message is simply annoying and confusing.

What problem does it solve?---This solves the problem of the locale error reported in #2206 and #2269.

If it fixes an open issue, please link to the issue here.---Fixes #2206 (and a duplicate #2269).

How Has This Been Tested?

Please describe in detail how you tested your changes.---I cannot directly reproduce the problem in my environment because I installed en_US.UTF-8 in my environment, but I can instead emulate the issue by setting another locale, which is not present in my system. I confirmed that I can reproduce the error message by rewriting the line to LC_ALL='it_IT.UTF-8':

bash: warning: setlocale: LC_ALL: cannot change locale (it_IT.UTF-8): No such file or directory

Then, I changed it to local LC_ALL=C and confirmed that the error message disappears.

Include details of your testing environment, and the tests you ran to---Here it is:
$ uname -a
Linux chatoyancy 6.5.12-100.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Mon Nov 20 22:28:44 UTC 2023 x86_64 GNU/Linux
$ ble/widget/display-shell-version
GNU bash, version 5.3.0(165)-alpha (x86_64-pc-linux-gnu) [Fedora Linux 39 (Server Edition)]
ble.sh, version 0.4.0-devel4+3e3d0d3e (noarch) [git 2.46.0, GNU Make 4.3, GNU Awk 5.2.2, API 3.2, PMA Avon 8-g1, (GNU MPFR 4.2.0-p12, GNU MP 6.2.1)]
bash-preexec, (hash:5f1208c33e624859eea70e3843bd9b8c9a06819e, 13046 bytes) (noarch)
bash-it, version +d3e0b3a1 (noarch), alias(), completion(), plugin(blesh)
locale: LANG=en_US.UTF-8
terminal: TERM=screen.xterm-256color wcwidth=16.0-emacs, screen:49900 (83;49900;0), contra:0 (99;0)
options: -emacs +vi +inherit_errexit +progcomp_alias
$ locale -a
C
C.utf8
POSIX
cs_CZ
cs_CZ.utf8
de_AT
de_AT.utf8
de_AT@euro
de_BE
de_BE.utf8
de_BE@euro
de_CH
de_CH.utf8
de_DE
de_DE.utf8
de_DE@euro
de_IT
de_IT.utf8
de_LI.utf8
de_LU
de_LU.utf8
de_LU@euro
en_AG
en_AU
en_AU.utf8
en_BW
en_BW.utf8
en_CA
en_CA.utf8
en_DK
en_DK.utf8
en_GB
en_GB.iso885915
en_GB.utf8
en_HK
en_HK.utf8
en_IE
en_IE.utf8
en_IE@euro
en_IL
en_IN
en_NG
en_NZ
en_NZ.utf8
en_PH
en_PH.utf8
en_SC.utf8
en_SG
en_SG.utf8
en_US
en_US.iso885915
en_US.utf8
en_ZA
en_ZA.utf8
en_ZM
en_ZW
en_ZW.utf8
fr_BE
fr_BE.utf8
fr_BE@euro
fr_CA
fr_CA.utf8
fr_CH
fr_CH.utf8
fr_FR
fr_FR.iso88591
fr_FR.utf8
fr_FR@euro
fr_LU
fr_LU.utf8
fr_LU@euro
ja_JP.eucjp
ja_JP.sjis
ja_JP.utf8
zh_CN
zh_CN.gb18030
zh_CN.gbk
zh_CN.utf8
zh_HK
zh_HK.big5hkscs
zh_HK.utf8
zh_SG
zh_SG.gbk
zh_SG.utf8
zh_TW
zh_TW.euctw
zh_TW.utf8

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
    • Note: this change doesn't require a documentation change.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
    • Note: I didn't add a new file.
  • I have added tests to cover my changes, and all the new and existing tests pass.

Copy link
Contributor

@seefood seefood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like a fix.
I'm not sure why we need to change the locale at all here, though. won't it make sense to use the user's existing locale in most cases?

@akinomyoga
Copy link
Contributor Author

looks like a fix. I'm not sure why we need to change the locale at all here, though. won't it make sense to use the user's existing locale in most cases?

We measure the command duration using the time obtained from EPOCHREALTIME, which is a clock with the resolution of a microsecond. The number obtained by EPOCHREALTIME is formatted using the current locale. If the user's locale is just used, the format of EPOCHREALTIME is unpredictable. We want to process the values obtained from EPOCHREALTIME to calculate the duration of command executions, so we need to normalize the format of EPOCHREALTIME to a known format. In particular, the decimal point in EPOCHREALTIME can be a comma or another character depending on the locale. For this reason, we cannot directly use the user's existing locale. This is the reason that we have the function _shell_duration_en in the first place; the function safely gets the value of EPOCHREALTIME with a known locale (that would be expected to exist in the system).

@seefood seefood merged commit 3c0baf5 into Bash-it:master Nov 19, 2024
3 of 6 checks passed
@akinomyoga akinomyoga deleted the fix-github-issue-2206 branch November 19, 2024 06:43
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.

bash: warning: setlocale: LC_ALL: cannot change locale (en_uS.UTF-8): No such file or directory
2 participants