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

refactor/ffi: FFI-specific Result types for Safe App and Safe Auth #1068

Merged
merged 6 commits into from Jan 2, 2020

Conversation

@peter-wilkins
Copy link
Contributor

peter-wilkins commented Nov 8, 2019

This PR creates FFI-specific Result types for better separation.

Common FFI error codes have been moved to safe_core.

Resolves #1055

@peter-wilkins peter-wilkins requested a review from nbaksalyar as a code owner Nov 8, 2019
@peter-wilkins peter-wilkins force-pushed the peter-wilkins:move-ffi-errors branch 3 times, most recently from a17085c to 725c5ca Nov 8, 2019
@peter-wilkins

This comment has been minimized.

Copy link
Contributor Author

peter-wilkins commented Nov 9, 2019

I've allowed some clippy lints in order to pass CI

    clippy::missing_safety_doc,
    clippy::redundant_clone,
    clippy::unit_cmp

Happy to remove and fix the warnings - perhaps better in a new PR.

@m-cat

This comment has been minimized.

Copy link
Contributor

m-cat commented Nov 15, 2019

Thanks so much for taking this on! I had a brief look and it looks great. I'll try to do a full review early next week, we've been quite busy!

Happy to remove and fix the warnings - perhaps better in a new PR.

The first lint is fine to allow (I did this myself in another PR), but not sure about the other two. Can you fix the code so clippy doesn't complain? Let me know if you need help with that, of course.

peter-wilkins added a commit to peter-wilkins/safe_client_libs that referenced this pull request Nov 19, 2019
peter-wilkins added a commit to peter-wilkins/safe_client_libs that referenced this pull request Nov 19, 2019
Copy link
Contributor

m-cat left a comment

Great work, thanks a lot!

Went through the review comments on #1063 and marked the ones that have been addressed as "resolved". For future reference, can you avoid making separate PRs? It is easier to review if all review comments and changes are in one place. If you make a new commit (instead of a new PR), it's easier to see changes made by the new commit.

One more note: I tried generating bindings and ran into the following error:

Parsing ffi::errors::codes ("../safe_authenticator/src/ffi/errors/codes/mod.rs")

Looks like pub use crate::ffi::errors::codes::*; will cause safe_bindgen to look for a file called errors/codes/mod.rs (errors/codes.rs should work as well, based on my reading of the safe_bindgen source). Do you mind moving the error codes to this errors/codes.rs file? The existing errors.rs file will have to go into errors/mod.rs. I apologize for the trouble, safe_bindgen is pretty primitive in its parsing of the module tree.

safe_app/src/ffi/errors.rs Outdated Show resolved Hide resolved
safe_app/src/errors.rs Outdated Show resolved Hide resolved
safe_app/src/ffi/access_container.rs Outdated Show resolved Hide resolved
safe_app/src/ffi/ipc.rs Show resolved Hide resolved
safe_app/src/ffi/mutable_data/entries.rs Outdated Show resolved Hide resolved
safe_app/src/lib.rs Outdated Show resolved Hide resolved
safe_app/src/lib.rs Show resolved Hide resolved
safe_authenticator/src/ipc.rs Show resolved Hide resolved
safe_app/src/ffi/tests/nfs.rs Show resolved Hide resolved
@peter-wilkins

This comment has been minimized.

Copy link
Contributor Author

peter-wilkins commented Nov 26, 2019

Getting this error from running cargo build --features=bindings , is this the right command to build the bindings?.

Parsing ffi ("../safe_core/src/ffi/mod.rs")
Parsing ffi::arrays ("../safe_core/src/ffi/arrays.rs")
Parsing ffi::error_codes ("../safe_core/src/ffi/error_codes.rs")
Parsing ffi::ipc::req ("../safe_core/src/ffi/ipc/req.rs")
Parsing ffi::ipc::resp ("../safe_core/src/ffi/ipc/resp.rs")

--- stderr
thread 'main' panicked at '

!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
!   unwrap! called on Result::Err                                              !
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
safe_app/build.rs:64,9 in build_script_build::bindings

Err([Error { level: Error, span: None, message: "can not handle types in other modules (except `libc` and `std::os::raw`)" }])

probably something I did, otherwise could possibly be this change? https://github.com/maidsafe/safe_client_libs/pull/1090/files#diff-8bc5bc46d72b5b112fb69680ab2589cfR12

safe_app/src/lib.rs Outdated Show resolved Hide resolved
safe_app/src/ffi/tests/nfs.rs Show resolved Hide resolved
Copy link
Contributor

m-cat left a comment

Can you move the safe_auth error codes to their own file, like you did in safe_app?

Getting this error from running cargo build --features=bindings , is this the right command to build the bindings?.

Yeah, should be. The errors you're getting were actually caused by me in an earlier commit, great catch! As the error says, safe_bindgen doesn't understand things like arrays::BlsPublicKey, so you'll have to change it to use BlsPublicKey; and use it directly, same for the other array types in that file. Hopefully it's not too much work for you to fix. Thanks!

safe_app/src/errors.rs Outdated Show resolved Hide resolved
safe_core/src/ffi/error_codes.rs Outdated Show resolved Hide resolved
safe_app/src/ffi/errors/codes.rs Outdated Show resolved Hide resolved
@m-cat

This comment has been minimized.

Copy link
Contributor

m-cat commented Nov 27, 2019

The build error is being fixed here:

#1094

Copy link
Contributor

m-cat left a comment

Looks great, this is coming along nicely! Just a couple minor comments from me, so we should be almost done.

safe_authenticator/src/ffi/errors/codes.rs Outdated Show resolved Hide resolved
safe_app/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

m-cat left a comment

This is almost perfect now 😅 Noticed that you're missing an export for the FFI error codes in safe_app/lib.rs:

pub use crate::ffi::errors::codes::*;

safe_authenticator/lib.rs looks fine.

Also, it looks like there's some conflicts between your branch and master -- do you mind rebasing onto master?

@m-cat

This comment has been minimized.

Copy link
Contributor

m-cat commented Nov 29, 2019

Also, looks like the codes module in safe_app isn't public. Looks fine in safe_auth ffi/errors/mod.rs, though.

@peter-wilkins

This comment has been minimized.

Copy link
Contributor Author

peter-wilkins commented Nov 29, 2019

Thanks for all your help and patience. I've merged master - getting new issue with bindgen:

Parsing ffi::error_codes ("../safe_core/src/ffi/error_codes.rs")
None, error: bindgen can not handle constant ERR_ENCODE_DECODE_ERROR

nothing obvious is jumping out at me.
Btw, I'm away for the weekend. Back on Monday.

@peter-wilkins

This comment has been minimized.

Copy link
Contributor Author

peter-wilkins commented Nov 29, 2019

hmm, looks like it is parsing the file twice and succeeds the first time

Parsing ffi::error_codes ("../safe_core/src/ffi/error_codes.rs")
...
Parsing ffi::error_codes ("../safe_core/src/ffi/error_codes.rs")
None, error: bindgen can not handle constant ERR_ENCODE_DECODE_ERROR
@m-cat

This comment has been minimized.

Copy link
Contributor

m-cat commented Nov 29, 2019

Awesome! It looks good now, apart from that issue. I'll look into it. Enjoy your weekend!

@m-cat

This comment has been minimized.

Copy link
Contributor

m-cat commented Dec 2, 2019

I've raised a PR which will fix this issue. It turns out that safe_bindgen did not support parsing of negative constants.

@m-cat

This comment has been minimized.

Copy link
Contributor

m-cat commented Dec 18, 2019

Hey @peter-wilkins, I think all remaining issues here have been resolved. One of our tests was failing (not due to this PR) but it should be fine now. If you can rebase this PR we'll be able to merge it in!

@peter-wilkins

This comment has been minimized.

Copy link
Contributor Author

peter-wilkins commented Dec 19, 2019

Hi @m-cat that's great to hear. I'm on sick leave so can't do it right away. Feel free to finish it off if you don't want to wait.

@m-cat

This comment has been minimized.

Copy link
Contributor

m-cat commented Dec 30, 2019

Just got back from vacation myself. I'll happily refactor this tomorrow, though!

This PR creates FFI-specific Result types for better separation.

Common FFI error codes have been moved to safe_core.

Resolves #1055
@m-cat m-cat force-pushed the peter-wilkins:move-ffi-errors branch 2 times, most recently from 4055996 to caaa2d5 Dec 31, 2019
@m-cat
m-cat approved these changes Dec 31, 2019
Copy link
Member

lionel1704 left a comment

Thanks @peter-wilkins and @m-cat.
Amazing work!

@lionel1704 lionel1704 merged commit 4b0326b into maidsafe:master Jan 2, 2020
15 checks passed
15 checks passed
Rustfmt-Clippy
Details
Build Scripts
Details
Build iOS (aarch64-apple-ios) Build iOS (aarch64-apple-ios)
Details
Build iOS (x86_64-apple-ios) Build iOS (x86_64-apple-ios)
Details
Build Android (armv7-linux-androideabi, prod)
Details
Build Android (armv7-linux-androideabi, dev)
Details
Build Android (x86_64-linux-android, prod)
Details
Build Android (x86_64-linux-android, dev)
Details
Test (ubuntu-latest)
Details
Test (windows-latest)
Details
Test (macOS-latest)
Details
Test Publish
Details
Build iOS Universal (prod)
Details
Build iOS Universal (dev)
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.