search

fzyzcjy / flutter_rust_bridge

Flutter/Dart <-> Rust binding generator, feature-rich, but seamless and simple.
https://fzyzcjy.github.io/flutter_rust_bridge/
MIT License
4.29k stars 301 forks source link

Logging Overhaul #2308

Open patmuk opened 2 months ago

patmuk commented 2 months ago

Changes

Please list issues fixed by this PR here, using format "Fixes #the-issue-number".

Checklist

Remark for PR creator

patmuk commented 2 months ago

@fzyzcjy I cleaned up the branch ... that is, I needed to create a new branch. As I could not swap the branch from the old PR (#2303) I created this new one.

Will close the old one if this (macro) approach is working :)

patmuk commented 2 months ago

I solved the dependency issue, but I have a code generation issue now:

In the macro code, in frb_rust/src/log_2_dart.rs I get:

  --> /Users/patmuk/code/own/oss/frb/flutter_rust_bridge_fork_pat/frb_rust/src/log_2_dart.rs:81:17
   |
81 |             use frb_generated::StreamSink;
   |                 ^^^^^^^^^^^^^ use of undeclared crate or module `frb_generated`

Clearly, in this code I have not generated code. But I need to refer to the StreamSink, which should be generated ...

Do you have an advice how to progress?

  1. I could copy the generated code for the StreamSink to the macro as well - but this way we are close (back?) to the non-macro way in PR #2303
  2. I could probably inject it as a parameter of the macro ... but then the StreamSink needs to be defined in the users project - not a very clean solution
  3. statically calling code generation?

I am sure you have a better idea :) It is a bit cat-and-mouse: The logger code should be generated (actually integrated), but it needs the StreamSink to be generated to work ...

As for solving the log dependency: For using log::info!() in the user's rust code, he needs to use flutter_rust_bridge::log_2_dart::log; (with no modification on the cargo.toml, or use log;, but then importing the crate in the cargo.toml. While the latter can be done when creating a project the first time (in the cargo.toml template you pointed me to), it is a breaking change that needs to be migrated for users with existing projects.

Nevertheless, I want to leave this decision open until I see if use flutter_rust_bridge::log_2_dart is needed anyways - maybe to solve the problem above.

fzyzcjy commented 2 months ago

In the macro code

Standard Rust would need crate::frb_generated::... to find the frb_geneated.rs

patmuk commented 2 months ago

Standard Rust would need crate::frb_generated::... to find the frb_geneated.rs

The problem is, that there is no struct StreamSink declared yet. It is generated later.

error[E0432]: unresolved import `crate::frb_generated`
 --> /Users/patmuk/code/own/oss/frb/flutter_rust_bridge_fork_pat/frb_rust/src/log_2_dart.rs:4:12
  |
4 | use crate::frb_generated::StreamSink;
  |            ^^^^^^^^^^^^^ could not find `frb_generated` in the crate root
error[E0412]: cannot find type `StreamSink` in this scope
  --> /Users/patmuk/code/own/oss/frb/flutter_rust_bridge_fork_pat/frb_rust/src/log_2_dart.rs:87:30
   |
87 |                 stream_sink: StreamSink<Log2DartLogRecord>,
   |                              ^^^^^^^^^^ not found in this scope
   |
  ::: /Users/patmuk/code/own/oss/frb/flutter_rust_bridge_fork_pat/frb_rust/src/misc/user_utils.rs:10:5
   |
10 |     setup_logging!();
   |     ---------------- in this macro invocation
   |
   = note: this error originates in the macro `setup_logging` (in Nightly builds, run with -Z macro-backtrace for more info)
error[E0412]: cannot find type `StreamSink` in this scope
  --> /Users/patmuk/code/own/oss/frb/flutter_rust_bridge_fork_pat/frb_rust/src/log_2_dart.rs:92:29
   |
92 |                 log_stream: StreamSink<Log2DartLogRecord>,
   |                             ^^^^^^^^^^ not found in this scope
   |
  ::: /Users/patmuk/code/own/oss/frb/flutter_rust_bridge_fork_pat/frb_rust/src/misc/user_utils.rs:10:5
   |
10 |     setup_logging!();
   |     ---------------- in this macro invocation
   |
   = note: this error originates in the macro `setup_logging` (in Nightly builds, run with -Z macro-backtrace for more info)
Some errors have detailed explanations: E0412, E0432.
For more information about an error, try `rustc --explain E0412`.
patmuk commented 2 months ago

