-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add Swift wrapper #96
Conversation
Signed-off-by: conanoc <[email protected]>
Can you review this PR @blu3beri @andrewwhitehead ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor notes, but mainly the header generation code is important as the next time its generated all the changed will be reverted.
I am not a Swift developer so I did not review the swift code in here :).
build-xcframework.sh
Outdated
@@ -125,7 +127,7 @@ cp ../../$HEADER_PATH/$HEADER_NAME Headers/$FRAMEWORK_LIBRARY_NAME.h | |||
mkdir Modules | |||
touch Modules/module.modulemap | |||
cat <<EOT >> Modules/module.modulemap | |||
framework module $FRAMEWORK_LIBRARY_NAME { | |||
framework module $FRAMEWORK_MODULE_NAME { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this changed? I am not too familiar with the module.modulemap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the name of the module used in Objective-C or Swift. I want it to be imported with the name "AriesFramework" instead of "aries_askar". See https://github.com/conanoc/aries-askar/blob/swift_pr/wrappers/swift/Askar/Sources/Askar/Crypto.swift#L2
It's the convention in Swift, and this will not affect other languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong. Swift does not seem to consult this module name when importing a framework. I'll rollback this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
include/libaries_askar.h
Outdated
@@ -7,7 +7,7 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a generated file so if something changes here we should make sure that cbindgen generates the file with these changes as this will be overwritten when a new header is generated. Could you make it gets generated with your changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it's possible. I've created an issue (#99) to track this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
dependencies: ["Askar"]), | ||
.binaryTarget( | ||
name: "AskarFramework", | ||
url: "https://github.com/hyperledger/aries-framework-swift/releases/download/binary-release-askar-v0.2.7/AskarFramework.xcframework.zip", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this url definitive? There is already an xcframework on the releases of this library. Maybe in the future it can just use that one (right now it also includes Android builds though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link should be changed later. I'm thinking of adding a workflow job for this.
Signed-off-by: conanoc <[email protected]>
@andrewwhitehead I get the following error on pg_test. What should I do?
|
The build issue should be fixed by #102 |
@andrewwhitehead Could you merge this and #102 PR? |
I extracted part of this PR to #110 and I'll close this. |
This is a PR to add a Swift wrapper.