-
Notifications
You must be signed in to change notification settings - Fork 289
i18n POC #680
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
base: vgough/rust
Are you sure you want to change the base?
i18n POC #680
Conversation
Experimental German translation Simplify i18n usage with i18n_format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a proof-of-concept for internationalization (i18n) support using the gettext framework. It wraps user-facing strings in the Rust codebase with i18n_format! macros and adds the necessary infrastructure for translation management.
- Adds gettext-based i18n infrastructure with German translations as a proof of concept
- Wraps error messages and log output in translation macros
- Creates build system support for compiling .po translation files to .mo format
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| xpo.sh | Shell script for extracting and updating translation files from source code |
| po/encfs.pot | Translation template with extracted translatable strings |
| po/de.po | German translation file with 775 lines including legacy translations |
| po/LINGUAS | List of supported languages (currently only German) |
| build.rs | Build script that compiles .po files to .mo format using msgfmt |
| src/main.rs | Initializes i18n and wraps user-facing strings in translation macros |
| src/lib.rs | Implements i18n module with gettext integration and feature flag support |
| src/fs.rs | Wraps filesystem error messages with translation macros |
| src/crypto/file.rs | Wraps decryption error messages with translation macros |
| Cargo.toml | Adds gettext-rs and i18n-format dependencies with gettext feature flag |
| README.md | Documents i18n support and gettext version requirements |
| INSTALL.md | Adds documentation for disabling i18n and customizing locale directory |
| .gitignore | Ignores editor temporary files (vim swap files) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| For i18n, the script xpo.sh may be used to update translations from time to | ||
| time. For translation string contribution, see the management service hosted by | ||
| Canonical [Launchpad](https://launchpad.net/encfs). NOTE: frozen versions of | ||
| gettext utilities in Linux distributions may lack Rust langugate support. |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo in the word "langugate" which should be "language".
| gettext utilities in Linux distributions may lack Rust langugate support. | |
| gettext utilities in Linux distributions may lack Rust language support. |
| let locale_dir = concat!(env!("OUT_DIR"), "/po"); | ||
| // actually make it customizable at build time | ||
| let locale_dir = option_env!("ENCFS_LOCALE_DIR").unwrap_or(locale_dir); |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The runtime code uses option_env!("ENCFS_LOCALE_DIR") to read an environment variable at compile time, but this won't read the value that was set using cargo:rustc-env=ENCFS_LOCALE_DIR in build.rs (line 21). The cargo:rustc-env directive sets a compile-time environment variable that should be read with env!() instead. The current implementation will always use the hardcoded fallback path and ignore the value set by build.rs.
| let locale_dir = concat!(env!("OUT_DIR"), "/po"); | |
| // actually make it customizable at build time | |
| let locale_dir = option_env!("ENCFS_LOCALE_DIR").unwrap_or(locale_dir); | |
| // Use the locale directory provided at build time via `cargo:rustc-env=ENCFS_LOCALE_DIR` | |
| let locale_dir = env!("ENCFS_LOCALE_DIR"); |
| gettext-rs = { version = "0.7.7", features = ["gettext-system"] } | ||
| i18n-format = "0.4.0" |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gettext feature is defined but doesn't enable the required dependencies. The gettext-rs and i18n-format dependencies should be marked as optional and only enabled when the gettext feature is active. Currently, these dependencies are always included even when building with --no-default-features, which contradicts the stated goal in INSTALL.md of being able to disable i18n support.
| let (encrypted_name, new_iv) = | ||
| self.cipher.encrypt_filename(name_bytes, iv).map_err(|e| { | ||
| error!("Encrypt filename failed: {}", e); | ||
| error!("{}", i18n_format!("Encrypt filename failed: {}", e)); //.unwrap()); // formatx!(gettext("Encrypt filename failed: {}"), e).unwrap()); |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an unnecessary commented-out code fragment ".unwrap()); // formatx!(gettext(...".unwrap());" at the end of the line. This should be removed to keep the code clean.
| error!("{}", i18n_format!("Encrypt filename failed: {}", e)); //.unwrap()); // formatx!(gettext("Encrypt filename failed: {}"), e).unwrap()); | |
| error!("{}", i18n_format!("Encrypt filename failed: {}", e)); |
| .arg(mo_path) | ||
| .arg("--statistics") | ||
| .status() | ||
| .expect("failed to execute msgfmt"); |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling here uses .expect() with a generic message. If the msgfmt command is not found in the PATH, this will produce a confusing error. Consider providing a more helpful error message that suggests installing gettext tools or checking the PATH.
| .expect("failed to execute msgfmt"); | |
| .unwrap_or_else(|e| { | |
| panic!( | |
| "failed to execute `msgfmt`: {}. Ensure GNU gettext tools are installed and that `msgfmt` is available in your PATH.", | |
| e | |
| ); | |
| }); |
|
|
||
| #[cfg(feature = "gettext")] | ||
| /// | ||
| /// Helper class to handle i18n-specific helper functions, or dummies wrappers to disable |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation comment uses "Helper class" which is not the appropriate Rust terminology. In Rust, this should be referred to as a "module" since it's defined with pub mod i18n. The comment should read "Helper module to handle i18n-specific helper functions..."
| /// Helper class to handle i18n-specific helper functions, or dummies wrappers to disable | |
| /// Helper module to handle i18n-specific helper functions, or dummies wrappers to disable |
|
|
||
| let locale_dir = env::var("ENCFS_LOCALE_DIR") | ||
| .map(PathBuf::from) | ||
| .unwrap_or_else(|_| PathBuf::from(env::var("OUT_DIR").unwrap()).join("i18n")); |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build script constructs a default locale_dir path using "i18n" as the subdirectory name, but when no ENCFS_LOCALE_DIR environment variable is set, this results in OUT_DIR/i18n being used. However, in src/lib.rs line 16, the default path uses OUT_DIR/po. This inconsistency could lead to runtime errors where the compiled .mo files are in one location but the code looks for them in another.
| .unwrap_or_else(|_| PathBuf::from(env::var("OUT_DIR").unwrap()).join("i18n")); | |
| .unwrap_or_else(|_| PathBuf::from(env::var("OUT_DIR").unwrap()).join("po")); |
| #!/bin/sh -e | ||
|
|
||
| pot=po/encfs.pot | ||
| langs=`grep -v '#' po/LINGUAS` |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script uses backticks for command substitution which is the older sh/bash syntax. While this works, the more modern and preferred syntax is $() instead of backticks. Consider changing langs=grep -v '#' po/LINGUAS`` to langs=$(grep -v '#' po/LINGUAS) for better readability and nesting support.
| langs=`grep -v '#' po/LINGUAS` | |
| langs=$(grep -v '#' po/LINGUAS) |
| fn main() { | ||
| // Skip compilation when using system gettext | ||
| if env::var_os("CARGO_FEATURE_GETTEXT_SYSTEM").is_some() { | ||
| println!("cargo:warning=Using system gettext translations"); | ||
| return; | ||
| } |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build.rs script will always attempt to compile translations and execute msgfmt, even when building with --no-default-features to disable i18n support. The script should check for the gettext feature and skip translation compilation when the feature is disabled. Consider adding a feature check at the beginning: if env::var_os("CARGO_FEATURE_GETTEXT").is_none() { return; }
Basic example of POC intergration into a Rust project. Some bits are still missing: