Skip to content
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

Avoid conflicted and redundant FFI with generation over multiple blocks of rust Api #527

Merged
merged 64 commits into from
Jul 5, 2022

Conversation

dbsxdbsx
Copy link
Contributor

@dbsxdbsx dbsxdbsx commented Jun 22, 2022

What's new

  1. For Api defined by users explicitly, now frb could check confliction over Api defined in different blocks.
  2. Flag exclude_sync_execution_mode_utility is needless now, all related code is deleted. And in rust, only the first block would contain Api free_WireSyncReturnStruct.
  3. In dart, after introducing feature: multiple blocks of files in one command #516, ALL rust Api are generated in EVERY generated dart file, within class like class xxxWire implements FlutterRustBridgeWireBase {, even some of them are not used for the block. Now this is fixed to a great degree.
  4. For implicit Api, like new_uint_8_list, which is automatically generated when there is an Api with String as a parameter, it would raise conflicts with multiple blocks, [Bug] "symbol new_uint_8_list is already defined" when using String as parameter in multiple rust module #511. Now it is fixed over blocks, by making each conflicted Api name with a number suffix. For example, If there are 2 blocks both needing this Api, then the Api would be defined as new_uint_8_list_1 and new_uint_8_list_2 respectivley.

Thoughts on avoiding FFI conflicts over blocks

There are generally two approaches to avoid this:

  1. Put the conflicted Api into another shared file.
  2. Give each conflicted Api a suffix to make them not the same among all blocks.

The 1st approach needs to add new file(s) and raise complex in building logic. Also, this way seems to be not a good practice especially in a big project with different teams in charge of different blocks--- it is not decoupling.
The only advantage of the 1st is that it has less code. And the 2ed approach just have the opposite features.
Therefore, for similar usage as before, for decoupling, and for not too much logic modification in frb, I chosen the 2ed approach.

Todos

  • for feature 1, now it can't do checking over conflicts among the same block. May do it later.
  • for feature 3, implicitly defined Api, like new_uint_8_list_1,new_uint_8_list_2 would still be written to ALL generated dart files, it is not a big deal, but not perfect.
  • Some test examples over multiple blocks are needed, like that in flutter_rust_bridge\frb_example\pure_dart\rust.
  • doc (no here, but this pr: Doc for generation with multiple blocks of Api #552)

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 22, 2022

Good job! Will review once everything is finished and CI passes

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 22, 2022

And I do think some "should panic" examples are also needed--- How to make it, does it need some modification over GitHub CI test?

Agree. Maybe need some modification I guess. Feel free to modify .github folder, and your change to CI will also be run and you can see it in the PR's panel

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 22, 2022

like new_uint_8_list_1,new_uint_8_list_2 would still be written to ALL generated dart files, it is not a big deal, but not perfect.

This will result in bigger generated code size. As we know, when building android/ios app, app size is very important.

@dbsxdbsx
Copy link
Contributor Author

dbsxdbsx commented Jun 22, 2022

like new_uint_8_list_1,new_uint_8_list_2 would still be written to ALL generated dart files, it is not a big deal, but not perfect.

This will result in bigger generated code size. As we know, when building android/ios app, app size is very important.

Frankly, I didn't find a good way to excluding the implicit symbols, seems needing another extraction on generator.extern_func_collector.names as that in "Phase: Generate Rust code", and put it before calling frb_codegen(). Or is there a better idea?

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 22, 2022

Maybe yes, at least need to create an intermediate representation instead of directly output some string

@dbsxdbsx
Copy link
Contributor Author

dbsxdbsx commented Jun 24, 2022

@fzyzcjy, the left CI failures are the same --- difference between typedef uintptr_t = ffi.UnsignedLong; and typedef uintptr_t = ffi.UnsignedLongLong; My pr should not change on this, is this introduced by bump test? How to deal with it?

By the way, acording to the usage, I think ffi.UnsignedLongLong; is correct.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 24, 2022

That is because when you run codegen on different platforms that uintptr is different. I have indeed hacked it:

run: sed -i '' -e 's/^typedef uintptr_t =.*$/typedef uintptr_t = ffi.UnsignedLong;/g' frb_example/pure_dart/dart/lib/bridge_generated.dart

So, the solution (or, hack...) is: After running codegen on your computer, before committing you code, please manually (or write a script) modify that file and replace it with UnsignedLong

@dbsxdbsx
Copy link
Contributor Author

@fzyzcjy, I added an example for testing Apis consisting of multiple blocks in folder pure_dart_multi and modify the ci.yaml accordingly. Could you please manually check them?
In addition, I still wonder how to build a "should panic" example---Is there an example for ci.yaml ?

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 26, 2022

In addition, I still wonder how to build a "should panic" example---Is there an example for ci.yaml ?

Not yet. But I guess you can assert non-zero exit code in shell

frb_example/pure_dart_multi/README.md Outdated Show resolved Hide resolved
frb_example/pure_dart_multi/dart/lib/main.dart Outdated Show resolved Hide resolved
frb_example/pure_dart_multi/dart/valgrind_util.py Outdated Show resolved Hide resolved
frb_example/pure_dart_multi/rust/external/Cargo.toml Outdated Show resolved Hide resolved
@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 26, 2022

Good job! Except for a few points that we can discuss

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 27, 2022

@dbsxdbsx
Copy link
Contributor Author

@fzyzcjy, now almost everything is ready. If no other issue, I think it time to merge. I would refine doc correspondingly in another pr --- I need link to example modified here, so this pr should be published before writing doc.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 27, 2022

Sure. Just fix the CI otherwise we will have a broken CI on master which is terrible

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 27, 2022

Use a yaml verifier to check it.

Comment on lines 5 to 8
# if [[ -z "${CARGO_TARGET_DIR}" ]]; then
# echo 'Please set environment variable CARGO_TARGET_DIR'
# exit 1
# fi
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporary hack to check CI is ok, just remember not to submit to the final PR :)

