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

ffigen_codelab: Add step_07 #1201

Merged
merged 41 commits into from
Dec 21, 2022
Merged

ffigen_codelab: Add step_07 #1201

merged 41 commits into from
Dec 21, 2022

Conversation

domesticmouse
Copy link
Contributor

Pre-launch Checklist

  • I read the Effective Dart: Style recently, and have followed its advice.
  • I signed the CLA.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-devrel channel on Discord.

@domesticmouse domesticmouse marked this pull request as draft December 2, 2022 23:10
@domesticmouse domesticmouse marked this pull request as ready for review December 3, 2022 09:24
@domesticmouse
Copy link
Contributor Author

PTAL @maks

Btw, you'll be amused to learn that the Duktape integration is breaking on Windows. I'm pretty certain I had it working, but I've got no idea how I broke it. Sigh.

@maks
Copy link
Contributor

maks commented Dec 4, 2022

Looks great @domesticmouse ! Builds and runs fine on Ubuntu for me:

image

And looking at the last CI run looks like you solved the windows issue?

One small thing is that I think src/CMakeLists.txt has a typo on line 14?

@domesticmouse
Copy link
Contributor Author

Nah, I haven't added tests. 😝

@domesticmouse
Copy link
Contributor Author

@maks added a test and I have no idea how to make the test code load the library. Any ideas?

@maks
Copy link
Contributor

maks commented Dec 4, 2022

@domesticmouse Hmm good question! I have an idea, let me have a quick go to see if it will work...

@maks
Copy link
Contributor

maks commented Dec 4, 2022

@domesticmouse ok I got a quick unit test working here for me: maks@569fb6d

using the tip from dart-archive/ffi#39 (comment)

My quick hack is only for Linux so it will need to be generalised for the diff paths if you want to test on all OS's.

I'm also not sure why it doesn't seem to work for widget tests, but maybe for widget tests it would be better to not use the real native lib, ie. to have a riverpod provider for the native lib and then in the widgettests use a providerscope override to supply a fake provider?

@domesticmouse
Copy link
Contributor Author

@domesticmouse ok I got a quick unit test working here for me: maks@569fb6d

using the tip from dart-archive/ffi#39 (comment)

My quick hack is only for Linux so it will need to be generalised for the diff paths if you want to test on all OS's.

Thank you @maks 🤓

I'm also not sure why it doesn't seem to work for widget tests, but maybe for widget tests it would be better to not use the real native lib, ie. to have a riverpod provider for the native lib and then in the widgettests use a providerscope override to supply a fake provider?

For this codelab, testing the FFI is the point 😎

@maks
Copy link
Contributor

maks commented Dec 4, 2022

For this codelab, testing the FFI is the point sunglasses

Yes of course, sorry testing with the plugin in a Flutter widget test is the important part! 👍🏻
I can see you spotted the issue with the widget test was just the string matching, doh! sorry I should have spotted that too.

@domesticmouse
Copy link
Contributor Author

Inspecting the generated DLL files with DLL Export Viewer, I'm seeing the DLL generated in Step 3 has appropriate exported entry points, yet the one in step 5 has nothing exported. I have vague recollections that windows requires something specific to export DLL entries?

@domesticmouse
Copy link
Contributor Author

Status summary:

  • Example is complete - JavaScript REPL and has been tested on Windows, macOS, Linux, iOS and Android.
  • Example includes minimal smoke test to prove library integration. Works locally on macOS, Windows, Linux
  • GitHub Action CI is passing the smoke test on macOS and Windows, but failing Linux
  • GitHub Action Linux CI can't run flutter build linux --debug which is required to build the Duktape C library.

@domesticmouse
Copy link
Contributor Author

@godofredoc Any ideas on how I can integrate the appropriate GitHub actions to install the required tooling to make compiling the Flutter linux app work?

@maks
Copy link
Contributor

maks commented Dec 5, 2022

@godofredoc Any ideas on how I can integrate the appropriate GitHub actions to install the required tooling to make compiling the Flutter linux app work?

sorry to butt in here @domesticmouse but could you perhaps add a script step that does sudo apt install ninja-build ?

@domesticmouse
Copy link
Contributor Author

sorry to butt in here @domesticmouse but could you perhaps add a script step that does sudo apt install ninja-build ?

Good call, that worked

@domesticmouse
Copy link
Contributor Author

PTAL @RedBrogdon

I need to land this to complete the codelab write up.

Copy link
Contributor

@RedBrogdon RedBrogdon left a comment

Choose a reason for hiding this comment

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

LGTM. Is there anything we need to do with the Duktape license other than make sure it's included?

@domesticmouse domesticmouse merged commit e666a98 into flutter:main Dec 21, 2022
@domesticmouse domesticmouse deleted the ffigen-step07 branch December 21, 2022 01:35
@domesticmouse
Copy link
Contributor Author

LGTM. Is there anything we need to do with the Duktape license other than make sure it's included?

Duktape is MIT licensed (https://github.com/svaarala/duktape/blob/master/LICENSE.txt). Should we add a licenses section to the App UI?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants