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

Fix/example #5

Closed
wants to merge 5 commits into from
Closed

Fix/example #5

wants to merge 5 commits into from

Conversation

Roms1383
Copy link
Contributor

Hello again!
I could correctly build the plugin so far,
then I tried to run the tests in the generated dart_example folder but there was a few minor obstacles:

  • test dependency was missing in dart_example/pubspec.yml

  • fixed dart_example/test/enum_test.dart

  • for a reason that I ignore I had to build with crate type dylib

  • manually copy built Rust library to ../dart_example/libexample.dylib

    probably because on M1 I'm required to run:

    export MEMBRANE_LLVM_PATHS=/usr/local/opt/llvm && cargo run --target=x86_64-apple-darwin

    which builds to example/target/x86_64-apple-darwin/debug/libexample.dylib instead.

    Library is called libexample.dylib, as per generator lib arg
    while some binaries are called generator / generator.d after Cargo bin name
    honestly I'm not sure if it matters, but in case.

Now, it runs smooth :)

@Roms1383
Copy link
Contributor Author

This PR also include #4 😅

@Roms1383 Roms1383 marked this pull request as ready for review February 20, 2022 14:13
@jerel
Copy link
Owner

jerel commented Feb 22, 2022

@Roms1383 thanks for your contribution! I'll review it shortly.

jerel added a commit that referenced this pull request Mar 24, 2022
@jerel
Copy link
Owner

jerel commented Mar 24, 2022

Thank you for your pull request @Roms1383, very well organized. I have pulled in some of your changes as part of 566cb17.

Here's some feedback on the commits that I skipped over and why:

  • Add notes about MacOS M1
    • I think that there is a better way than adding documentation for arch -x86_64 and cargo run --target=x86_64-apple-darwin. We're using membrane in a large project and several of my coworkers are using M1 devices without using x86 features. If an M1 is set up correctly running dart --version should show something like Dart SDK version: 2.15.0-178.1.beta (beta) ... on "macos_arm64" and it should work with the arm64 versions of llvm. cargo run also works without specifying a target.
  • Add dylib to example crate type
    • As far as I know this shouldn't be necessary. We're using crate-type = ["lib", "cdylib", "staticlib"] successfully across Linux, MacOS, and MacOS M1 and in my experimentation adding dylib to the example app breaks on it Linux.
  • Add missing test dev dependency
    • Membrane skips test in the generated pubspec.yaml file because adding it will mean that the dependency is added to all downstream projects that use membrane. I consciously omitted it so as to not force more dependencies than absolutely required. The integration tests run dart pub add test before running and anyone wanting to use dart test directly in the example app will need to as well.
  • Fix enum test
    • This was a side effect from the way that the enum_test.dart case was used. I've added a comment to the test that explains further and a test configuration file that prevents the test from breaking the example build when the project is generated with C style enums instead of class enums.

Thanks for your contribution, I'll close this PR as I merged the above manually.

@jerel jerel closed this Mar 24, 2022
@Roms1383
Copy link
Contributor Author

Ah thanks for your feedback, I learned a couple of things here!

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.

None yet

2 participants