search

hyperledger-archives / indy-sdk-react-native

React Native wrapper around Indy SDK Java and Objective-C wrappers.
Apache License 2.0
22 stars 14 forks source link

feat!: update iOS installation to hyperledger #56

Closed JamesKEbert closed 2 years ago

JamesKEbert commented 2 years ago

This PR expands upon @niallshaw-absa's efforts in #54, and places the IndySDK framework file and podspec within this Hyperledger repo. I've left the podspec author under @niallshaw-absa, as you performed the majority of the work, but I'd be happy to change this if desired. Again, thank you to your efforts @niallshaw-absa!

Additionally, this PR updates:

If any testing is desired, an essentially identical setup is available using 

source 'https://github.com/JamesKEbert/indy-sdk-react-native'

I've marked this as a breaking change, as noted by @TimoGlastra in https://github.com/hyperledger/indy-sdk-react-native/pull/54#issuecomment-1217811816

jakubkoci commented 2 years ago

@JamesKEbert Thanks for the updates 👍

Hermes is actually not needed if set android:extractNativeLibs="true" see https://github.com/hyperledger/indy-sdk/issues/2346#issuecomment-944254027. So, maybe just mention it in your Hermes description.

I'm not sure if it's a good idea to put binary files into the repo with source code 🤔 🤷‍♂️ If we add Android libs It might bloat the size of the repo to almost 50 MB and that might grow with eventual updates of those libraries (I'm not sure how GitHub handles incremental updates for binary files now). Does Hyperledger have some shareable storage for such cases?

JamesKEbert commented 2 years ago

Hey @jakubkoci, thanks for the review & comments! I didn't quite follow on your comment surrounding Hermes, based off of what you mentioned in the linked thread, android:extractNativeLibs="true" will also crash the app with a ZMQ error, so I'm not quite sure how that helps.

App with React Native 0.64.2 also crashes with ZMQ error when you set the following flag in AndroidManifest.xml file

Let me know what I'm missing though.

I'm not sure if it's a good idea to put binary files into the repo with source code 🤔 🤷‍♂️ If we add Android libs It might bloat the size of the repo to almost 50 MB

I do agree on this, but I don't think we need to add the Android libs, as the existing CI/CD referenced for Android installation works fine for Android, it's just iOS it's broken with as the last version available is 1.8.2.

and that might grow with eventual updates of those libraries (I'm not sure how GitHub handles incremental updates for binary files now).

Given the frequency of updates is low--the last release, 1.16.0, was released in February of 2021, I don't expect the amount of storage required to be immense here. We've had a community issue of not having a consistent location at which the Indy.Framework file has been made available, which has contributed to reducing the ease of use of indy-sdk-react-native.

I am okay if we have a different place in mind for the Indy.Framework, but using the Hyperledger Github seemed like the least controversial location, but I'm definitely not set on it.

Does Hyperledger have some shareable storage for such cases?

@TimoGlastra do you have any insight here?

(I'm not sure how GitHub handles incremental updates for binary files now)

This did make me realize that the zip file should likely have a version in the name, that way we don't override it or break backwards compatibility with any future indy releases.

--

I would definitely like to hear additional thoughts on the above though! Thank you :)

TimoGlastra commented 2 years ago

@TimoGlastra do you have any insight here?

Not sure. Jakub and I have been disagreeing on this for a long time. In my opinion we should first focus on ease of use (so adding it to the repo) and later focus on reducing repo / package size.

We're now just using expo for everything with the indy-sdk-expo-plugin meaning we're not affected by this issue ourselves anymore. That repo has the binaries stored in the repo / npm package / plugin itself.

genaris commented 2 years ago

I told you @JamesKEbert , this was a hot topic some time ago and you revamped it 😛.

While I agree with both sides, I'm a bit curious about why this IndySdk.zip cannot be uploaded to the repo from where it came from (indy-sdk or the sovrin releases). I feel that it would be more 'trustable' a binary stored in repo.sovrin (even if it was uploaded manually) than a repo in github. I'm sure there is a good story behind this: I'll be happy to hear it 😄

jakubkoci commented 2 years ago

@JamesKEbert Ah, it works when it's set to android:extractNativeLibs="false", which is currently the default. Sorry for the confusion. The point is that our app is running without Hermes and connection to the ledger works without a problem.

Regarding the binaries :)

To me knowledge the removing binary files from git is not an easy task:

I did it for our Absa React Native wallet repo when it has 300 MB and I was lucky that we were just 2 developers and I could push to a newly created repo on Github.

If our argument is easy-to-use, then we’re good with the current setup, aren’t we?

If the problem is that Absa is hosting it, which can be an issue when Absa goes out of business or decides to remove it or restrict access, then I 100 % agree with you. But I’m sorry I’m a bit hesitant about an approach when we try to solve it the easiest way for us now without worrying about future developers and maintainers.

TimoGlastra commented 2 years ago

I've been thinking, and what if we add the indy-sdk binary to the release on github. You can add multiple assets, and that way we can download it from the HL github, but it is not actually in the repo.

I've added it as an example to the 0.2.2 release:

For now we can just reference the 0.2.2 release binary, and whenever a new binary is created we attach it to the latest release and hardcode that in the podspec file.

JamesKEbert commented 2 years ago

The approach I've been meaning to pursue was to ask Ry if there are any build hosting resources that Hyperledger has available (which I can still do if we want to). Your solution seems an elegant approach to me though @TimoGlastra!

niall-shaw commented 2 years ago

So then we can change the source property in the podspec to point to the artifact of the relevant release? Edit: ah sorry, I didn't fully read your above comment @TimoGlastra ; sounds good to me!

jakubkoci commented 2 years ago

I've been thinking, and what if we add the indy-sdk binary to the release on github. You can add multiple assets, and that way we can download it from the HL github, but it is not actually in the repo.

💯 👍

JamesKEbert commented 2 years ago

I've updated the PR as discussed in the AFJ Call 10-13-22. Thing of note--since we've updated to 1.16.0 of indy I made the assumption that we'd upload to 0.2.2 the Indy.Framework created by Berend from https://github.com/animo/Indy.Framework that would be named ios-indy-sdk-1.16.0.zip @TimoGlastra. I created a corresponding 1.16.0 podspec as well. I also squashed everything to ensure no binary information would be committed to the repo 👍

niall-shaw commented 2 years ago

Thing of note--since we've updated to 1.16.0 of indy I made the assumption that we'd upload to 0.2.2 the Indy.Framework created by Berend from https://github.com/animo/Indy.Framework that would be named ios-indy-sdk-1.16.0.zip

Looks like we will also have to rename the existing IndySdk.zip to ios-indy-sdk-1.15.0.zip also, right?

TimoGlastra commented 2 years ago

I created a corresponding 1.16.0 podspec as well.

Will the latest podspec automatically be used?

niall-shaw commented 2 years ago

Any further update on this, or can the change be merged and released?

TimoGlastra commented 2 years ago

Just want to get confirmation that it actually works with the new url when you do a pod install / add it to a project. If someone can test this, we can merge it and create a new release

JamesKEbert commented 2 years ago

I've confirmed no raw.githuberusercontent urls are present--unfortunately the source command cannot specify a branch, so it must be main, which means that it must be merged in order to be tested. I can replicate on my personal repo if desired, but assuming the changes look good, I'd rather merge and then test.

Happy to help as needed 👍

JamesKEbert commented 2 years ago

Will the latest podspec automatically be used?

Yes--since we don't specify a version for Indy in the indy-sdk-react-native.podspec it should use the most recent version.