build(rog-control-center): compile .mo catalogs from .po at build time and install them - #265
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📜 Recent review details🧰 Additional context used🪛 checkmake (0.3.2)Makefile[warning] 99-99: Target body for "install-data-rog_gui" exceeds allowed length of 5 lines (10). (maxbodylength) 🔇 Additional comments (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe build now compiles GUI gettext catalogs, exposes their output path to the application, installs and removes the catalogs, and updates packaging, documentation, and Git ignore rules. ChangesGUI translation catalogs
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant BuildScript as build.rs
participant Msgfmt as msgfmt
participant Makefile
participant GUI as rog-control-center
BuildScript->>Msgfmt: Compile .po files into .mo catalogs
Msgfmt-->>BuildScript: Write locale catalogs
Makefile->>Makefile: Install catalogs into locale directories
GUI->>GUI: Read ROGCC_TRANSLATIONS_DIR
GUI->>GUI: Load local or system catalogs
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 35-36: Update build.rs to place all generated and removed .mo
catalogs in a declared OUT_DIR-based staging directory instead of
CARGO_MANIFEST_DIR, including the paths created near the po/mo assignments and
the later catalog handling block. Update the Makefile target at its
catalog-generation command to use the same declared output directory, ensuring
build and staging paths remain consistent across all affected sites.
- Around line 65-82: Update compile_locales to return errors instead of warning
and continuing when msgfmt is missing or catalog compilation fails; propagate
these errors to the build script entry point so the build fails. Preserve
successful compilation behavior, and add gettext to the package makedepends
configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ba265804-2ffd-4d46-a4c8-99984a75e7f2
📒 Files selected for processing (12)
.gitignoreMakefilerog-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)
.gitignore (1)
31-34: LGTM!Makefile (1)
143-143: LGTM!
eb08de0 to
daec4a2
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 `@Makefile`:
- Around line 104-105: Update the locale-installation command in compile_locales
to handle an empty catalog match before deriving the directory and running find.
Skip installation entirely when no non-English .mo files exist, while preserving
the current installation behavior when catalogs are present.
In `@rog-control-center/build.rs`:
- Around line 35-46: Remove stale locale directories from the translations
output before the catalog-generation loop in build.rs, preserving only locales
currently present under sources and excluding the en template. Update Makefile
at line 105 only if choosing the alternative approach: restrict installation to
catalogs generated for current source locales; otherwise no direct Makefile
change is needed.
In `@rog-control-center/src/main.rs`:
- Line 193: Update the translation path configuration used by
slint::init_translations! in main.rs so packaged execution resolves catalogs
from the installed locale root when RUST_TRANSLATIONS is unset, while preserving
OUT_DIR/translations for development builds. Adjust the corresponding
ROGCC_TRANSLATIONS_DIR setup in build.rs to select the appropriate runtime path;
apply the change in rog-control-center/src/main.rs:193 and
rog-control-center/build.rs:31.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 43cb9c56-8397-42c8-9cf4-4583ad5b27f3
📒 Files selected for processing (14)
.gitignoreMakefiledistro-packaging/PKGBUILDrog-control-center/build.rsrog-control-center/src/main.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 context used
🪛 checkmake (0.3.2)
Makefile
[warning] 99-99: Target body for "install-data-rog_gui" exceeds allowed length of 5 lines (6).
(maxbodylength)
🔇 Additional comments (5)
rog-control-center/build.rs (2)
5-6: LGTM!
25-30: LGTM!Also applies to: 32-34, 48-59
Makefile (1)
99-103: LGTM!Also applies to: 144-144
distro-packaging/PKGBUILD (1)
12-12: LGTM!Also applies to: 54-54
.gitignore (1)
31-32: LGTM!
daec4a2 to
10e4c67
Compare
coderabbitai
Bot
added
documentation
10e4c67 to
3bfd1b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Makefile`:
- Line 24: Update the Makefile SRC dependency list to include
rog-control-center/build.rs alongside the existing Cargo, Makefile, Rust source,
and translation files, so install targets rebuild after build-script changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8dc7d271-31be-4f8e-bafd-e1522d3a4e16
📒 Files selected for processing (15)
.gitignoreMakefiledistro-packaging/PKGBUILDrog-control-center/README.mdrog-control-center/build.rsrog-control-center/src/main.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 context used
🪛 checkmake (0.3.2)
Makefile
[warning] 99-99: Target body for "install-data-rog_gui" exceeds allowed length of 5 lines (10).
(maxbodylength)
🔇 Additional comments (6)
rog-control-center/build.rs (1)
6-6: LGTM!Also applies to: 24-65
rog-control-center/src/main.rs (1)
190-193: LGTM!rog-control-center/README.md (1)
13-13: LGTM!Makefile (1)
99-109: LGTM!Also applies to: 148-148
distro-packaging/PKGBUILD (1)
12-12: LGTM!Also applies to: 54-54
.gitignore (1)
31-33: LGTM!
coderabbitai
Bot
removed
the
documentation
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Makefile (1)
24-24: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winTrack
rog-control-center/build.rsinSRC.The current
SRClist tracks.pofiles but notrog-control-center/build.rs. If the binary target uses$(SRC)as its prerequisite,make installcan skip Cargo after a build-script change and install stale catalogs. Addrog-control-center/build.rstoSRC.Proposed fix
-SRC := Cargo.toml Makefile $(shell find -type f -wholename '**/src/*.rs') $(shell find rog-control-center/translations -type f -name '*.po') +SRC := Cargo.toml Makefile rog-control-center/build.rs $(shell find -type f -wholename '**/src/*.rs') $(shell find rog-control-center/translations -type f -name '*.po')#!/bin/sh set -eu rg -n -C 3 '^(SRC|build:|target/\$\(TARGET\)/\$\(BIN_ROG\):|install-data-rog_gui:)' Makefile rg -n 'rog-control-center/build\.rs' Makefile🤖 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 `@Makefile` at line 24, Update the Makefile SRC prerequisite list to include rog-control-center/build.rs alongside the existing Cargo.toml, Makefile, Rust source, and translation files, ensuring build-script changes trigger the binary rebuild and prevent stale catalogs during make install.
🤖 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 `@Makefile`:
- Around line 99-109: Extract the translation catalog discovery and installation
block from install-data-rog_gui into a dedicated helper target or script, and
invoke it from the recipe. Preserve the existing behavior and keep the
no-compiled-translations guard within the extracted helper.
---
Duplicate comments:
In `@Makefile`:
- Line 24: Update the Makefile SRC prerequisite list to include
rog-control-center/build.rs alongside the existing Cargo.toml, Makefile, Rust
source, and translation files, ensuring build-script changes trigger the binary
rebuild and prevent stale catalogs during make install.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cb450599-8537-4558-9d91-b45d0cd9dceb
📒 Files selected for processing (15)
.gitignoreMakefiledistro-packaging/PKGBUILDrog-control-center/README.mdrog-control-center/build.rsrog-control-center/src/main.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 context used
🪛 checkmake (0.3.2)
Makefile
[warning] 99-99: Target body for "install-data-rog_gui" exceeds allowed length of 5 lines (10).
(maxbodylength)
🔇 Additional comments (7)
rog-control-center/src/main.rs (1)
190-193: LGTM!rog-control-center/README.md (1)
13-13: LGTM!Makefile (1)
148-148: LGTM!distro-packaging/PKGBUILD (1)
12-12: LGTM!Also applies to: 54-54
.gitignore (1)
31-33: LGTM!rog-control-center/build.rs (2)
29-31: 🗄️ Data Integrity & IntegrationVerify that installation consumes the
OUT_DIRcatalogs.Line 29 writes catalogs below Cargo’s
OUT_DIR, not belowrog-control-center/translations. The install rules must copy the exactOUT_DIR/translations/<locale>/LC_MESSAGES/rog-control-center.motree. If they scan a source-tree or fixed staging path, the build succeeds but installed packages contain no catalogs.#!/bin/bash set -euo pipefail for file in Makefile distro-packaging/PKGBUILD; do printf '\n--- %s ---\n' "$file" rg -n -C 5 'OUT_DIR|target/.*/build|translations|LC_MESSAGES|rog-control-center\.mo|cargo (build|install)' "$file" || true doneExpected result: installation reads the same
OUT_DIRproduced byrog-control-center/build.rs, and uninstall removes the installed catalog files.Also applies to: 50-52
6-6: LGTM!
3bfd1b2 to
eec7bc1
Compare
coderabbitai
Bot
added
the
documentation
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Makefile`:
- Line 24: Update the Makefile SRC prerequisite definition to include the
translation directory tree or a filesystem-change stamp so additions and
deletions of translation files invalidate the GUI build; retain the existing
wildcard of PO files to detect translation content changes, and apply the same
adjustment to the corresponding repeated SRC definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 217932a6-c037-4d57-8a56-4e3d6a2e8bf6
📒 Files selected for processing (15)
.gitignoreMakefiledistro-packaging/PKGBUILDrog-control-center/README.mdrog-control-center/build.rsrog-control-center/src/main.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 context used
🪛 checkmake (0.3.2)
Makefile
[warning] 99-99: Target body for "install-data-rog_gui" exceeds allowed length of 5 lines (10).
(maxbodylength)
🔇 Additional comments (6)
rog-control-center/build.rs (1)
6-6: LGTM!Also applies to: 24-72
rog-control-center/src/main.rs (1)
190-193: LGTM!rog-control-center/README.md (1)
13-13: LGTM!Makefile (1)
148-148: LGTM!distro-packaging/PKGBUILD (1)
12-12: LGTM!Also applies to: 54-54
.gitignore (1)
31-33: LGTM!
…e and install them Co-Authored-By: NB-Group <NB-Group@users.noreply.github.com> Co-Authored-By: Marco Scardovi <scardracs@disroot.org>
eec7bc1 to
ad6dc51
Compare
|
@coderabbitai full review |
|
✅ Action performedFull review finished. |
1 participant
Description
Supersedes #252 and #243. Code is taken from both PRs and both authors have been added as co-authors
Tested Hardware & Environment
Verification and testing:
cargo fmt --all -- --check)cargo clippy --all -- -D warnings/cargo check --all-targets)cargo test --all)cargo cranky)