[UI redesign 1/3] rog-control-center: build .mo from .po at build time - #252
[UI redesign 1/3] rog-control-center: build .mo from .po at build time#252NB-Group wants to merge 1 commit into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe build script compiles existing gettext ChangesLocale build integration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Tools execution failed with the following error: Failed to run tools: 14 UNAVAILABLE: Connection dropped 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: 2
🤖 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 `@rog-control-center/build.rs`:
- Around line 14-20: Update the locale processing around the po path
construction to look for rog-control-center.po directly under entry.path(), not
inside LC_MESSAGES. Ensure the LC_MESSAGES directory is created with
create_dir_all before invoking msgfmt, then write the generated
rog-control-center.mo there while preserving the existing command flow.
- Around line 12-13: Update the translation build logic around the translations
directory iteration to locate sources at
translations/<locale>/rog-control-center.po instead of under LC_MESSAGES, invoke
msgfmt for each locale, and propagate filesystem and compilation failures. Treat
only ErrorKind::NotFound when starting msgfmt as optional; fail for other
process errors and non-zero exit statuses, and remove stale .mo files or replace
them only after successful compilation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f7a25478-00d4-467d-b781-3b8ed8f572d7
📒 Files selected for processing (11)
.gitignorerog-control-center/build.rsrog-control-center/translations/az/LC_MESSAGES/rog-control-center.morog-control-center/translations/en/LC_MESSAGES/rog-control-center.morog-control-center/translations/fr/LC_MESSAGES/rog-control-center.morog-control-center/translations/it/LC_MESSAGES/rog-control-center.morog-control-center/translations/pt_BR/LC_MESSAGES/rog-control-center.morog-control-center/translations/ru/LC_MESSAGES/rog-control-center.morog-control-center/translations/tr/LC_MESSAGES/rog-control-center.morog-control-center/translations/uk_UA/LC_MESSAGES/rog-control-center.morog-control-center/translations/zh_CN/LC_MESSAGES/rog-control-center.mo
📜 Review details
🔇 Additional comments (2)
rog-control-center/build.rs (1)
27-28: LGTM!.gitignore (1)
31-33: LGTM!
| if let Ok(entries) = std::fs::read_dir(&translations_dir) { | ||
| for entry in entries.flatten() { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- build.rs ---'
cat -n rog-control-center/build.rs
printf '%s\n' '--- related references ---'
rg -n -C 3 'translations_dir|msgfmt|read_dir|\.mo|\.po' rog-control-center Cargo.toml .github 2>/dev/null || trueRepository: OpenGamingCollective/asusctl
Length of output: 32942
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- translation file inventory ---'
find rog-control-center/translations -maxdepth 4 -type f -printf '%p %s bytes\n' 2>/dev/null | sort || true
printf '%s\n' '--- tracked translation files ---'
git ls-files 'rog-control-center/translations/**'
printf '%s\n' '--- ignore rules ---'
rg -n -C 2 '(^|/)(translations|.*\.mo)|\.mo' .gitignore '**/.gitignore' 2>/dev/null || true
printf '%s\n' '--- build-script call sites and Cargo metadata ---'
rg -n -C 3 'compile_locales|build *= *"build\.rs"|build\.rs|CARGO_MANIFEST_DIR' rog-control-center/Cargo.toml rog-control-center/build.rs Cargo.toml 2>/dev/null || trueRepository: OpenGamingCollective/asusctl
Length of output: 2785
🌐 Web query:
GNU gettext msgfmt manual output file behavior exit status invalid PO file
💡 Result:
The behavior of the GNU msgfmt utility regarding exit status and output file generation for invalid PO files is governed by the severity and nature of the errors encountered [1]. Exit Status When msgfmt encounters fatal errors during the parsing or processing of a PO file, it is designed to terminate with a non-zero exit status, typically EXIT_FAILURE (1) [1]. Fatal errors include issues such as invalid file syntax, missing or malformed header entries (when checks are enabled), or conflicts detected during mandatory validation [1][2]. The utility explicitly increments an error count and sets the exit status if errors are found during processes like message list checking [1]. Output File Behavior If msgfmt detects fatal errors that result in a non-zero exit status, it typically does not produce a valid binary MO file [1][3]. While specific behavior can sometimes depend on the version and the nature of the error, the standard expectation is that upon encountering fatal parsing or consistency errors, the tool will report the errors to standard error and fail to create or update the output file [1][2]. In cases where msgfmt exits with a status code of 0 but no output file is created (or an empty file is produced), it is often due to the following reasons: 1. Invalid PO Content: The input file may be technically syntactically correct enough to parse, but logically empty or missing required components (such as a valid header entry), leading the tool to have nothing to write to the MO file [3]. 2. Missing Translations: If a PO file consists entirely of empty or fuzzy translations (and the --use-fuzzy or -f flag is not specified), msgfmt will not include these in the final MO file, potentially resulting in an empty output or no output at all [3][2]. 3. Configuration: In some contexts, if checks fail but are not treated as fatal by the specific flags provided, the tool may exit with 0, but the output file may not be generated as expected [2]. To ensure robust error detection, it is recommended to use the --check or related flags (such as --check-format and --check-header) [4][5]. These flags force stricter validation of the PO file structure and contents, ensuring that the utility provides clear diagnostic messages and a non-zero exit status when inconsistencies are identified [4][2].
Citations:
- 1: https://github.com/dd32/gettext/blob/master/gettext-tools/src/msgfmt.c
- 2: https://pubs.opengroup.org/onlinepubs/9799919799/utilities/msgfmt.html
- 3: https://stackoverflow.com/questions/25777182/the-gettext-msgfmt-command-exits-with-status-code-0-but-does-not-write-an-mo
- 4: https://man7.org/linux/man-pages/man1/msgfmt.1.html
- 5: https://manpages.debian.org/unstable/gettext/msgfmt.1.en.html
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
build = Path("rog-control-center/build.rs").read_text()
checks = {
"read_dir errors are ignored": "if let Ok(entries) = std::fs::read_dir" in build,
"directory-entry errors are discarded": "entries.flatten()" in build,
"process status result is discarded": "let _ = std::process::Command::new" in build
and ".status();" in build,
"locale compilation returns no error": "fn compile_locales()" in build
and "fn compile_locales() -> Result" not in build,
}
for label, result in checks.items():
print(f"{label}: {'present' if result else 'absent'}")
translations = Path("rog-control-center/translations")
locale_dirs = sorted(p for p in translations.iterdir() if p.is_dir())
print(f"locale directories: {len(locale_dirs)}")
for locale in locale_dirs:
po = locale / "LC_MESSAGES" / "rog-control-center.po"
mo = locale / "LC_MESSAGES" / "rog-control-center.mo"
print(f"{locale}: po={'present' if po.exists() else 'absent'}, mo={'present' if mo.exists() else 'absent'}")
PYRepository: OpenGamingCollective/asusctl
Length of output: 882
Fix the locale source path and report compilation failures.
The tracked .po files are under translations/<locale>/rog-control-center.po, but this code searches under LC_MESSAGES, so it never invokes msgfmt. Handle filesystem and process errors, treat only ErrorKind::NotFound for msgfmt as optional, and fail on other errors or non-zero statuses. Remove stale .mo files or replace them only after successful compilation.
🤖 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 `@rog-control-center/build.rs` around lines 12 - 13, Update the translation
build logic around the translations directory iteration to locate sources at
translations/<locale>/rog-control-center.po instead of under LC_MESSAGES, invoke
msgfmt for each locale, and propagate filesystem and compilation failures. Treat
only ErrorKind::NotFound when starting msgfmt as optional; fail for other
process errors and non-zero exit statuses, and remove stale .mo files or replace
them only after successful compilation.
b502200 to
ce8fbfa
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.gitignore:
- Around line 31-33: Replace the broad *.mo pattern in .gitignore with a
path-specific rule that matches only generated .mo files under
rog-control-center/translations, preserving tracking for unrelated locale files
elsewhere in the repository.
In `@rog-control-center/build.rs`:
- Around line 9-15: Update compile_locales to return a Result and propagate
read_dir, directory-entry, create_dir_all, and file-operation errors instead of
discarding them; make non-zero msgfmt status return an error rather than only
warn. Ensure generated .mo catalogs are removed or atomically replaced when
compilation fails or a translation source is removed, and update the caller to
handle the propagated result.
- Around line 11-12: Update compile_locales to emit a cargo:rerun-if-changed
directive for translations_dir before reading the directory, ensuring Cargo
reruns the build script when any PO translation source changes. Leave the
existing directory-reading and locale compilation flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ef3fd0cc-1c3c-4603-bb17-e07891730a97
📒 Files selected for processing (11)
.gitignorerog-control-center/build.rsrog-control-center/translations/az/LC_MESSAGES/rog-control-center.morog-control-center/translations/en/LC_MESSAGES/rog-control-center.morog-control-center/translations/fr/LC_MESSAGES/rog-control-center.morog-control-center/translations/it/LC_MESSAGES/rog-control-center.morog-control-center/translations/pt_BR/LC_MESSAGES/rog-control-center.morog-control-center/translations/ru/LC_MESSAGES/rog-control-center.morog-control-center/translations/tr/LC_MESSAGES/rog-control-center.morog-control-center/translations/uk_UA/LC_MESSAGES/rog-control-center.morog-control-center/translations/zh_CN/LC_MESSAGES/rog-control-center.mo
📜 Review details
🔇 Additional comments (1)
rog-control-center/build.rs (1)
16-25: LGTM!
| # Compiled gettext binaries — .po sources are tracked, .mo are build artifacts | ||
| *.mo | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Scope the .mo ignore rule to control-center catalogs.
The unanchored *.mo pattern ignores every matching file below the repository root, not only generated files under rog-control-center/translations. This can hide unrelated locale files. Git documents that patterns without a slash can match at any level. (git-scm.com)
Use a path-specific rule.
Proposed fix
-*.mo
+/rog-control-center/translations/*/LC_MESSAGES/rog-control-center.mo📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Compiled gettext binaries — .po sources are tracked, .mo are build artifacts | |
| *.mo | |
| # Compiled gettext binaries — .po sources are tracked, .mo are build artifacts | |
| /rog-control-center/translations/*/LC_MESSAGES/rog-control-center.mo | |
🤖 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 @.gitignore around lines 31 - 33, Replace the broad *.mo pattern in
.gitignore with a path-specific rule that matches only generated .mo files under
rog-control-center/translations, preserving tracking for unrelated locale files
elsewhere in the repository.
| fn compile_locales() { | ||
| let root = env!("CARGO_MANIFEST_DIR"); | ||
| let translations_dir = PathBuf::from(root).join("translations"); | ||
| let Ok(entries) = std::fs::read_dir(&translations_dir) else { | ||
| return; | ||
| }; | ||
| for entry in entries.flatten() { |
There was a problem hiding this comment.
Propagate locale compilation failures.
The path fix is present, but the previous error-handling issue remains. read_dir and individual directory-entry errors are discarded. create_dir_all errors are ignored. A non-zero msgfmt status only emits a warning. An old ignored .mo can remain after a failed or removed translation source.
Return a Result, propagate filesystem errors, fail on non-zero msgfmt status, and remove or atomically replace generated catalogs.
Also applies to: 26-38, 42-43
🤖 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 `@rog-control-center/build.rs` around lines 9 - 15, Update compile_locales to
return a Result and propagate read_dir, directory-entry, create_dir_all, and
file-operation errors instead of discarding them; make non-zero msgfmt status
return an error rather than only warn. Ensure generated .mo catalogs are removed
or atomically replaced when compilation fails or a translation source is
removed, and update the caller to handle the propagated result.
| let translations_dir = PathBuf::from(root).join("translations"); | ||
| let Ok(entries) = std::fs::read_dir(&translations_dir) else { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- build.rs ---'
cat -n rog-control-center/build.rs
printf '%s\n' '--- build-script related configuration ---'
rg -n -C 4 'slint_build|compile_with_config|rerun-if-changed|compile_locales|translations|gettext|po|mo' rog-control-center Cargo.toml Cargo.lock 2>/dev/null || true
printf '%s\n' '--- relevant tracked files ---'
git ls-files 'rog-control-center/*' | rg '(^|/)(build\.rs|.*\.(po|mo)|Cargo\.toml)$' || trueRepository: OpenGamingCollective/asusctl
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- build.rs ---'
cat -n rog-control-center/build.rs
printf '%s\n' '--- build-script references ---'
rg -n -C 5 'slint_build|compile_with_config|rerun-if-changed|compile_locales|translations|gettext|po|mo' rog-control-center Cargo.toml Cargo.lock 2>/dev/null || true
printf '%s\n' '--- relevant tracked files ---'
git ls-files 'rog-control-center/*' | rg '(^|/)(build\.rs|.*\.(po|mo)|Cargo\.toml)$' || trueRepository: OpenGamingCollective/asusctl
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- rog-control-center/build.rs ---'
sed -n '1,140p' rog-control-center/build.rs
printf '%s\n' '--- rog-control-center/Cargo.toml build configuration ---'
rg -n -C 8 'build\s*=|slint-build|gettext|compile' rog-control-center/Cargo.toml
printf '%s\n' '--- build-script APIs and rerun directives ---'
rg -n -C 6 'slint_build|compile_with_config|rerun-if-changed|compile_locales' --glob '!rog-control-center/translations/**' --glob '!Cargo.lock' .
printf '%s\n' '--- translation files ---'
git ls-files 'rog-control-center/translations/**' | sed -n '1,120p'
printf '%s\n' '--- generated MO files ---'
git ls-files | rg '(^|/)[^/]+\.mo$' || trueRepository: OpenGamingCollective/asusctl
Length of output: 6303
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- pinned Slint revision ---'
rg -n -A12 -B3 '^name = "slint-build"$|^name = "slint"$' Cargo.lock
printf '%s\n' '--- local Cargo source cache candidates ---'
find "${HOME}/.cargo" -maxdepth 5 -type f \( -path '*slint-build*/src/*.rs' -o -name 'build.rs' \) 2>/dev/null | sed -n '1,80p' || trueRepository: OpenGamingCollective/asusctl
Length of output: 8046
🌐 Web query:
Cargo build scripts rerun-if-changed directory recursive behavior slint_build compile_with_config source
💡 Result:
In Cargo, the cargo:rerun-if-changed=PATH instruction directs Cargo to re-run a build script if the specified path changes [1][2]. When this path points to a directory, Cargo performs a recursive scan of the directory to detect modifications [1][3]. Cargo monitors for changes in file timestamps ("mtime"), and if any file or sub-directory within the specified path is modified, added, or deleted, the build script will be re-run [1][3]. This recursive behavior ensures that changes to nested source files or resources are correctly detected [3]. Regarding slint_build::compile_with_config in the Slint crate: This function is used within a build.rs script to compile .slint files [4]. As part of its operation, it internally tracks input files and dependencies associated with the Slint markup [5][6]. It automatically emits cargo:rerun-if-changed instructions for these identified dependencies to ensure the build script triggers a re-compilation whenever those specific files change [5][6]. If you experience unexpected or redundant rebuilds, it may be due to how these paths are being tracked or if they point to non-existent or invalid locations, which can cause Cargo to consider the build "dirty" [7]. Top results: [1][3][5][7]
Citations:
- 1: https://doc.rust-lang.org/cargo/reference/build-scripts.html?highlight=red+zone
- 2: https://doc.rust-lang.org/cargo/reference/build-scripts.html
- 3: Check if rerun-if-changed points to a directory. rust-lang/cargo#8973
- 4: https://docs.slint.dev/latest/docs/rust/slint_build/
- 5: https://docs.slint.dev/latest/docs/rust/src/slint_build/lib.rs
- 6: https://docs.rs/slint-build/latest/src/slint_build/lib.rs.html
- 7: Embedding fonts with EmbedForSoftwareRenderer causes a build to run every time slint-ui/slint#11360
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact Slint lockfile entries ---'
awk '
/^name = "(slint|slint-build)"$/ {show=1; n=0}
show {print; n++}
show && n >= 14 {show=0}
' Cargo.lockRepository: OpenGamingCollective/asusctl
Length of output: 844
🌐 Web query:
site:github.com/slint-ui/slint compile_with_config rerun-if-changed slint_build
💡 Result:
When using slint_build::compile_with_config in a Rust build.rs script, you generally do not need to manually call cargo:rerun-if-changed for your primary .slint file or its direct dependencies, as the slint-build crate automatically tracks and emits these instructions to Cargo [1]. Key details regarding build script behavior with Slint: 1. Automatic Tracking: The slint-build crate internally scans for dependencies (including imported .slint files and embedded resources) and emits the necessary cargo:rerun-if-changed lines to Cargo [1]. This ensures that your build script re-runs automatically when any of these files change [1][2]. 2. Manual Configuration: If you have additional files or resources that are not automatically detected by the Slint compiler—such as custom asset folders or configuration files—you can manually add them to your build.rs using the standard Cargo build script command [2]: println!("cargo:rerun-if-changed=path/to/your/file"); 3. Environment Variables: slint-build also handles common environment variables that affect the build (e.g., SLINT_STYLE, SLINT_ASSET_SECTION) by emitting cargo:rerun-if-env-changed instructions [1][3]. If your custom logic relies on other environment variables, you should emit these manually as well: println!("cargo:rerun-if-env-changed=YOUR_ENV_VAR"); 4. Troubleshooting Excessive Rebuilds: If you experience issues where the build script reruns unnecessarily, verify that your paths are absolute and that they do not contain dynamic elements (like incorrectly formatted paths or temporary file paths) that change across builds [3]. Common causes for "dirty" builds include misconfigured font paths or cache-invalidating environment variable changes [3][4]. For most standard projects, simply calling slint_build::compile_with_config is sufficient to keep your build in sync with your UI file changes [5][6][7].
Citations:
- 1: https://github.com/slint-ui/slint/blob/507b650e/api/rs/build/lib.rs
- 2: Cannot select anything in ComboBox widgets slint-ui/slint#4033
- 3: Embedding fonts with EmbedForSoftwareRenderer causes a build to run every time slint-ui/slint#11360
- 4: rerun-if-env-changed=SLINT_GENERATED_INCLUDE_DIR slint-ui/slint#5045
- 5: https://github.com/slint-ui/slint/blob/582433b8/demos/usecases/rust/build.rs
- 6: https://github.com/slint-ui/slint/blob/master/api/rs/slint/mcu.md
- 7: https://github.com/slint-ui/slint/blob/0917b6cd/api/rs/slint/mcu.md
🏁 Script executed:
#!/bin/bash
set -eu
url='https://raw.githubusercontent.com/slint-ui/slint/2c94caa2b439c8b4476a63866b00b3001307aad1/api/rs/build/lib.rs'
tmp=$(mktemp)
trap 'rm -f "$tmp"' EXIT
curl --fail --silent --show-error "$url" > "$tmp"
printf '%s\n' '--- rerun directives in pinned slint-build source ---'
rg -n -C 8 'rerun-if-changed|compile_with_config|print_rustc_flags' "$tmp"
printf '%s\n' '--- Cargo documentation wording ---'
curl --fail --silent --show-error 'https://doc.rust-lang.org/cargo/reference/build-scripts.html' |
sed -n '/rerun-if-changed/,/rerun-if-env-changed/p' |
sed -n '1,100p'Repository: OpenGamingCollective/asusctl
Length of output: 12535
Register the translation sources with Cargo.
compile_locales does not emit a cargo:rerun-if-changed directive for the .po files. slint_build::compile_with_config emits selective directives for Slint dependencies, so Cargo can skip the build script after a PO-only change. The generated .mo file can remain stale.
Register the translation directory:
Proposed minimal change
let root = env!("CARGO_MANIFEST_DIR");
let translations_dir = PathBuf::from(root).join("translations");
+ println!("cargo:rerun-if-changed={}", translations_dir.display());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let translations_dir = PathBuf::from(root).join("translations"); | |
| let Ok(entries) = std::fs::read_dir(&translations_dir) else { | |
| let root = env!("CARGO_MANIFEST_DIR"); | |
| let translations_dir = PathBuf::from(root).join("translations"); | |
| println!("cargo:rerun-if-changed={}", translations_dir.display()); | |
| let Ok(entries) = std::fs::read_dir(&translations_dir) else { |
🤖 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 `@rog-control-center/build.rs` around lines 11 - 12, Update compile_locales to
emit a cargo:rerun-if-changed directive for translations_dir before reading the
directory, ensuring Cargo reruns the build script when any PO translation source
changes. Leave the existing directory-reading and locale compilation flow
unchanged.
|
I already had a PR open for it #243 but I can close it without problem. remind to .gitignore the .mo file: this way we can compile them when needed |
ce8fbfa to
4ccc41d
Compare
|
@scardracs cheers.Do ahead and close #243. This one already gitignores the |
4ccc41d to
3e371ff
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
rog-control-center/build.rs (2)
9-12: 🗄️ Data Integrity & Integration | 🟠 MajorRegister translation sources with Cargo.
The custom locale scan does not emit
cargo:rerun-if-changed. A.poedit or a new locale can therefore leave the generated.mounchanged during an incremental build.Add the directive before
read_dir:Proposed fix
let translations_dir = PathBuf::from(root).join("translations"); + println!("cargo:rerun-if-changed={}", translations_dir.display()); let Ok(entries) = std::fs::read_dir(&translations_dir) else {🤖 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 `@rog-control-center/build.rs` around lines 9 - 12, Update compile_locales to emit a cargo:rerun-if-changed directive for translations_dir before calling std::fs::read_dir, so Cargo rebuilds when locale files change or are added.
12-15: 🗄️ Data Integrity & Integration | 🟠 MajorPropagate locale compilation failures instead of logging them.
The build script discards filesystem errors and treats failed
msgfmtexecution as success.compile_locales()returns(), so Line 43 cannot stop the build. A failed or removed source can leave an ignored stale.mofile.Return a
Result, propagate filesystem errors, keep onlyErrorKind::NotFoundfor an unavailablemsgfmtas the optional case, and fail on other process errors or non-zero statuses. Generate into a temporary file and rename it only after success. Remove generated catalogs whose source files no longer exist.Minimal propagation shape
-fn compile_locales() { +fn compile_locales() -> Result<(), Box<dyn std::error::Error>> { @@ - let Ok(entries) = std::fs::read_dir(&translations_dir) else { - return; - }; - for entry in entries.flatten() { + let entries = std::fs::read_dir(&translations_dir)?; + for entry in entries { + let entry = entry?; @@ - if !po.exists() { - continue; - } + match std::fs::metadata(&po) { + Ok(metadata) if metadata.is_file() => {} + Ok(_) => continue, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => continue, + Err(e) => return Err(e.into()), + } @@ - let _ = std::fs::create_dir_all(mo.parent().unwrap_or(entry.path().as_path())); + std::fs::create_dir_all(mo.parent().unwrap_or(entry.path().as_path()))?; @@ - println!("cargo:warning=msgfmt failed for {} (status {status})", po.display()); + return Err(format!("msgfmt failed for {} (status {status})", po.display()).into()); @@ - println!("cargo:warning=msgfmt could not run for {}: {e}", po.display()); + return Err(e.into()); @@ + Ok(()) } @@ - compile_locales(); + compile_locales()?;Also applies to: 19-37, 43-43
🤖 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 `@rog-control-center/build.rs` around lines 12 - 15, Update compile_locales() to return and propagate a Result: surface translations directory and entry filesystem errors, treat only msgfmt ErrorKind::NotFound as optional, and fail on other execution errors or non-zero statuses. Compile each catalog into a temporary file and rename it only after successful completion, remove stale generated catalogs when their source files are absent, and update the caller at line 43 to propagate the result so failures stop the build.
🤖 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.
Duplicate comments:
In `@rog-control-center/build.rs`:
- Around line 9-12: Update compile_locales to emit a cargo:rerun-if-changed
directive for translations_dir before calling std::fs::read_dir, so Cargo
rebuilds when locale files change or are added.
- Around line 12-15: Update compile_locales() to return and propagate a Result:
surface translations directory and entry filesystem errors, treat only msgfmt
ErrorKind::NotFound as optional, and fail on other execution errors or non-zero
statuses. Compile each catalog into a temporary file and rename it only after
successful completion, remove stale generated catalogs when their source files
are absent, and update the caller at line 43 to propagate the result so failures
stop the build.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 73307a7c-3d05-4e35-9902-9e0ae5fd55a1
📒 Files selected for processing (11)
.gitignorerog-control-center/build.rsrog-control-center/translations/az/LC_MESSAGES/rog-control-center.morog-control-center/translations/en/LC_MESSAGES/rog-control-center.morog-control-center/translations/fr/LC_MESSAGES/rog-control-center.morog-control-center/translations/it/LC_MESSAGES/rog-control-center.morog-control-center/translations/pt_BR/LC_MESSAGES/rog-control-center.morog-control-center/translations/ru/LC_MESSAGES/rog-control-center.morog-control-center/translations/tr/LC_MESSAGES/rog-control-center.morog-control-center/translations/uk_UA/LC_MESSAGES/rog-control-center.morog-control-center/translations/zh_CN/LC_MESSAGES/rog-control-center.mo
📜 Review details
🔇 Additional comments (1)
.gitignore (1)
31-33: LGTM!
…ated .mo Add a msgfmt step in build.rs that compiles each .po into a .mo next to it, so translations are rebuilt from source instead of carrying stale binaries. Remove the checked-in .mo files and gitignore them.
3e371ff to
dfb0269
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
.gitignore (1)
31-33:⚠️ Potential issue | 🟡 MinorScope the
.moignore rule to control-center catalogs.Line 33 ignores every
.mofile at any depth. This can hide unrelated locale catalogs elsewhere in the repository. Use a path-specific rule.Proposed fix
-*.mo +/rog-control-center/translations/*/LC_MESSAGES/rog-control-center.mo🤖 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 @.gitignore around lines 31 - 33, Update the .gitignore rule for compiled gettext binaries so it matches only .mo catalogs under rog-control-center, rather than every .mo file in the repository. Preserve the explanatory comments and use a path-specific pattern rooted at the control-center directory.rog-control-center/build.rs (2)
9-12:⚠️ Potential issue | 🟠 MajorRegister
.poinputs with Cargo.
compile_localesdoes not register translation sources withcargo:rerun-if-changed. A PO-only change can therefore leaverog-control-center.mostale when Slint’s build integration uses selective rerun tracking.Register source changes and handle added or removed locales. Because this function writes generated files below
translations, do not watch the entire tree unless generated outputs are excluded. Use source-only tracking or move generated catalogs to a build-output directory.🤖 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 `@rog-control-center/build.rs` around lines 9 - 12, Update compile_locales to emit cargo:rerun-if-changed directives for every source .po file under translations, including added and removed locale inputs, while excluding generated outputs from tracking. Ensure changes to any registered PO source trigger regeneration without watching the entire generated-containing tree.
12-15:⚠️ Potential issue | 🟠 MajorPropagate locale compilation failures.
Line 12 treats every
read_dirfailure as success. Line 15 drops directory-entry errors. Line 20 treats metadata errors as a missing source. Line 26 dropscreate_dir_allfailures. Lines 28-36 only warn aftermsgfmtfails, and Line 43 cannot propagate an error.A failed build can leave an old or partial
.mo. Removing a.poalso leaves its old.mo. Return aResult, propagate filesystem errors, keep only a missingmsgfmtexecutable optional, fail on non-zero status, and clean or atomically replace generated catalogs.Also applies to: 20-22, 26-38, 42-43
🤖 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 `@rog-control-center/build.rs` around lines 12 - 15, Update the locale compilation function and its caller to return and propagate Result errors instead of treating failures as success. Propagate read_dir, directory-entry, metadata, create_dir_all, and msgfmt execution errors; only allow a missing msgfmt executable to remain optional, while non-zero status must fail the build. Remove stale .mo files when sources are missing, and generate catalogs via a temporary file followed by cleanup/atomic replacement so failed builds cannot leave partial outputs.
🤖 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.
Duplicate comments:
In @.gitignore:
- Around line 31-33: Update the .gitignore rule for compiled gettext binaries so
it matches only .mo catalogs under rog-control-center, rather than every .mo
file in the repository. Preserve the explanatory comments and use a
path-specific pattern rooted at the control-center directory.
In `@rog-control-center/build.rs`:
- Around line 9-12: Update compile_locales to emit cargo:rerun-if-changed
directives for every source .po file under translations, including added and
removed locale inputs, while excluding generated outputs from tracking. Ensure
changes to any registered PO source trigger regeneration without watching the
entire generated-containing tree.
- Around line 12-15: Update the locale compilation function and its caller to
return and propagate Result errors instead of treating failures as success.
Propagate read_dir, directory-entry, metadata, create_dir_all, and msgfmt
execution errors; only allow a missing msgfmt executable to remain optional,
while non-zero status must fail the build. Remove stale .mo files when sources
are missing, and generate catalogs via a temporary file followed by
cleanup/atomic replacement so failed builds cannot leave partial outputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 54219e79-966c-4817-9333-48481fbd8d34
📒 Files selected for processing (11)
.gitignorerog-control-center/build.rsrog-control-center/translations/az/LC_MESSAGES/rog-control-center.morog-control-center/translations/en/LC_MESSAGES/rog-control-center.morog-control-center/translations/fr/LC_MESSAGES/rog-control-center.morog-control-center/translations/it/LC_MESSAGES/rog-control-center.morog-control-center/translations/pt_BR/LC_MESSAGES/rog-control-center.morog-control-center/translations/ru/LC_MESSAGES/rog-control-center.morog-control-center/translations/tr/LC_MESSAGES/rog-control-center.morog-control-center/translations/uk_UA/LC_MESSAGES/rog-control-center.morog-control-center/translations/zh_CN/LC_MESSAGES/rog-control-center.mo
📜 Review details
🔇 Additional comments (2)
rog-control-center/build.rs (2)
5-8: LGTM!
16-19: LGTM!Also applies to: 23-25
|
Superseded by #265. Code was taken from both this PR and #243 and both @NB-Group and @scardracs have been added as co-authors. You may both review that PR and after coderabbit's run, I will merge it |
3 participants
Part of the UI redesign stack (1/3) — split out for easier review:
The three PRs touch disjoint files and can be reviewed / merged independently in any order.
Add a
msgfmtstep inbuild.rsthat compiles each.pointo a.monext to it, so translations rebuild from source instead of carrying stale binaries. Drop the checked-in.mofiles and gitignore them.