Copy link
Contributor Author

@dbsxdbsx dbsxdbsx Jul 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporary hack to check CI is ok, just remember not to submit to the final PR :)

Actually, I am trying to figure out what make this issue;

And I still think there should be api_1.rs and api_2.rs in pure_dart_multi\rust\src (making suffix consistent), but not api.rs and api_2.rs.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think there should be api_1.rs and api_2.rs in pure_dart_multi\rust\src, but not api.rs and api_2.rs.

Sure, you can put those two names.

Btw it will be even greater if it is named with meaning, e.g. api_apple, api_orange, etc. Otherwise people may wrongly think only numbers are supported.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thread 'main' panicked at 'called Result::unwrap() on an Err value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', /home/runner/work/flutter_rust_bridge/flutter_rust_bridge/frb_codegen/src/config.rs:379:77

What about print out some error message during this unwrap. For example, replace .unwrap() with .expect("the file is: {}", your_file_name). Then you know which file is it thinking.

For example, it maybe because, when assembling filename, you add an extra / etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think there should be api_1.rs and api_2.rs in pure_dart_multi\rust\src, but not api.rs and api_2.rs.

Sure, you can put those two names.

Btw it will be even greater if it is named with meaning, e.g. api_apple, api_orange, etc. Otherwise people may wrongly think only numbers are supported.

I would give detailed explanation in doc afterwards.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw you can also debug locally by running those commands on your computer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw you can also debug locally by running those commands on your computer

Now I am pretty sure it is caused by the shared flutter_rust_bridge_example. Look at here: I switch the order of single and multi blocks, and as expected, the latter one is ruined, while the former one is ok. And in this case, they both used the same flutter_rust_bridge_example-3b9163c44a994e76.

But... how to fix it?

Copy link
Owner

@fzyzcjy fzyzcjy Jul 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, instead of one valgrind script in CI, lets create two "job"s:

  • valgrind script (the old one)
  • pure_dart_multi

Then, the two runs in parallel without any interference

Btw we already have a ton of jobs. For example, valgrind test, run codegen, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, instead of one valgrind script in CI, lets create two "job"s:

  • valgrind script (the old one)
  • pure_dart_multi

Then, the two runs in parallel without any interference

Btw we already have a ton of jobs. For example, valgrind test, run codegen, ...

I find a better way. The [lib] from Cargo.toml used by single and multi blocks examples are now separated by different names flutter_rust_bridge_example and flutter_rust_bridge_example_multi. Now there is no valgrind conflict issue.

By the way, to make distinguishable,c6ed19d, I also renamed the package names with flutter_rust_bridge_example_single_block_test and flutter_rust_bridge_example_multi_blocks_test respectively, is that ok?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! LGTM

@fzyzcjy fzyzcjy merged commit 797040d into fzyzcjy:master Jul 5, 2022
@fzyzcjy
Copy link
Owner

fzyzcjy commented Jul 5, 2022

Great job!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants