Avoid global ffigen #605

Roms1383 · 2022-08-01T05:54:14Z

Fixes #602.

Checklist

An issue to be fixed by this PR is listed above.
New tests are added to ensure new features are working. End-to-end tests are usually in the ./frb_example/pure_dart example, more specifically, rust/src/api.rs and dart/lib/main.dart.
The code generator has been run, and code changes are commited. (Otherwise, CI will fail.)
If this PR adds/changes features, documentations (in the ./book folder) are updated.
CI is passing.

Roms1383 · 2022-08-01T05:54:58Z

Right now my code in frb_codegen is very messy, will have to clean it up following your recommandations.
Also a bunch of tasks are yet to be done : update docs, etc.

fzyzcjy · 2022-08-01T05:56:43Z

Good job! Feel free to mark it as "ready for review" when ready for review :)

Roms1383 · 2022-08-01T05:58:47Z

Honestly there's a bunch of changes in frb_codegen/src/commands.rs that can greatly be improved, but I would prefer to have your feedback first @fzyzcjy.

fzyzcjy · 2022-08-01T06:00:22Z

Honestly there's a bunch of changes in frb_codegen/src/commands.rs that can greatly be improved

Yes, I totally agree - code refactor is very welcomed!

Indeed, when I firstly publish this open source lib, I did not expect it to grow this popular :) So the code style was not that good at that time. And later when it grow bigger and bigger more problems arise.

but I would prefer to have your feedback first

I guess you mean I review the current code? Sure, I will do it in a minute.

fzyzcjy

Good job!

frb_codegen/src/error.rs

frb_codegen/src/commands.rs

move ensure_dependencies, ensure_dev_dependencies, guess_toolchain and related structs

also kept the check on its original value at the bottom of frb_codegen method since it conditionally check on freezed and build_runner, which are only available down the scope

rename DependenciesContext into PackageManager, and rename argument name in functions, add Display implementation and change visibility refactor error MissingDep and InvalidDep with named fields

version req comparison is not implemented in semver crate yet

Roms1383 · 2022-08-01T13:52:16Z

While updating the docs I noticed the same recommendations are literally redundant in 4 different markdown files.
Maybe we could specify it once in one file and link all the other files to it ?

I also noticed the CI won't pass because flutter --version is not recognized, for example this failed job.

Since I started to remove the code to check dart package version requirements comparison (see discussion above), I guess I can even remove more code.

Not sure if it should be marked as ready for review yet ?

fzyzcjy · 2022-08-01T14:05:03Z

the same recommendations are literally redundant in 4 different markdown files.
Maybe we could specify it once in one file and link all the other files to it ?

Sure :)

Is it possible to use this feature: https://rust-lang.github.io/mdBook/format/mdbook.html#including-files

fzyzcjy · 2022-08-01T14:06:07Z

I also noticed the CI won't pass because flutter --version is not recognized, for example this failed job.

Weird... So how do we run other flutter commands? Maybe missing PATH env var or something?

fzyzcjy · 2022-08-01T14:06:47Z

Not sure if it should be marked as ready for review yet ?

Surely depends on you :) I will review 8hr later when getting up tomorrow

Roms1383 · 2022-08-02T06:30:32Z

Sure :)
Is it possible to use this feature: https://rust-lang.github.io/mdBook/format/mdbook.html#including-files

Oh ok, exactly what I was thinking then 👌
p.s: I'll address it last.

Roms1383 · 2022-08-02T06:35:43Z

Weird... So how do we run other flutter commands? Maybe missing PATH env var or something?

As far as I understand flutter_rust_bridge github workflow, so far all the examples were treated as depending onto dart toolchain, as can be seen in the common job step.
And looking at all the steps from ci.yaml, no matter these are pure dart or with flutter, they all setup repo the same.

How do you want to address it ? Add flutter toolchain for all, or split the steps in 2 different jobs ? @fzyzcjy

Roms1383 · 2022-08-02T06:37:51Z

Surely depends on you :) I will review 8hr later when getting up tomorrow

Well then I'm gonna wait to make some more progress to avoid consuming your Github Action credit and your time altogether. I still have to improve on these points 😅

