Rust locale by jreidinger · Pull Request #533 · agama-project/agama

jreidinger · 2023-04-14T08:44:50Z

Problem

Language dbus backend is quite limited in functionality and still eating and lot of memory.

Solution

try to write language component in rust to compare it.

The D-Bus interface org.opensuse.Agama.Language1 is replaced by org.opensuse.Agama.Locale1, described in doc/locale_api.md
Two new crates in the rust/ directory:

agama-dbus-server the common server for Rust-hosted services
agama-locale-data library interfacing with langtable, in particular python-langtable-data.rpm which is just the XML part without the Python code

$ cd service; bundle exec bin/agamactl 
ERROR: Using 'agamactl' to start all services no longer works.
NOTE: It had race conditions all along and now there's a Rust service it can't reach too.
NOTE: Use `systemctl start agama.service` instead
NOTE: which is setup by a) RPMs b) ./setup-service.sh

coveralls · 2023-04-14T08:48:43Z

Changes unknown
when pulling ce3362a on rust_locale
into ** on master**.

imobachgs · 2023-04-14T09:09:31Z

rust/agama-locale-data/src/lib.rs

+
+#[derive(Debug, Deserialize)]
+pub struct Keyboards {
+    pub keyboard: Vec<Keyboard>


np: If you do not plan to add anything else, you could use the newtype pattern idiom.

rust/agama-locale-data/src/lib.rs

imobachgs

I don't know whether we need to split the code into so many modules, but it looks fine, as I do not have enough experience. I would do reduce the amount of repeated code too.

And thanks for writing those unit tests 💯

imobachgs · 2023-04-18T09:32:56Z

rust/agama-locale-data/src/lib.rs

+pub mod ranked;
+
+pub fn get_keyboards() -> keyboard::Keyboards {
+    const FILE_PATH: &str = "/usr/share/langtable/data/keyboards.xml.gz";


I would put all the constants outside of the functions so it is easier to adjust the names. Although I would expect Rust not define the const on every single call.

rust/agama-locale-data/src/lib.rs

mvidner · 2023-04-21T07:02:31Z

rust/agama-dbus-server/src/main.rs

+        self.keyboard_id = keyboard.to_string();
+    }
+
+    fn list_timezones(&self, locale: &str) -> Vec<(String, String)> {


A bad input makes a thread panic and then the service is running in an unusable state. We should send error replies instead

$ ./target/debug/agama-dbus-server & $ busctl --user call org.opensuse.Agama.Locale1 /org/opensuse/Agama/Locale1 org.opensuse.Agama.Locale1 ListTimezones s idontknow thread 'zbus::Connection executor' panicked at 'Wrong language {language}', agama-locale-data/src/localization.rs:12:14 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace Call failed: Connection timed out

use a file_reader helper

mvidner · 2023-04-24T08:37:54Z

@jreidinger I have pushed code that enables error reporting via the D-Bus methods

Funnily, it does not help with the error that I originally encountered: ListTimezones("nosuchlanguage") because that one is pretty deep down the call stack and I want to think about the general API there first.

mvidner · 2023-04-24T08:39:50Z

rust/agama-locale-data/src/lib.rs

+    #[test]
+    fn test_get_languages() {
+        let result = get_languages().unwrap();
+        assert_eq!(result.language.len(), 356);


Guess what, on my system this fails because the actual result is 354 🤣

interesting why you have different data from langtable.

I think I took an old python3-langtable-0.0.51 and symlinked the directory. But the result is valid even for proper packaging: upstream will add some data and this will break our brittle tests.

considering langtable, de-emphasizing systemd but not throwing it away just yet because systemd:locale != langtable:(territory+language) well yes but the `+` is a nontrivial operation

mvidner · 2023-05-11T08:58:47Z

CI Failures:

integration-tests: Playwright... ?? Do I need to run it locally to figure it out
cli_build: (that's Rust build) python-langtable-data is missing; the job runs cargo on ububtu-latest

this only helps for local testing, not for CI

TODO: move this test to integration tests where we can install the python-langtable-data dependency

Its child services would fail anyway when they could not reach Locale

…ks it that's why we would see the login screen in Playwright tests and that needs the frontend built and installed first

mvidner · 2023-05-22T05:35:10Z

@jreidinger I made CI green again, yay :)

imobachgs

The approach looks good. I have some comments, especially about the internal APIs. Nothing to block this PR, though.

doc/locale_api.md

rust/agama-dbus-server/src/error.rs

imobachgs · 2023-05-23T20:31:24Z

rust/agama-dbus-server/src/locale.rs

+            let (loc_language, loc_territory) = agama_locale_data::parse_locale(locale.as_str())?;
+            let languages = agama_locale_data::get_languages()?;
+            let territories = agama_locale_data::get_territories()?;
+            let language = languages


I like that get_languages and get_territories return their structs (territory::Territory and languages::Languages). But it would be even better if those structs implemented an API to find a language or a territory instead of iterating here. You would not even need to know that the languages::Languages has a language field.

Not to mention that this code would be much shorter.

Agreed, I think it makes sense. I just want to have something working quickly and then abstract usable parts to those structs. Parts that are really used.

imobachgs · 2023-05-23T20:34:12Z

rust/agama-dbus-server/src/locale.rs

+
+    #[dbus_interface(property)]
+    fn locales(&self) -> Vec<String> {
+        return self.locales.to_owned();


I would say it is more idiomatic to omit the return (and the semicolon).

imobachgs · 2023-05-23T20:57:58Z

rust/agama-dbus-server/src/locale.rs

+
+    #[dbus_interface(property)]
+    fn supported_locales(&self) -> Vec<String> {
+        return self.supported_locales.to_owned();


Suggested change

return self.supported_locales.to_owned();

self.supported_locales.to_owned()

doc/locale_api.md

rust/README.md

Co-authored-by: Imobach González Sosa <igonzalezsosa@suse.com>

and other doc fixes thanks to Imo's review

or else the daemon WILL disconnect you!! ;-)

thanks to Imo and https://www.deepl.com/write Also link to the cargo_audit PR CI skip, docs only

rust/agama-dbus-server/src/locale.rs

by running `cargo test` in integration_tests job where we do have the langtable dependency With that, integration_tests has swallowed the entire cli_build job.

This makes python-langtable-data available if you only set up the backend I hope I got the split right

mvidner

Yay, thanks, LGTM now 😄

mvidner and others added 8 commits April 11, 2023 15:01

Legacy-free Locale Service, API proposal

76da8aa

better formatting

6b0e10f

Proposal layer for Agama.Locale1, including Priority<T>

23272f5

fixup, accidental readonly by copy-paste

bf50605

Agama.TimeDate1 proposal layer design, just 2 properties

1db73f2

initial commit for locale data

aaadb76

add first struct for serde deserialization

ab47c97

first working code to get list of keyboards

475a755

imobachgs approved these changes Apr 14, 2023

View reviewed changes

jreidinger and others added 5 commits April 14, 2023 13:32

structure code a bit better

c19ca27

add listing of languages

ae17463

Switch from X11Keyboard to VConsoleKeyboard

9b05440

add terrioty call and reafctor ranked lists

28bec0d

add timezone parts

d6f3fcd

imobachgs reviewed Apr 18, 2023

View reviewed changes

jreidinger and others added 5 commits April 18, 2023 17:18

Adds method to get all timezones

db43ccf

WIP another try, with langtable backend in mind

3594325

Add dbus server for locale ( and not only for that )

b6ca533

initial dbus service implementation

cd87f2a

WIP localized timezones

c30f003

mvidner reviewed Apr 21, 2023

View reviewed changes

mvidner and others added 5 commits April 21, 2023 10:11

English+native locale labels like YaST; no kb localized labels

75f473b

final timezone functionality

1af7921

enable again check of timezones size

a289660

Use anyhow::Error to report agama-locale-data errors via D-Bus

367a277

list_x11_keyboards and list_timezones may return errors too

f5ec539

use a file_reader helper

mvidner reviewed Apr 24, 2023

View reviewed changes

Update to reflect initial implementation in rust_locale

6d47b48

considering langtable, de-emphasizing systemd but not throwing it away just yet because systemd:locale != langtable:(territory+language) well yes but the `+` is a nontrivial operation

mvidner force-pushed the rust_locale branch from 01363ba to 23fa502 Compare May 10, 2023 15:09

Fix tests for Language UI

706c35f

mvidner force-pushed the rust_locale branch from 23fa502 to 706c35f Compare May 10, 2023 15:20

mvidner added 5 commits May 11, 2023 14:59

setup.sh: install python-langtable-data

4ce2c17

this only helps for local testing, not for CI

FIXME: revert: disable agama-locale-data tests in CI

ce3362a

TODO: move this test to integration tests where we can install the python-langtable-data dependency

CI test: use setup-service.sh instead of racy manual service startup

33034d4

agamactl: Break plain agamactl up front, instead of later

7fefb8b

Its child services would fail anyway when they could not reach Locale

CI: show journal _after_ tests have autostarted the services

69700e2

mvidner force-pushed the rust_locale branch from f65409c to 6f24a69 Compare May 21, 2023 19:55

CI: start 'cockpit-ws --local-session' before ./setup-service.sh bloc…

4376b1f

…ks it that's why we would see the login screen in Playwright tests and that needs the frontend built and installed first

mvidner force-pushed the rust_locale branch from 6f24a69 to 4376b1f Compare May 21, 2023 20:55

Merge branch 'master' into rust_locale

1cf6b64

mvidner marked this pull request as ready for review May 22, 2023 05:31

imobachgs reviewed May 23, 2023

View reviewed changes

mvidner and others added 5 commits May 24, 2023 09:48

Better spelling

88f390f

Co-authored-by: Imobach González Sosa <igonzalezsosa@suse.com>

Resolve a note about langtable keyboards

7d7350f

and other doc fixes thanks to Imo's review

Spell D-Bus properly

04e5dd6

or else the daemon WILL disconnect you!! ;-)

Improve grammar

8c04226

thanks to Imo and https://www.deepl.com/write Also link to the cargo_audit PR CI skip, docs only

implement find_by_id

ac3c4ce

imobachgs reviewed May 24, 2023

View reviewed changes

rust/agama-dbus-server/src/locale.rs Outdated Show resolved Hide resolved

update from review

339d899

imobachgs approved these changes May 24, 2023

View reviewed changes

mvidner added 3 commits May 24, 2023 17:11

Re-enable agama-locale-data tests

605ca2c

by running `cargo test` in integration_tests job where we do have the langtable dependency With that, integration_tests has swallowed the entire cli_build job.

setup.sh + setup-service.sh: install RPM dependencies in both

224520f

This makes python-langtable-data available if you only set up the backend I hope I got the split right

setup.sh + setup-service.sh: use --gpg-auto-import-keys where it matters

d2264fc

mvidner approved these changes May 24, 2023

View reviewed changes

jreidinger merged commit d09d1e1 into master May 24, 2023

jreidinger deleted the rust_locale branch May 24, 2023 17:38

	return self.supported_locales.to_owned();
	self.supported_locales.to_owned()

Conversation

jreidinger commented Apr 14, 2023 • edited by mvidner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Uh oh!

coveralls commented Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

imobachgs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvidner commented Apr 24, 2023

Uh oh!

mvidner Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvidner commented May 11, 2023

Uh oh!

mvidner commented May 22, 2023

Uh oh!

imobachgs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mvidner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jreidinger commented Apr 14, 2023 •

edited by mvidner

Loading

coveralls commented Apr 14, 2023 •

edited

Loading

mvidner Apr 24, 2023 •

edited

Loading