I progressed a bit, but have the same problem.

I moved my macro setup_logging to frb_rust/src/for_generated/boilerplate.rs. The StreamSink dependency resolves, but SSE isn't generated (yet):

error[E0599]: the method `add` exists for struct `StreamSink<Log2DartLogRecord>`, but its trait bounds were not satisfied
  --> src/frb_generated.rs:34:1
   |
34 | / flutter_rust_bridge::frb_generated_boilerplate!(
35 | |     default_stream_sink_codec = SseCodec,
36 | |     default_rust_opaque = RustOpaqueMoi,
37 | |     default_rust_auto_opaque = RustAutoOpaqueMoi,
38 | | );
   | | ^
   | | |
   | | method cannot be called on `StreamSink<Log2DartLogRecord>` due to unsatisfied trait bounds
   | |_doesn't satisfy `Log2DartLogRecord: SseEncode`
   |   method `add` not found for this struct
   |
note: trait bound `Log2DartLogRecord: SseEncode` was not satisfied
  --> src/frb_generated.rs:34:1
   |
34 | / flutter_rust_bridge::frb_generated_boilerplate!(
35 | |     default_stream_sink_codec = SseCodec,
36 | |     default_rust_opaque = RustOpaqueMoi,
37 | |     default_rust_auto_opaque = RustAutoOpaqueMoi,
38 | | );
   | | ^ unsatisfied trait bound introduced here
   | |_|
   |
note: the trait `SseEncode` must be implemented
  --> src/frb_generated.rs:34:1
   |
34 | / flutter_rust_bridge::frb_generated_boilerplate!(
35 | |     default_stream_sink_codec = SseCodec,
36 | |     default_rust_opaque = RustOpaqueMoi,
37 | |     default_rust_auto_opaque = RustAutoOpaqueMoi,
38 | | );
   | |_^
   = help: items from traits can only be used if the trait is implemented and in scope
   = note: the following trait defines an item `add`, perhaps you need to implement it:
           candidate #1: `Add`
   = note: this error originates in the macro `$crate::setup_logging` which comes from the expansion of the macro `flutter_rust_bridge::frb_generated_boilerplate` (in Nightly builds, run with -Z macro-backtrace for more info)

Is there an even later place to call the macro? Somehow it looks like the codegen would have to run twice, first to expand the macros and then to generate the code for the StreamSink<Log2DartLogRecord>?

fzyzcjy commented 2 months ago

The problem is, that there is no struct StreamSink declared yet. It is generated later.

Hmm that's weird: Then how can users have something like 

fn f(a: StreamSink<i32>) {}

in their code (e.g. in frb_example/dart_minimal/simple.rs)?

If users can do that, then we may also be able to do that, since calling a macro is nothing but copy-paste some code into frb_example/dart_minimal

fzyzcjy commented 2 months ago

Is there an even later place to call the macro?

In frb_example/dart_minimal/simple.rs. Just like we generate a fn init_app() {}, we should generate a your_macro_name_for_logging!();.

We deliberately do this, because by doing so, users can very easily modify/customize/disable the default logging solution, by e.g. changing that line to your_macro_name_for_logging!(their_custom_args); or remove that line. This is also why I choose to generate the init app function.

patmuk commented 2 months ago

Is there an even later place to call the macro?

In frb_example/dart_minimal/simple.rs. Just like we generate a fn init_app() {}, we should generate a your_macro_name_for_logging!();.

We deliberately do this, because by doing so, users can very easily modify/customize/disable the default logging solution, by e.g. changing that line to your_macro_name_for_logging!(their_custom_args); or remove that line. This is also why I choose to generate the init app function.

