fzyzcjy / flutter_rust_bridge

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

Fix the duplicated "dummy_method_to_enforce_bundling" in C header within multi-blocks #1024

Closed dbsxdbsx closed 1 year ago

dbsxdbsx commented 1 year ago

This PR fixes https://github.com/fzyzcjy/flutter_rust_bridge/issues/1014. And I think the idea @WangJiaJun8922 proposed is correct (thanks~), since dummy_method_to_enforce_bundling has nothing to do with the shared stuff in the context of multi-blocks. In any case, giving dummy_method_to_enforce_bundling a suffix of API block/class name makes sense, unless --class-name is not specified.

In addition, the bail function is refined to make log output consistent.

dbsxdbsx commented 1 year ago

I realized that for Mac/Ios, the name for presetting dummy_method_to_enforce_bundling should be changed, too.

@fzyzcjy, for a single block, should the generated be something like dummy_method_to_enforce_bundling_FlutterRustBridgeExample or just dummy_method_to_enforce_bundling? If take the 1st option, then doc need to some modification to mention the prefix issue.
I prefer the 1st one, since finally users would have to now the concept of prefix for dummy_method_to_enforce_bundling when taking multi-blocks.

By the way, where does FlutterRustBridgeExample come from for example with_flutter?

fzyzcjy commented 1 year ago

Good job! Will start reviewing after CI passes

I realized that for Mac/Ios, the name for presetting dummy_method_to_enforce_bundling should be changed, too.

Why? would be great to know more details

By the way, where does FlutterRustBridgeExample come from for example with_flutter?

Which FlutterRustBridgeExample - may need to point out the exact line of code

dbsxdbsx commented 1 year ago

Why? would be great to know more details

Referring to the CI issue from macOS, the signature is still dummy_method_to_enforce_bundling, but not dummy_method_to_enforce_bundling_FlutterRustBridgeExample. Maybe the signature in the swift file needs to be changed before generation.

Which FlutterRustBridgeExample - may need to point out the exact line of code

In example with_flutter, let c_dummy_code = generator::c::generate_dummy(&effective_func_names, &config.class_name); , the config.class_name is FlutterRustBridgeExample, but I didn't find where the specific name is pushed in for this example(I checked CI test yaml, justfile, build.rs).

fzyzcjy commented 1 year ago

Hmm, so is it possible to generate one extra function, dummy_method_to_enforce_bundling, which calls the (possibly multiple) dummy_method_to_enforce_bundling_*?

For example, I see you generate two functions:

image

Can we generate one more:

static int64_t dummy_method_to_enforce_bundling(void) {
      int64_t dummy_var = 0;
    dummy_var ^= dummy_method_to_enforce_bundling_apiclass1();
    dummy_var ^= dummy_method_to_enforce_bundling_apiclass2();
    return dummyvar

etc.

This function should only be generated once (say, in the first file), but should call all dummy_method_to_enforcebundling*

fzyzcjy commented 1 year ago

By doing so, we avoid doing breaking changes, since no end-user facing api is changed :)

dbsxdbsx commented 1 year ago

@fzyzcjy , within https://github.com/fzyzcjy/flutter_rust_bridge/pull/1024/commits/a9b56702e26d3f829e10550238c2fcc5508496ae, besides your proposal, there are other 3 things needed to mention:

  1. the signature for frb_codegen is refined to include the whole configs, which is essential for catching global information when generating for a specific config. Also, this modification paves the way for future refinement of the shared stuff.
  2. replace crate extend with trait mechanism, since it can't be used if it is defined in another module.
  3. For test purpose, add/refine the output of c-header for both "pure_dart" and "pure_dart_multi".
dbsxdbsx commented 1 year ago

@fzyzcjy, running "just precommit" on my win10 machine with flutter 3.7.1. The pubspec.lock would output dart: ">=2.18.0 <3.0.0, But CI would output >=2.18.0 <4.0.0. Is there something need to change?

fzyzcjy commented 1 year ago

The pubspec.lock would output dart: ">=2.18.0 <3.0.0, But CI would output >=2.18.0 <4.0.0. Is there something need to change?

Hmm maybe check the flutter and dart versoin in the CI? I am not sure why that happens

You may also make a hack: Even if your local pubspec.lock differs from CI, keep that change local and do not commit it into git

dbsxdbsx commented 1 year ago

You may also make a hack: Even if your local pubspec.lock differs from CI, keep that change local and do not commit it into git

I did so at first, but then there are inconsistent format issues with dart format. By the way, is it ok to use the latest flutter/dart in CI? I saw flutter 3.7 appear in the previous pr.

fzyzcjy commented 1 year ago

I did so at first, but then there are inconsistent format issues with dart format

Hmm...

is it ok to use the latest flutter/dart in CI?

IMHO yes. What is your opinion?

dbsxdbsx commented 1 year ago

IMHO yes. What is your opinion? At present, what about making the flutter version of CI to be 3.7.1.

fzyzcjy commented 1 year ago

Sure!

fzyzcjy commented 1 year ago

CI is fixed in another PR so by merging the master maybe we are ready to go :)

dbsxdbsx commented 1 year ago

The format issue still exists for ubuntu. Maybe I need to do just precommit in a Linux system. Anyway, @fzyzcjy, could you please check if the rust logic is ok?

fzyzcjy commented 1 year ago

Sure, I will probably review it within a day!

fzyzcjy commented 1 year ago

/cc @temeddix who fixed the CI (https://github.com/fzyzcjy/flutter_rust_bridge/pull/1039)

temeddix commented 1 year ago

Right now just precommit is a bit broken. Post-PR https://github.com/fzyzcjy/flutter_rust_bridge/pull/1051 is waiting to be merged, as this allows just precommit to work again.

fzyzcjy commented 1 year ago

waiting to be merged

Done ;) (given it is green)

dbsxdbsx commented 1 year ago

@temeddix, thanks for your fixed CI issue. But here is a format issue to mention. On win10, with flutter 3.7.3: This line in example with_flutter during just precommit would first format into:

external dynamic /* List<dynamic> */ wire_handle_zero_copy_vec_of_primitive_sync(int n);

Then, in afterward task, it would be reformatted back to

external dynamic /* List<dynamic> */
      wire_handle_zero_copy_vec_of_primitive_sync(int n);
fzyzcjy commented 1 year ago

I am also reproducing this problem. I will fix it in https://github.com/fzyzcjy/flutter_rust_bridge/issues/1053 - just wait until I fix and update master :)

temeddix commented 1 year ago

Good :) I will be busy for a while with my own schedules..

fzyzcjy commented 1 year ago

@temeddix take your time :)

dbsxdbsx commented 1 year ago

I don't understand the issue from CI.

fzyzcjy commented 1 year ago

I don't understand the issue from CI.

Well you may debug it (locally?), try to retry running it to see whether it disappears, or revert relavent changes if non critical

dbsxdbsx commented 1 year ago

I don't understand the issue from CI.

Well you may debug it (locally?), try to retry running it to see whether it disappears, or revert relavent changes if non critical

I can't debug it locally, because I don't have an APPLE machine. And unfortunately, the issue still occurs. And it is weird, IOS is all ok, only macOS failed.

fzyzcjy commented 1 year ago

Hmm which code causes this, and shall we revert it?

dbsxdbsx commented 1 year ago

Hmm which code causes this, and shall we revert it?

A side question, there is not CI test for pure_dart_multi any longer. Are they deleted on purpose?

fzyzcjy commented 1 year ago

There should definitely be tests about that! It may be dropped accidentically when I was refactoring https://github.com/fzyzcjy/flutter_rust_bridge/blob/master/justfile. Feel free to fix it (e.g. in another PR)

dbsxdbsx commented 1 year ago

There should definitely be tests about that! It may be dropped accidentically when I was refactoring https://github.com/fzyzcjy/flutter_rust_bridge/blob/master/justfile. Feel free to fix it (e.g. in another PR)

I mean the the example in CI test file. Meanwhile, I am out of time these 2~3 days.

fzyzcjy commented 1 year ago

Take your time!

I mean the the example in CI test file

Could you please point to that line? Not very sure what is example in CI. The (new) CI file indeed executes the justfile

dbsxdbsx commented 1 year ago

justfile

Take your time!

I mean the the example in CI test file

Could you please point to that line? Not very sure what is example in CI. The (new) CI file indeed executes the justfile

I mean that I only see pure_dart example in the CI folder,but not pure_dart_multi: image

In addition, I also can't find some command that executes the justifle, could you point that out?

fzyzcjy commented 1 year ago

Oh the ci folder calls: https://github.com/fzyzcjy/flutter_rust_bridge/blob/master/justfile

dbsxdbsx commented 1 year ago

justfile

which line? I didn't find it. image

fzyzcjy commented 1 year ago

https://github.com/fzyzcjy/flutter_rust_bridge/blob/master/justfile

many places e.g.

image

dbsxdbsx commented 1 year ago

https://github.com/fzyzcjy/flutter_rust_bridge/blob/master/justfile

many places e.g.

image

No, I mean which line of which file in the CI folder calls the justfile?

fzyzcjy commented 1 year ago

Each just sometihng in ci.yaml will call the something in /justfile file.

image

dbsxdbsx commented 1 year ago

@fzyzcjy, when running just dart_linter_pana in win10, I got issue:

$     just dart_linter_pana
flutter pub global activate pana
Package pana is currently active at version 0.21.26.
Resolving dependencies...
The package pana is already activated at newest available version.
To recompile executables, first run `flutter pub global deactivate pana`.
Installed executable pana.
Activated pana 0.21.26.
cd frb_dart && pana.bat --no-warning --line-length 80 --exit-code-threshold 0
INFO       Running `D:\flutter\bin\cache\dart-sdk\bin\dart.exe --no-analytics --version`...
INFO       Running `flutter.bat --suppress-analytics --no-version-check --version --machine`...
INFO       Running `git rev-parse --show-toplevel`...
INFO       Running `D:\flutter\bin\cache\dart-sdk\bin\dart.exe --no-analytics pub get --no-example`...
INFO       Running `D:\flutter\bin\cache\dart-sdk\bin\dart.exe --no-analytics pub outdated --json --up-to-date --no-dev-dependencies --no-dependency-overrides`...
INFO       Analyzing package...
INFO       Running `D:\flutter\bin\cache\dart-sdk\bin\dart.exe --no-analytics analyze --format machine bin`...
INFO       Running `D:\flutter\bin\cache\dart-sdk\bin\dart.exe --no-analytics analyze --format machine lib`...
INFO       Running `git init`...
INFO       Running `git remote add origin https://github.com/fzyzcjy/flutter_rust_bridge`...
INFO       Running `git remote show origin`...
INFO       Running `git fetch --depth 1 --no-recurse-submodules origin master`...
INFO       Running `git ls-tree -r --name-only --full-tree origin/master`...
INFO       Unable to parse `pubspec.yaml` from git repository. Invalid argument(s): Path "frb_dart/pubspec.yaml" is not normalized.
           Invalid argument(s): Path "frb_dart/pubspec.yaml" is not normalized.
           #0      GitLocalRepository._assertPathFormat (package:pana/src/repository/git_local_repository.dart:223:7)
           #1      GitLocalRepository.showStringContent (package:pana/src/repository/git_local_repository.dart:156:5)
           #2      checkRepository.matchRepoPubspecYaml (package:pana/src/repository/check_repository.dart:91:36)
           #3      checkRepository (package:pana/src/repository/check_repository.dart:159:27)
           <asynchronous suspension>
           #4      SharedAnalysisContext.verifyRepository (package:pana/src/package_context.dart:72:24)
           <asynchronous suspension>
           #5      checkPubspecUrls (package:pana/src/references/pubspec_urls.dart:60:30)
           <asynchronous suspension>
           #6      followsTemplate.checkPubspec (package:pana/src/report/template.dart:186:25)
           <asynchronous suspension>
           #7      followsTemplate (package:pana/src/report/template.dart:254:26)
           <asynchronous suspension>
           #8      createReport (package:pana/src/report/create_report.dart:42:5)
           <asynchronous suspension>
           #9      PackageAnalyzer._inspect (package:pana/src/package_analyzer.dart:315:15)
           <asynchronous suspension>
           #10     PackageAnalyzer.inspectDir.<anonymous closure> (package:pana/src/package_analyzer.dart:143:14)
           <asynchronous suspension>
           #11     withTempDir (package:pana/src/utils.dart:236:12)
           <asynchronous suspension>
           #12     main (file:///C:/Users/dbsx/AppData/Local/Pub/Cache/hosted/pub.dev/pana-0.21.26/bin/pana.dart:176:19)
           <asynchronous suspension>
INFO       Running `D:\flutter\bin\cache\dart-sdk\bin\dart.exe --no-analytics format --output=none --set-exit-if-changed --line-length 80 C:\Users\dbsx\AppData\Local\Temp\pana_73d49721\frb_dart\bin`...
INFO       Running `D:\flutter\bin\cache\dart-sdk\bin\dart.exe --no-analytics format --output=none --set-exit-if-changed --line-length 80 C:\Users\dbsx\AppData\Local\Temp\pana_73d49721\frb_dart\lib`...

## ✗ Follow Dart file conventions (20 / 30)
### [x] 0/10 points: Provide a valid `pubspec.yaml`

<details>
<summary>
Failed to verify repository URL.
</summary>

Please provide a valid [`repository`](https://dart.dev/tools/pub/pubspec#repository) URL in `pubspec.yaml`, such that:  

 * `repository` can be cloned,
 * a clone of the repository contains a `pubspec.yaml`, which:,
    * contains `name: flutter_rust_bridge`,
    * contains a `version` property, and,
    * does not contain a `publish_to` property.

Unable to parse `pubspec.yaml` from git repository. Invalid argument(s): Path "frb_dart/pubspec.yaml" is not normalized.
</details>

### [*] 5/5 points: Provide a valid `README.md`

### [*] 5/5 points: Provide a valid `CHANGELOG.md`

### [*] 10/10 points: Use an OSI-approved license

Detected license: `MIT`.

## ✓ Provide documentation (10 / 10)
### [*] 10/10 points: Package has an example

## ✓ Platform support (20 / 20)
### [*] 20/20 points: Supports 6 of 6 possible platforms (**iOS**, **Android**, **Web**, **Windows**, **MacOS**, **Linux**)

* ✓ Android
* ✓ iOS
* ✓ Windows
* ✓ Linux
* ✓ MacOS
* ✓ Web

## ✓ Pass static analysis (30 / 30)
### [*] 30/30 points: code has no errors, warnings, lints, or formatting issues

## ✓ Support up-to-date dependencies (20 / 20)
### [*] 10/10 points: All of the package dependencies are supported in the latest version

|Package|Constraint|Compatible|Latest|
|:-|:-|:-|:-|
|[`args`]|`^2.3.1`|2.4.0|2.4.0|
|[`build_cli_annotations`]|`^2.1.0`|2.1.0|2.1.0|
|[`colorize`]|`^3.0.0`|3.0.0|3.0.0|
|[`js`]|`^0.6.4`|0.6.7|0.6.7|
|[`meta`]|`^1.3.0`|1.9.0|1.9.0|
|[`path`]|`^1.8.1`|1.8.3|1.8.3|
|[`puppeteer`]|`^2.12.0`|2.22.0|2.22.0|
|[`shelf`]|`^1.3.2`|1.4.0|1.4.0|
|[`shelf_static`]|`^1.1.1`|1.1.1|1.1.1|
|[`shelf_web_socket`]|`^1.0.2`|1.0.3|1.0.3|
|[`tuple`]|`^2.0.1`|2.0.1|2.0.1|
|[`uuid`]|`^3.0.6`|3.0.7|3.0.7|
|[`web_socket_channel`]|`^2.2.0`|2.3.0|2.3.0|
|[`yaml`]|`^3.1.1`|3.1.1|3.1.1|

<details><summary>Transitive dependencies</summary>

|Package|Constraint|Compatible|Latest|
|:-|:-|:-|:-|
|[`archive`]|-|3.3.6|3.3.6|
|[`async`]|-|2.10.0|2.10.0|
|[`collection`]|-|1.17.1|1.17.1|
|[`convert`]|-|3.1.1|3.1.1|
|[`crypto`]|-|3.0.2|3.0.2|
|[`http`]|-|0.13.5|0.13.5|
|[`http_parser`]|-|4.0.2|4.0.2|
|[`logging`]|-|1.1.1|1.1.1|
|[`mime`]|-|1.0.4|1.0.4|
|[`petitparser`]|-|5.2.0|5.2.0|
|[`pointycastle`]|-|3.6.2|3.6.2|
|[`pool`]|-|1.5.1|1.5.1|
|[`source_span`]|-|1.9.1|1.9.1|
|[`stack_trace`]|-|1.11.0|1.11.0|
|[`stream_channel`]|-|2.1.1|2.1.1|
|[`string_scanner`]|-|1.2.0|1.2.0|
|[`term_glyph`]|-|1.2.1|1.2.1|
|[`typed_data`]|-|1.3.1|1.3.1|
</details>

To reproduce run `dart pub outdated --no-dev-dependencies --up-to-date --no-dependency-overrides`.

[`args`]: https://pub.dev/packages/args
[`build_cli_annotations`]: https://pub.dev/packages/build_cli_annotations
[`colorize`]: https://pub.dev/packages/colorize
[`js`]: https://pub.dev/packages/js
[`meta`]: https://pub.dev/packages/meta
[`path`]: https://pub.dev/packages/path
[`puppeteer`]: https://pub.dev/packages/puppeteer
[`shelf`]: https://pub.dev/packages/shelf
[`shelf_static`]: https://pub.dev/packages/shelf_static
[`shelf_web_socket`]: https://pub.dev/packages/shelf_web_socket
[`tuple`]: https://pub.dev/packages/tuple
[`uuid`]: https://pub.dev/packages/uuid
[`web_socket_channel`]: https://pub.dev/packages/web_socket_channel
[`yaml`]: https://pub.dev/packages/yaml
[`archive`]: https://pub.dev/packages/archive
[`async`]: https://pub.dev/packages/async
[`collection`]: https://pub.dev/packages/collection
[`convert`]: https://pub.dev/packages/convert
[`crypto`]: https://pub.dev/packages/crypto
[`http`]: https://pub.dev/packages/http
[`http_parser`]: https://pub.dev/packages/http_parser
[`logging`]: https://pub.dev/packages/logging
[`mime`]: https://pub.dev/packages/mime
[`petitparser`]: https://pub.dev/packages/petitparser
[`pointycastle`]: https://pub.dev/packages/pointycastle
[`pool`]: https://pub.dev/packages/pool
[`source_span`]: https://pub.dev/packages/source_span
[`stack_trace`]: https://pub.dev/packages/stack_trace
[`stream_channel`]: https://pub.dev/packages/stream_channel
[`string_scanner`]: https://pub.dev/packages/string_scanner
[`term_glyph`]: https://pub.dev/packages/term_glyph
[`typed_data`]: https://pub.dev/packages/typed_data

### [*] 10/10 points: Package supports latest stable Dart and Flutter SDKs

## ✓ Support sound null safety (20 / 20)
### [*] 20/20 points: Package and dependencies are fully migrated to null safety!

Points: 120/130.
error: Recipe `dart_linter_pana` failed on line 166 with exit code 127

What does Path "frb_dart/pubspec.yaml" is not normalized. mean? (the line 166 here refers to cd frb_dart && {{pana}} --no-warning --line-length 80 --exit-code-threshold 0)

dbsxdbsx commented 1 year ago

@fzyzcjy, everything is done. To sum up:

fzyzcjy commented 1 year ago

Good job!

fzyzcjy commented 1 year ago

frb_codegen is for single-block frb_codegen_multi for multi block

@dbsxdbsx - maybe we should update the doc to tell users how to use it? I guess we should add a few /// ... comments to frb_codegen and frb_codegen_multi, and briefly mention that users should read corresponding doc in our mini book

fzyzcjy commented 1 year ago

Hmm there was a reply here asking how to use it, but later it seems to be deleted by the author?

Alex-A4 commented 1 year ago

Hmm there was a reply here asking how to use it, but later it seems to be deleted by the author?

Yeah, I found in files

Alex-A4 commented 1 year ago

Hmm, it looks like here's a strange behaviour. Our code root: https://github.com/broxus/nekoton_bridge/tree/attempt_generate_from_multiple_files/packages/nekoton_bridge Look at files: crypto/mnemonic/mnemonic_api.dart crypto/encrypted_key/encrypted_key_api.dart

Both files have class MnemonicTypeG with _$MnemonicTypeG but on rust side both files look into native/src/nekoton/models_api.rs which in dart side doesn't contains such class.

And I still get duplicate error:

conflicting implementations of trait `flutter_rust_bridge::IntoDartExceptPrimitive` for type `nekoton::models_api::MnemonicTypeG`
   --> packages/nekoton_bridge/native/src/encrypted_key_generated.rs:81:1
    |
81  | impl support::IntoDartExceptPrimitive for MnemonicTypeG {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `nekoton::models_api::MnemonicTypeG`
    |
   ::: packages/nekoton_bridge/native/src/mnemonic_generated.rs:124:1
    |
124 | impl support::IntoDartExceptPrimitive for MnemonicTypeG {}
    | ------------------------------------------------------- first implementation here

You can clone repo from branch attempt_generate_from_multiple_files and run cargo build inside native folder

dbsxdbsx commented 1 year ago

frb_codegen is for single-block frb_codegen_multi for multi block

@dbsxdbsx - maybe we should update the doc to tell users how to use it? I guess we should add a few /// ... comments to frb_codegen and frb_codegen_multi, and briefly mention that users should read corresponding doc in our mini book

I think the doc with /// ... is enough to explain frb_codegen_multi and frb_codegen. I guess the signatures of the 2 functions are easy to read, right?

What about doing a little modification for the function docs like this:

/// when the API is only defined in 1 rust file(block), take this one for generation, where `config`
/// is the instance containing all information to the API file(block), and `all_symbols` contains
/// all unique APIs defined in the file mentioned above.
/// If APIs are defined in more than 1 rust file(block), use `frb_codegen_multi` instead.
pub fn frb_codegen(config: &config::Opts, all_symbols: &[String]) -> anyhow::Result<()> {
...
}

/// the function is used only for cases with multi-blocks when there are
/// more than 1 API block. And because the current block to deal with needs information
/// from all other blocks, so `all_configs` is used here,
/// with `index` refers to the place of the current block to deal with.
/// For details on how to take advantage of multi-blocks, please refers to this article: https://cjycode.com/flutter_rust_bridge/feature/multiple_files.html
pub fn frb_codegen_multi(
    all_configs: &[config::Opts],
    index: usize,
    all_symbols: &[String],
) -> anyhow::Result<()> { 
...
}
dbsxdbsx commented 1 year ago

Both files have class MnemonicTypeG with _$MnemonicTypeG but on rust side both files look into native/src/nekoton/models_api.rs which in dart side doesn't contains such class.

@Alex-A4, for your issue:

  1. I wonder if this issue only occurs after updating to v1.65.1?(I notice that you used v1.64.0 before) If this is the case, it is weird, because I only update the generated c-header files, which should no effect other parts of frb.
  2. Though I'm not familiar to the conflict issue with trait(I didn't implement generation for feature trait), it seems that it is some conflict like this. Maybe it is a good time to review your issue again after the shared part has been totally decoupled.
fzyzcjy commented 1 year ago

I think the doc with /// ... is enough to explain frb_codegen_multi and frb_codegen.

I think so