fzyzcjy · 2022-08-02T06:44:49Z

Add flutter toolchain for all, or split the steps in 2 different jobs ?

adding flutter toolchain LGTM, since that is really a flutter project :)

fzyzcjy · 2022-08-02T06:46:57Z

Surely depends on you :) I will review 8hr later when getting up tomorrow

Well then I'm gonna wait to make some more progress to avoid consuming your Github Action credit and your time altogether. I still have to improve on these points 😅

Btw that 8hr has already passed - I reviewed it this morning (about 6hr ago from now).

No worries about CI - the definition, "continuous" integration, means it will run for every commit. Big repos like Rust, Flutter, etc, all do that. Btw GitHub supports open source projects by providing free CI for public repos.

As for my time - I am happy to reply if you have trouble that cannot be solved easily or need review or need discussions etc :)

Roms1383 · 2022-08-02T06:57:24Z

Alright ! A big thanks anyway, that's a pleasure contributing to your repo @fzyzcjy :)

add flutter setup action to run codegen jobs in ci and post release workflows

add install dependencies step for examples in run codegen workflow

fzyzcjy · 2022-08-02T11:08:46Z

Also a pleasure for me :)

fzyzcjy

Good job! Only some nits and I am ready to merge it

.github/workflows/ci.yaml

.github/workflows/post_release.yaml

book/src/setup_codegen.md

book/src/tutorial_with_flutter.md

frb_codegen/src/commands.rs

frb_codegen/src/utils.rs

flutter sdk already contains dart sdk

remove setup_codegen.md

fzyzcjy

LGTM :) we only have a few conversations unresolved yet (which GitHub blocks merging, surely)

fzyzcjy · 2022-08-03T06:49:49Z

going to merge once ci pass :)

Roms1383 · 2022-08-03T07:32:23Z

CI is failing:

should I add architecture: x64 to subosito/flutter-action@v2 ?

fzyzcjy · 2022-08-03T07:42:00Z

Given the another ios test passes, the code should be ok. That ci failure is indeed another quite annoying problem... Feel free to solve it (in another PR)!

fzyzcjy · 2022-08-03T07:43:39Z

@all-contributors please add @Roms1383 for doc

allcontributors · 2022-08-03T07:43:46Z

@fzyzcjy

I've put up a pull request to add @Roms1383! 🎉

Roms1383 added 4 commits August 1, 2022 12:47

🔨 update just lint to format rust code altogether

12c869f

⬆️ add ffi and ffigen dependencies to examples

ef84ec4

♻️ use local dependencies instead of global

26de1f3

💚 remove ffigen as a global dependency from CI

666b799

🎨 lint code

69b74b9

fzyzcjy reviewed Aug 1, 2022

View reviewed changes

Roms1383 added 11 commits August 1, 2022 13:24

🚚 move redundant code from commands to utils

fa0e943

move ensure_dependencies, ensure_dev_dependencies, guess_toolchain and related structs

🔨 update just check to run clippy in frb_codegen altogether

185d469

♻️ refactor ensure dependencies methods

4488c38

♻️ parse dart root only once

9c242b3

also kept the check on its original value at the bottom of frb_codegen method since it conditionally check on freezed and build_runner, which are only available down the scope

♻️ add dart root or default

b6a8c8e

🎨 lint and fix clippy warnings

e90a486

♻️ refactor, improve naming

f8f968b

rename DependenciesContext into PackageManager, and rename argument name in functions, add Display implementation and change visibility refactor error MissingDep and InvalidDep with named fields

🎨 simplify code

374de6b

🔥 remove version req comparison

92b60ba

version req comparison is not implemented in semver crate yet

✏️ remove unecessary fully qualified syntax

9b9ec7d

📝 update install docs

e6ec7da

Merge branch 'master' into chore/avoid-global-ffigen

fe86a45

Roms1383 added 4 commits August 2, 2022 14:04

💚 add flutter setup action

310c89d

add flutter setup action to run codegen jobs in ci and post release workflows

🎨 fix clippy warning

6c31eea

💚 add install deps step

4e79ebe

add install dependencies step for examples in run codegen workflow

📝 reuse parts of the docs and fix mardown lint warnings

63603f4

Roms1383 marked this pull request as ready for review August 2, 2022 09:54

Update deps.md

9525d5c

fzyzcjy reviewed Aug 2, 2022

View reviewed changes

Roms1383 added 7 commits August 3, 2022 08:31

Merge branch 'fzyzcjy:master' into chore/avoid-global-ffigen

98f3a19

💚 remove unused install step

c8575ce

flutter sdk already contains dart sdk

🔥 remove unused markdown file

15d6e46

remove setup_codegen.md

✏️ fix doc wording

ef86e5b

♻️ refactor pattern matching into condition

f6b6366

♻️ reuse impl method

94c11a4

♻️ add version method to package version

d56544d

Roms1383 requested a review from fzyzcjy August 3, 2022 01:43

fzyzcjy reviewed Aug 3, 2022

View reviewed changes

Roms1383 added 2 commits August 3, 2022 13:36

⏪ remove install deps in CI

01ee615

💚 install deps in CI

c09c62e

fzyzcjy merged commit e742f07 into fzyzcjy:master Aug 3, 2022

allcontributors bot mentioned this pull request Aug 3, 2022

docs: add Roms1383 as a contributor for doc #610

Merged

Roms1383 mentioned this pull request Aug 3, 2022

Chore/improve version checking #613

Merged

5 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid global ffigen #605

Avoid global ffigen #605

Roms1383 commented Aug 1, 2022 •

edited

Loading

Roms1383 commented Aug 1, 2022

fzyzcjy commented Aug 1, 2022

Roms1383 commented Aug 1, 2022

fzyzcjy commented Aug 1, 2022 •

edited

Loading

fzyzcjy left a comment

Roms1383 commented Aug 1, 2022

fzyzcjy commented Aug 1, 2022

fzyzcjy commented Aug 1, 2022

fzyzcjy commented Aug 1, 2022

Roms1383 commented Aug 2, 2022

Roms1383 commented Aug 2, 2022

Roms1383 commented Aug 2, 2022

fzyzcjy commented Aug 2, 2022

fzyzcjy commented Aug 2, 2022 •

edited

Loading

Roms1383 commented Aug 2, 2022

fzyzcjy commented Aug 2, 2022

fzyzcjy left a comment

fzyzcjy left a comment •

edited

Loading

fzyzcjy commented Aug 3, 2022

Roms1383 commented Aug 3, 2022

fzyzcjy commented Aug 3, 2022

fzyzcjy commented Aug 3, 2022

allcontributors bot commented Aug 3, 2022

Avoid global ffigen #605

Avoid global ffigen #605

Conversation

Roms1383 commented Aug 1, 2022 • edited Loading

Checklist

Roms1383 commented Aug 1, 2022

fzyzcjy commented Aug 1, 2022

Roms1383 commented Aug 1, 2022

fzyzcjy commented Aug 1, 2022 • edited Loading

fzyzcjy left a comment

Choose a reason for hiding this comment

Roms1383 commented Aug 1, 2022

fzyzcjy commented Aug 1, 2022

fzyzcjy commented Aug 1, 2022

fzyzcjy commented Aug 1, 2022

Roms1383 commented Aug 2, 2022

Roms1383 commented Aug 2, 2022

Roms1383 commented Aug 2, 2022

fzyzcjy commented Aug 2, 2022

fzyzcjy commented Aug 2, 2022 • edited Loading

Roms1383 commented Aug 2, 2022

fzyzcjy commented Aug 2, 2022

fzyzcjy left a comment

Choose a reason for hiding this comment

fzyzcjy left a comment • edited Loading

Choose a reason for hiding this comment

fzyzcjy commented Aug 3, 2022

Roms1383 commented Aug 3, 2022

fzyzcjy commented Aug 3, 2022

fzyzcjy commented Aug 3, 2022

allcontributors bot commented Aug 3, 2022

Roms1383 commented Aug 1, 2022 •

edited

Loading

fzyzcjy commented Aug 1, 2022 •

edited

Loading

fzyzcjy commented Aug 2, 2022 •

edited

Loading

fzyzcjy left a comment •

edited

Loading