I put the macro call (setup_logging!();) intofrb_example/dart_minimal/rust/src/api/minimal.rs`, but get the same error

Click me ``` error[E0599]: the method `add` exists for struct `StreamSink`, but its trait bounds were not satisfied --> src/api/minimal.rs:8:1 | 8 | #[frb(init)] | ^^^^^^^^^^^^ | | | method cannot be called on `StreamSink` due to unsatisfied trait bounds | doesn't satisfy `Log2DartLogRecord: SseEncode` | ::: src/frb_generated.rs:34:1 | 34 | / flutter_rust_bridge::frb_generated_boilerplate!( 35 | | default_stream_sink_codec = SseCodec, 36 | | default_rust_opaque = RustOpaqueMoi, 37 | | default_rust_auto_opaque = RustAutoOpaqueMoi, 38 | | ); | |_- method `add` not found for this struct | note: trait bound `Log2DartLogRecord: SseEncode` was not satisfied --> src/frb_generated.rs:34:1 | 34 | / flutter_rust_bridge::frb_generated_boilerplate!( 35 | | default_stream_sink_codec = SseCodec, 36 | | default_rust_opaque = RustOpaqueMoi, 37 | | default_rust_auto_opaque = RustAutoOpaqueMoi, 38 | | ); | | ^ unsatisfied trait bound introduced here | |_| | note: the trait `SseEncode` must be implemented --> src/frb_generated.rs:34:1 | 34 | / flutter_rust_bridge::frb_generated_boilerplate!( 35 | | default_stream_sink_codec = SseCodec, 36 | | default_rust_opaque = RustOpaqueMoi, 37 | | default_rust_auto_opaque = RustAutoOpaqueMoi, 38 | | ); | |_^ = help: items from traits can only be used if the trait is implemented and in scope = note: the following trait defines an item `add`, perhaps you need to implement it: candidate #1: `Add` = note: this error originates in the macro `setup_logging` which comes from the expansion of the macro `flutter_rust_bridge::frb_generated_boilerplate` (in Nightly builds, run with -Z macro-backtrace for more info) For more information about this error, try `rustc --explain E0599`. error: could not compile `frb_example_dart_minimal` (lib) due to 1 previous error 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) #1 simpleBuild (package:flutter_rust_bridge_utils/src/simple_build_utils.dart:30:5) stdout: Error: Compiling native assets failed. ```

I tried different places (inside init_app, inside minimal_adder, nowhere, but annotated with #[frb(init)]), but I am getting the same error message.

patmuk commented 2 months ago

When I copy the rust code from the macro directly into dart_minimal/minimal.rs the code generator creates the SSE code and the StreamSink.

Maybe the macro get's expanded, but the code inside not parsed by the generator?

fzyzcjy commented 2 months ago

Hmm, could you please make a minimal reproducible sample? I guess there may be some problems in some details. Anyway, other rust macros seem to work well. @MnlPhlp 's https://github.com/MnlPhlp/flutter_logger also uses macro and also works.

patmuk commented 2 months ago

Hmm, could you please make a minimal reproducible sample?

Here is the minimal example: https://github.com/patmuk/flutter_rust_bridge/tree/logging_macro_minimal

I figured that removing the Log trait implementation: impl log::Log for Log2Dart { makes it work!

So, somehow the trait gets into the way of code generation:

            #[frb(non_opaque)]
            pub struct Log2Dart {
                stream_sink: StreamSink<Log2DartLogRecord>,
            }
   |   method cannot be called on `StreamSink<Log2DartLogRecord>` due to unsatisfied trait bounds
   |   doesn't satisfy `Log2DartLogRecord: SseEncode`

I might be able to workaround by having the StreamSink not be a field of Log2Dart, which implements the log trait. But I think there is a bug here that might hit somebody in a similar constellation.

UPDATE: I moved the StreamSink out of the struct Log2Dart (second commit in the minimal-example-branch) - but the problem is still the same!

Somehow implementing the trait log:Log on a unit struct (Log2Dart) hinders SSE code generation for another struct Log2DartRecord.

fzyzcjy commented 2 months ago

Hmm I cannot see macro call in https://github.com/patmuk/flutter_rust_bridge/blob/logging_macro_minimal/frb_example/dart_minimal/rust/src/api/minimal.rs. Could you please point out more details?

EDIT: Oh I see... Instead of 

#[frb(init)]
pub fn init_app() {
    flutter_rust_bridge::setup_default_user_utils();
    setup_logging!();
}

what about

setup_logging!();

#[frb(init)]
pub fn init_app() {
    flutter_rust_bridge::setup_default_user_utils();
}

(and maybe some other minor modifications after that).

Just imagine macro call as copy-paste something to there.

Also, learning how to debug macros (e.g. the cargo expand) may help.

patmuk commented 2 months ago

I reconstructed the macro code into the dart_minimal project - and realized that some imports are missing ... and that I initialized it from the wrong side: Instead of from the rust code the initialization has to happen from the dart code init_logger(); - which is actually the function you wanted to have (taking a log level, custom log function, and not logging anything if not called).

So, I am onto something :)

However, I am wondering where the dart code from #frb[dart_code=] ends up? In frb_generated.dart or minimal.dart (or similar)? I can't find it ... but I assume that I call it on the wrong place:

in the macro definition I had 

#[macro_export]
macro_rules! setup_logging {
    () => {
        #[frb(dart_code = "
(... dart code ...)
")]
pub fn rust_logger_init(){
(... rust code ...)
}

but I think pub fn rust_logger_init() is problematic, somehow hindering the StreamSink sse code generation. But I have to put something there, right? Just the code, starting with use ... looks not to work, as I can't find the generated dart code. I tried to put it into a mod but still no dart code.

fzyzcjy commented 1 month ago

but I think pub fn rust_logger_init() is problematic,

I cannot get it...

Anyway, for simplicity, maybe you can firstly ignore the macro problem and do everything directly. Only after we are 100% done, let's move to macro.

patmuk commented 1 month ago

Anyway, for simplicity, maybe you can firstly ignore the macro problem and do everything directly. Only after we are 100% done, let's move to macro.

Here is the minimal example: https://github.com/patmuk/flutter_rust_bridge/tree/logging_macro_minimal

The code is in frb_examples/dart_minimal/rust/src/api/minimal.rs.

I reverted back to a state without macros. Here I try to use #[frb(dart_code= )] to inline the dart code - but no code is added in the generated output.

I made a minimal example of my problem with #[frb(dart_code = )].

I realized that the dart_code is removed, if the struct it is annotating is not used.

Can one only add the #[frb(dart_clone= )] attribute to structs? Not to mods?

patmuk commented 1 month ago

Update before you answer: I restructured the code, so the #[frb(dart_code = )] attribute is annotated at a struct. Tis works now!

So, if this limitation:

is true, I will update the documentation of it :)

fzyzcjy commented 1 month ago

If so, I suspect dart_code may currently only be put inside class or something like that... Thus, we may enhance frb a little bit to let it generate dart_code for functions etc. (which is not too hard, since dart_code is nothing but copy-pasting)

patmuk commented 1 month ago

If so, I suspect dart_code may currently only be put inside class or something like that... Thus, we may enhance frb a little bit to let it generate dart_code for functions etc. (which is not too hard, since dart_code is nothing but copy-pasting)

Yes, that is actually the case :) I rewrote the documentation: https://github.com/fzyzcjy/flutter_rust_bridge/pull/2313

For me that limitation is ok at the moment. For the general case it is a bit weird that one needs to wrap the dart_code in a struct, and additionally that this struct needs to have another rust fn implemented - or be a parameter of another fn.

Would be good to change that :)

