943 Local Setup - Kept Old US Local Setup In place - #1344
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new “Locale Setup” utility under the Utils tab to help users configure a UTF-8 locale (intended to address Debian’s default 12-hour time/locale experience discussed in #943), while keeping the existing “US Locale Setup” utility available.
Changes:
- Added a new Utils menu entry for “Locale Setup” pointing to a new
locale-setup.shscript. - Introduced
core/tabs/utils/locale-setup.shto detect/suggest a locale and generate it. - Updated the user guide walkthrough to mention the new utility.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| docs/content/userguide/walkthrough.md | Adds “Locale Setup” to the walkthrough list (currently with a small Markdown formatting issue). |
| core/tabs/utils/tab_data.toml | Registers the new “Locale Setup” utility in the Utils catalog (task flaging needs adjustment for privileged edits). |
| core/tabs/utils/locale-setup.sh | New script to suggest and apply a UTF-8 locale (needs hardening for idempotence, clearer failure behavior, and safer network call). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…timeout for iso lookup and defaulted to US if fails, addd conditional statements for modifying locale based on Copilot suggestions
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an interactive locale setup script that detects a suggested locale, validates input, updates system locale configuration, runs ChangesLocale Setup Utility
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LocaleSetup as locale-setup.sh
participant Ifconfig as ifconfig.io
participant SystemFiles as /etc locale files
participant LocaleGen as locale-gen
LocaleSetup->>Ifconfig: Fetch country_code
Ifconfig-->>LocaleSetup: Return country code
LocaleSetup->>User: Prompt with suggested locale
User-->>LocaleSetup: Accept or enter locale
LocaleSetup->>SystemFiles: Update locale configuration
LocaleSetup->>LocaleGen: Generate selected locale
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
core/tabs/utils/locale-setup.sh (1)
12-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
printfinstead ofecho.Replace the locale-map and error-message
echocalls withprintffor portable shell output. As per coding guidelines, “useprintfrather thanechofor portable formatted output.”Also applies to: 72-72, 90-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/tabs/utils/locale-setup.sh` around lines 12 - 46, Replace every echo invocation in the locale mapping, fallback, and associated error-message paths with portable printf calls, including the locations referenced around lines 72 and 90. Preserve each existing locale value, fallback behavior, and error-message content while ensuring formatting remains equivalent.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/tabs/utils/locale-setup.sh`:
- Around line 12-46: Correct the country-to-locale mappings in the locale
selection case statement: use `gb` for `en_GB.UTF-8` instead of `uk`, map `ch`
to the appropriate Swiss locale instead of falling back, map `sg` to Singapore’s
locale rather than `de_CH.UTF-8`, and update `cf` to the Central African
Republic locale. Preserve all other mappings and the existing default fallback.
- Around line 61-76: Harden the LOCALE validation loop in locale setup to accept
C.UTF-8, require a safe locale-character whitelist that excludes
command-injection metacharacters, and verify the locale is available before any
/etc/environment update. Preserve the existing format validation for valid UTF-8
locales, then only pass validated values to the escalated sed commands.
In `@core/tabs/utils/tab_data.toml`:
- Around line 151-155: Add a command_exists precondition to the Locale Setup
entry in the tab data, checking for locale-gen before displaying or running
locale-setup.sh; follow the existing utility entries’ condition syntax and leave
the task metadata unchanged.
---
Nitpick comments:
In `@core/tabs/utils/locale-setup.sh`:
- Around line 12-46: Replace every echo invocation in the locale mapping,
fallback, and associated error-message paths with portable printf calls,
including the locations referenced around lines 72 and 90. Preserve each
existing locale value, fallback behavior, and error-message content while
ensuring formatting remains equivalent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d497e908-958e-40f5-af9e-b88ccd0f11ec
📒 Files selected for processing (3)
core/tabs/utils/locale-setup.shcore/tabs/utils/tab_data.tomldocs/content/userguide/walkthrough.md
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a729863eaa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #!/bin/sh -e | ||
|
|
||
| . ../common-script.sh | ||
| . ../common-service-script.sh |
There was a problem hiding this comment.
On systems that provide locale-gen but lack systemctl, rc-service, and sv—such as minimal containers or chroots—sourcing this helper immediately calls checkInitManager and exits before the locale prompt, even though this utility does not manage services. Source only common-script.sh here so every environment admitted by the catalog precondition can run the utility.
AGENTS.md reference: AGENTS.md:L87-L87
Useful? React with 👍 / 👎.
| script = "locale-setup.sh" | ||
| task_list = "PFM" |
There was a problem hiding this comment.
Because multi_select defaults to true, this state-changing utility can be queued with other commands even though it pauses for interactive input and modifies the machine's global locale. Mark the entry multi_select = false so it must be run independently as required for interactive and state-dependent operations.
AGENTS.md reference: AGENTS.md:L114-L115
Useful? React with 👍 / 👎.
| case "$LOCALE" in | ||
| *[![:alnum:]_.@-]*|'') ;; | ||
| *.UTF-8) break ;; | ||
| esac |
There was a problem hiding this comment.
For any syntactically plausible but unsupported input such as en_UK.UTF-8, this check accepts the value, after which the script writes it to /etc/environment, /etc/locale.gen, and /etc/locale.conf before locale-gen reports failure. The failed run therefore leaves the system configured with an invalid locale; verify the value against the installed locale definitions before modifying any files.
Useful? React with 👍 / 👎.
| fi | ||
| } | ||
|
|
||
| checkEnv |
There was a problem hiding this comment.
On an Arch system without yay or paru, this unconditional checkEnv call reaches checkAURHelper, installs base-devel and git with pacman, clones yay-bin, and installs it before showing the locale prompt. That unrelated privileged package-management work is not disclosed by this utility's description or its sole PFM flag; initialize only the checks needed for locale configuration, or make AUR setup opt-in.
AGENTS.md reference: AGENTS.md:L110-L115
Useful? React with 👍 / 👎.
| if grep -q '^LC_ALL=' /etc/environment 2>/dev/null; then | ||
| "$ESCALATION_TOOL" sed -i "s/^LC_ALL=.*/LC_ALL=${LOCALE}/" /etc/environment | ||
| else | ||
| printf 'LC_ALL=%s\n' "$LOCALE" | "$ESCALATION_TOOL" tee -a /etc/environment >/dev/null |
There was a problem hiding this comment.
For every successful run, writing LC_ALL to /etc/environment makes it override LANG and all category-specific LC_* settings in subsequent login sessions. Users therefore cannot change language or regional categories through desktop settings or per-user environment configuration; persist LANG as the default and leave LC_ALL unset.
Useful? React with 👍 / 👎.
| et) printf "%b" "et_EE.UTF-8" ;; | ||
| fa) printf "%b" "fa_IR.UTF-8" ;; |
There was a problem hiding this comment.
The lookup returns ISO country codes, but fa and et are language codes rather than the country codes for Iran (ir) and Estonia (ee). Consequently Iranian and Estonian users fall through to en_US.UTF-8, while an IP geolocated to Ethiopia (ET) is incorrectly offered et_EE.UTF-8; use the corresponding country-code keys.
Useful? React with 👍 / 👎.
| [[data]] | ||
| name = "Locale Setup" |
There was a problem hiding this comment.
The top-level utility entries are ordered alphabetically, but inserting Locale Setup after Numlock on Startup places it out of sequence and propagates that ordering to the TUI and generated guide. Move this entry before Numlock or run the repository's TOML sorter.
AGENTS.md reference: AGENTS.md:L147-L148
Useful? React with 👍 / 👎.
Type of Change
Description
Added option for new Local Setup (UTF-8 only). Kept original US locale setup in place.
Issues / other PRs related
Screenshots (if applicable)