Bash-it / bash-it

A community Bash framework.
MIT License
14.3k stars 2.29k forks source link

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

Closed akinomyoga closed 4 days ago

akinomyoga commented 5 days ago

Description

This fixes #2206 (and a duplicate #2269) as described in comments https://github.com/Bash-it/bash-it/issues/2269#issuecomment-2478178460 and https://github.com/Bash-it/bash-it/issues/2206#issuecomment-2479428229.

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: ```console $ 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

Checklist:

akinomyoga commented 5 days ago

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).