patmuk commented 1 month ago

@fzyzcjy I finally made it :)

Please have a look - I suggest to first start with the documentation in website/docs/guides/how-to/logging.md. Let us discuss if you find something not ergonomic or would prefer it in a different way. I can change or explain why I need to do it this way (and learn from you if that can be done differently :) ).

Here are some topics to address:

  1. I did not implement any tests. pure_dart looked quite complex. Maybe you can do a start here? However, I am using the logging in dart_minimal, but with the default configuration.
  2. I added #[allow(dead_code)] in frb_codegen/src/library/codegen/generator/misc/structs_macro.rs. I did this because I got may warnings of not using mir and custom when calling the code generation. We can revert this, if this warnings just appeared locally to me!
  3. Similarly, I added 
    lints.rust]
unexpected_cfgs = { level = "allow", check-cfg = ['cfg(wasm)'] }

    to frb_rust/Cargo.toml to surpress these warnings.

  4. I removed the opinionated logging from frb_codegen/src/library/utils/logs.rs. I did not replaced it either ... I was not sure if this is logging you use for your own project (in which case you could change to this new implementation) or if this is used within the rust code of frb.
  5. I removed the previous logger implementation in frb_rust/Cargo.tomland frb_rust/src/misc/user_utils.rs
  6. I put the macro in frb_rust/src/misc/logging.rs - any better fitting place is good as well :)
  7. I made the macro's integration dependent to the log feature in frb_rust/src/misc/mod.rs

I did not yet waited for CI to finish - will pick that up later.

fzyzcjy commented 1 month ago

Good job and congratulations!

I will review probably within a day or two, since quite busy now.

patmuk commented 1 month ago

About 4.: I added opinionated_logging again. I saw that this is used to log with build.rs, for which we can't use this logger, as it needs codegen to work.

patmuk commented 1 month ago

@fzyzcjy please have a look at CI as well - I don't know why it is failing :(

fzyzcjy commented 1 month ago

I did not implement any tests

No worries, we can do it last. Yes just do things in pure_dart. Probably add a few lines mimicking e.g. https://github.com/fzyzcjy/flutter_rust_bridge/blob/4f5cf68f56ed56df62264227d81891744c356403/frb_example/pure_dart/test/api/misc_no_twin_example_a_test.dart#L10

I put the macro in

no worries, exported macros are by default used as flutter_rust_brige::something!(). Thus, we do not expose detailed path to end users, and we are free to move to here and there in the future.

I made the macro's integration dependent to the log feature

Sure, if one user wants to forbid that feature, he or she should change the logging logic as well

patmuk commented 1 month ago

Now I am thinking if it wouldn't have been better to implement logging from the other side around: Send Dart logs to rust. That way we might not need macros, as from the Dart code it would just call the rust log function ... I could implement that in addition, have the user configure the direction he likes.

For me personally it would depend how to handle logging configuration best - I here I lack the experience. I mean, would it be better to have log outputs configured via rust (using opinionated_logger, thus one logger for codegen and prod, OSLogger and Android crates, ... maybe some fancy Network logging ...) or better in the Flutter ecosystem (maybe better integrated in the mobile OSs, into firebase crashlytics, etc.).

For now I am undecided and would first gather some experience. I understood that in your product you forward the rust logs to dart as well, right? Nevertheless, I think dart->rust would be very easy to implement (in fact easier than what I did ... duh).

fzyzcjy commented 1 month ago

Send Dart logs to rust

Hmm, if the user is "using Rust for e.g. computation-heavy work for the Flutter app", then I guess he or she will have already setup the Dart logging system before adding frb or Rust code, and thus "send rust logs to dart" would be a good default choice.

My personal choice for my app is also send Rust log to Dart.

That way we might not need macros

I guess it still needs macros (for complex scenarios at least)

patmuk commented 1 month ago

Send Dart logs to rust

(...) I guess he or she will have already setup the Dart logging system before (...)

True.

My personal choice for my app is also send Rust log to Dart.

Thanks for sharing.

That way we might not need macros

I guess it still needs macros (for complex scenarios at least)

True - at least the rust calls to be called by the Dart logging function (if Dart -> rust) need to be wired ... so the macro is a must.

I am currently simplifying the usage a bit. If you like you can already have a look - but I will extend the macro with the same optional parameters as the dart function init_logger so that less code is needed :)

patmuk commented 1 month ago

I finished the improvements - feel free to review this PR!

patmuk commented 1 month ago

Oh, I overlooked that you did already a review! Working through your comments now!

patmuk commented 1 month ago

I finished working through your review. Many thanks!

Now only until tests are to-be-done. I am contemplating to implement a Dart-to-Rust option as well, while I have this fresh in mind. I would do so by splitting up the macro (internally) and giving it a 4th parameter, on which dependence different code parts are inserted, so either rust logs are forwarded via a stream to dart (current implementation) or dart logs are processed by dart calling the rust log function.

Sounds easy, but is probably not needed, as discussed before ...

For now I will hold still until we resolved the above discussions.

The bloating of the minimal.rs is a bad think, would be good if we find a way to put the logging code into frb_generated or similar.

fzyzcjy commented 1 month ago

Great and you are welcome!

Dart-to-Rust option

I guess we can add this option in doc only and mention "if you want it, create an issue and discuss your scenarios". I personally think this will not be very frequently needed, thus we can save time efforts not implementing it, but I can be totally wrong!

The bloating of the minimal.rs is a bad think, would be good if we find a way to put the logging code into frb_generated or similar.

Totally agree (see my reply above)

patmuk commented 1 month ago

I did not implement any tests

No worries, we can do it last. Yes just do things in pure_dart. Probably add a few lines mimicking e.g. https://github.com/fzyzcjy/flutter_rust_bridge/blob/4f5cf68f56ed56df62264227d81891744c356403/frb_example/pure_dart/test/api/misc_no_twin_example_a_test.dart#L10

I looked into it, but it is not so easy to understand ... it would help, if you can give me a kick-start here: If you could implement a test for the default settings, meaning implementing a test that calls enable_frb_logging!() on the rust side and executes a log::info!('whatever rust') from Rust and a FRBLogger.getLogger(whatever dart) from Dart, that would be great!

Based on that I can do all the other scenarios.

As for asserting the test result, if it is too complicated to assert what has been written to the console we can set a custom log function that writes into a variable instead.