-
Notifications
You must be signed in to change notification settings - Fork 3
FF-3753 feat(dart): Dart SDK #197
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
Conversation
Copied flutter_rust_bridge minimal example[1] and beat it into working: - vendored simple_build.dart to remove dependency on an unpublished git-only package - updated packages because I couldn't get dart test to work otherwise - stripped flutter_rust_bridge internal options [1]: https://github.com/fzyzcjy/flutter_rust_bridge/tree/05247dd90a7fcb2512c5ede13cf00271b0eb548e/frb_example/dart_minimal
🦋 Changeset detectedLatest commit: 69fdcb2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Dropping `BackgroundRuntime` cancels all tracked tasks. This may (and have) lead to hard to debug issues when configuration poller just stops working and `.wait_for_configuration()` unhelpfully returns `PollerThreadPanicked`. (I am a little embarrassed to say how long this simple issue took me to debug when it was paired with running in Dart environment.)
I'm going to work on adding documentation next but this is now ready for review 🎉 |
await this._core!.waitForInitialization(); | ||
} | ||
|
||
String stringAssignment(String flagKey, Subject subject, String defaultValue) { |
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.
Just wanted to note that this is a different signature than usual, with a subject instead of subjectId and subjectAttributes
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.
Yeah, the ask for review leaked slightly earlier than I expected, so I didn't have the time to annotate all the interesting parts 🙂
For the difference here: I hate Attributes
/ContextAttributes
distinction in other SDKs, so I tried to experiment with it. This change makes signature uniform for both assignments and bandits.
Bandit signature for comparison (no ContextAttributes
!):
EvaluationResult<String> banditAction(
String flagKey,
Subject subject,
Map<String, Attributes> actions,
String defaultVariation)
and example usage in Dart (from test):
final bandit = client.banditAction(
'banner_bandit_flag',
Subject('alice')
..numberAttribute('age', 25)
..stringAttribute('country', 'USA')
..stringAttribute('genderIdentity', 'female'),
{
'nike': Attributes()
..numberAttribute('brand_affinity', 1.0)
..stringAttribute('loyalty_tier', 'gold')
..boolAttribute('purchased_last_30_days', true),
},
'control',
);
That seems to work better for typed languages that don't allow mixing strings and numbers in the same container. I like it but curious what other think.
@@ -101,6 +101,7 @@ impl BackgroundRuntime { | |||
|
|||
/// Command background activities to stop and exit. | |||
pub(crate) fn stop(&self) { | |||
log::debug!(target: "eppo", "stopping background runtime"); |
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.
Did you intend to keep this debug here?
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.
Absolutely: 5353290
I've spent a couple of days debugging why poller mysteriously crashes (spoiler: it was just background runtime silently stopping). That log line would cut the debugging to one minute 🙈
As a first-time Dart user, this looks very clean and insightful! Will be prudent and have a second pair of eyes approve |
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.
@rasendubi this is huge - I did a line by line review and enjoying reading all of it. I can tell you had to solve several challenges related to the background runtime across wasm/non-wasm as well as running in the javascript environment. Very appreciated that you didn't take shortcuts but seemed to step back and create generalized interfaces for the new behavior.
Before approving and releasing could you please help me run the dart & flutter web artifacts locally so I can get comfort with the tooling? Apologize for not figuring this out myself after some research.
|
||
Make async runtime abstract. | ||
|
||
This introduces an `AsyncRuntime` trait that allows us to abstract over different async runtimes. This is required to support Dart SDK that doesn't use tokio runtime in web build. |
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.
Wow, very nice!
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.
we probably need to add a note here about dropping openssl in favor of rust-tls
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.
@felipecsl I've added another changeset for this. This one affects all SDKs though
@@ -8,6 +8,7 @@ | |||
"test:rust": "cargo test", | |||
"test:python": "cd python-sdk && pytest", | |||
"test:ruby": "cd ruby-sdk && bundle exec rake build && bundle exec rspec", | |||
"test:dart": "cd dart-sdk && dart --enable-experiment=native-assets test", |
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.
Tried running this locally with exception:
Compiling eppo_dart v1.0.0 (/Users/leo/src/eppo-multiplatform/dart-sdk/rust)
error: failed to run custom build command for `eppo_dart v1.0.0 (/Users/leo/src/eppo-multiplatform/dart-sdk/rust)`
Caused by:
process didn't exit successfully: `/Users/leo/src/eppo-multiplatform/dart-sdk/rust/target/release/build/eppo_dart-6f7c55cf4f81c058/build-script-build` (exit status: 101)
--- stdout
[2025-02-22T16:06:52.693Z DEBUG /Users/leo/.cargo/registry/src/index.crates.io-6f17d22bba15001f/flutter_rust_bridge_codegen-2.8.0/src/library/codegen/config/config_parser.rs:51] Found config file ../flutter_rust_bridge.yaml
[2025-02-22T16:06:52.694Z DEBUG /Users/leo/.cargo/registry/src/index.crates.io-6f17d22bba15001f/flutter_rust_bridge_codegen-2.8.0/src/library/codegen/mod.rs:23] config=Config { base_dir: Some(".."), rust_input: Some("crate::api"), dart_output: Some("lib/src/rust"), c_output: Some("frb_generated.h"), duplicated_c_output: None, rust_root: None, rust_output: None, dart_entrypoint_class_name: None, dart_format_line_length: None, dart_preamble: None, rust_preamble: Some("use eppo_core::{Str, AttributeValue};\n"), dart_enums_style: None, add_mod_to_lib: None, llvm_path: None, llvm_compiler_opts: None, dart_root: None, build_runner: None, extra_headers: None, web: None, deps_check: None, dart3: None, full_dep: None, local: None, default_external_library_loader_web_prefix: None, dart_type_rename: None, enable_lifetime: None, type_64bit_int: None, default_dart_async: Some(false), stop_on_error: None, dump: None, dump_all: None, rust_features: None } meta_config=MetaConfig { watch: false }
[2025-02-22T16:06:52.695Z DEBUG /Users/leo/.cargo/registry/src/index.crates.io-6f17d22bba15001f/flutter_rust_bridge_codegen-2.8.0/src/library/codegen/config/internal_config_parser/mod.rs:33] InternalConfig.parse base_dir="/Users/leo/src/eppo-multiplatform/dart-sdk"
[2025-02-22T16:06:52.696Z ERROR /Users/leo/.cargo/registry/src/index.crates.io-6f17d22bba15001f/flutter_rust_bridge_codegen-2.8.0/src/library/utils/logs.rs:54] panicked at build.rs:25:6:
called `Result::unwrap()` on an `Err` value: Fail to canonicalize path="/Users/leo/src/eppo-multiplatform/dart-sdk/lib/src/rust"
Caused by:
No such file or directory (os error 2)
--- stderr
thread 'main' panicked at build.rs:25:6:
called `Result::unwrap()` on an `Err` value: Fail to canonicalize path="/Users/leo/src/eppo-multiplatform/dart-sdk/lib/src/rust"
Caused by:
No such file or directory (os error 2)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Unhandled exception:
ProcessException: Bad exit code (101). If you want to see extra information, set FRB_DART_RUN_COMMAND_STDERR=1
Command: cargo build --release
#0 runCommand (package:flutter_rust_bridge/src/cli/run_command.dart:67:5)
<asynchronous suspension>
#1 simpleBuild (file:///Users/leo/src/eppo-multiplatform/dart-sdk/simple_build.dart:54:5)
<asynchronous suspension>
stdout:
Error: Compiling native assets failed.
do you expect this command to succeed on macos?
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.
oh, it might be because you don't have lib/src/rust
directory. It's auto-generated, so git-ignored. I've just added a .gitkeep file, so git will create a directory for you when you pull latest changes
/// If you want your task to react to shutdown and perform some cleanup, spawn your task with | ||
/// [`BackgroundThread::spawn_tracked()`] and watch this cancellation token. | ||
/// | ||
/// Note: this is a child token of the main cancellation token, so canceling it does not cancel |
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.
very nice - this will be primarily used within SDKs or can you imagine a customer facing use too?
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.
I don't think there's customer use. Right now, this is only used within core (not even exported to individual sdks)
.0 | ||
.0 |
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.
If the underlying interfaces changes do you think the compiler will break here and alert us? Consider how to write this with protection and a log message to help a future developer.
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.
Yes, absolutely. That's one of the things that broke in web build because implementation is different.
We are pinning rust_flutter_bridge
version, so this should not break unexpectedly. And when it does, the error will be scoped to this file, so it will be quite clear what happened
/// `AsyncRuntime` abstracts over various Rust's async runtimes with minimal interface. This is | ||
/// usually just a tokio Handle but Dart SDK in wasm uses a custom implementation. |
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.
I did most of my reading around this change to understand it and got introduced to the Send
interface, thanks! https://doc.rust-lang.org/nomicon/send-and-sync.html
Also needed help from AI to understand how it fit together with the wasm single threaded runtime.
In the future, if more non-Send futures need handling, you may need a more generalized solution (e.g., perhaps relax the trait bound for wasm via conditional compilation, or provide a separate method for non-Send tasks). For now, the approach is correct and contained to config polling.
What do you imagine doing down the line to evolve this interface?
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.
Yeah, I don't really like this AsyncRuntime
and I kinda used an escape hatch in configuration poller by invoking wasm spawn directly.
Having different definition for wasm is possible but it's going to be contagious as interfaces of spawn_tracked
/_untracked
would change as well. I don't think it's going to propagate much further though, so probably worth biting that bullet at some point.
I'm also not entirely sure what's multi-threading story for wasm is. It looks like there is some support for threading on some platforms and on web via SharedArrayBuffer
(when cross-origin isolation is enabled).
flag_key: &str, | ||
subject_key: Str, | ||
subject_attributes: HashMap<Str, AttributeValue>, | ||
) -> (Option<Str>, Option<AssignmentEvent>) { |
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.
Is it easier to return this type instead of a Result
for use in dart or did something else inform your decision?
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.
Don't remember exactly but it's probably because we needed to convert from uni-typed AssignmentValue
to Str
, so ended up with this destructuring:
let Ok(Some(Assignment {
value: AssignmentValue::String(result),
event,
})) = self.evaluator.get_assignment(...)
else {
return ...;
};
at which point the two sensible options are either Option<(Str, Option<AssignmentEvent>)>
(or Result
outside) or (Option<Str>, Option<AssignmentEvent>)
. The first one is more faithful (because event can only be present if there's an assignment) but the second is slightly easier to work from Dart:
final (result, event) = core.stringAssignment(flagKey, subject.key, subject.attributes.coreAttributes);
this.logAssignmentEvent(event);
return result ?? defaultValue;
vs. having to unnest two levels of optionals
@leoromanovsky sharing my stashed commands for web build (my understanding is that we'll put them in CI before release):
|
Another piece that I totally forgot to mention is that dart-sdk has its own cargo workspace, so for local testing we need to override eppo_core crate (similarly to how we do in ruby). Put the following in [patch.crates-io]
eppo_core = { path = '../../eppo_core' } |
Just to follow up here, I can confirm I was able to build for web with Dart 3.6 |
This is used as the name of js/wasm generated files.
* add plugin info * remove plugin details * vendor libs * remove corepack changes * add rust api files * add more generated code * add another missing file * catch reqwest panic * replace openssl with rustls-tls * update deps * remove native-tls and openssl * remove generated files * another file to remove * remove openssl-sys * newlines * revert this * keep vendored feature * try to fix python CI * set vars for linux gnu * install deps * another try * build with `cross` instead * fix cross commang * fix target platform triple * another try * revert action changes * another try - see PyO3/maturin-action#222 * once again * anotehr place
|
||
Make async runtime abstract. | ||
|
||
This introduces an `AsyncRuntime` trait that allows us to abstract over different async runtimes. This is required to support Dart SDK that doesn't use tokio runtime in web build. |
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.
we probably need to add a note here about dropping openssl in favor of rust-tls
export AR_aarch64_linux_android="$NDK_HOME/toolchains/llvm/prebuilt/darwin-x86_64/bin/llvm-ar" | ||
export RANLIB="$NDK_HOME/toolchains/llvm/prebuilt/darwin-x86_64/bin/llvm-ranlib" | ||
|
||
cargo build --release --target aarch64-linux-android -p eppo_dart |
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.
we can probably drop this in favor of https://github.com/cross-rs/cross in the future
@@ -0,0 +1,7 @@ | |||
--- | |||
"eppo_core": major |
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.
Thanks for making this a major bump - makes a ton of sense to give us room to test python and ruby more extensively.
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.
It doesn't give us much room. It is going to bump eppo_core's version in both python and ruby, so the next python/ruby releases will release with this new change. (This is the same regardless of whether it's a major or patch change.)
This reduces wasm file from 12M down to 2M.
@felipecsl python build started to fail. Do you think it is related to the rustls changes? |
@rasendubi it was working on friday on my branch #222 and also started failing there, I think something changed in the underlying OS, taking a look |
* chore(ci): dart-sdk rust cross compile for android target * eppo_dart instead of eppo_sdk * revert this * build patches * revert this * update file path for testing * another try * change working dir * CI tweaks * better logs * more fix attempts * aaaand another try * 10th time is the charm * check for path instead * check CI env var * passthrough env var * use the fork of maturin-action * yup * bump action version * fix step * fix ste * update action version, another shot at android fix * fix pytest * move file * add file * set rust root to current dir * fix cross build config * manually run codegen * install dart * reorder steps * another reorder * another try * go back to main maturin-action * remove custom build scripts * add android x86 * remove images cross config * go back to maturin-action v1
anything blocking this PR from being merged? |
No description provided.