Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

feat!: update iOS installation to hyperledger #56

Merged
merged 2 commits into from
Oct 19, 2022

Conversation

JamesKEbert
Copy link
Contributor

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:

  • The iOS instructions to include configuring of setting the Bitcode and Build Libraries for Distribution, and notes that Simulators are currently not supported.
  • The Android instructions to install Hermes and remove an unnecessary Java import.

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 #54 (comment)

@jakubkoci
Copy link
Contributor

jakubkoci commented Sep 19, 2022

@JamesKEbert Thanks for the updates 👍

Hermes is actually not needed if set android:extractNativeLibs="true" see hyperledger-archives/indy-sdk#2346 (comment). 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
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link

genaris commented Sep 21, 2022

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
Copy link
Contributor

jakubkoci commented Sep 22, 2022

@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
Copy link
Contributor

TimoGlastra commented Oct 12, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor

niall-shaw commented Oct 13, 2022

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
Copy link
Contributor

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.

💯 👍

chore: updated podspec sources, added podspec for indy 1.16.0

Signed-off-by: James Ebert <jamesebert.k@gmail.com>
@JamesKEbert
Copy link
Contributor Author

JamesKEbert commented Oct 14, 2022

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
Copy link
Contributor

niall-shaw commented Oct 14, 2022

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
Copy link
Contributor

I created a corresponding 1.16.0 podspec as well.

Will the latest podspec automatically be used?

s.source = { :http => "https://github.com/hyperledger/indy-sdk-react-native/releases/download/0.2.2/ios-indy-sdk-1.15.zip" }
s.framework = "Indy"
s.source_files = "**/*.h"
s.vendored_frameworks = "IndySdk/Indy.framework"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to change now that we renamed the zip file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be 1.15.0 to standardise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up creating a 1.15.0 and a 1.16.0 podspec (as it seemed Absa may have needed 1.15.0 specifically).

Shouldn't it be 1.15.0 to standardise?

Android was actually updated to 1.16.0 recentlyish, so I think we'd like to standardize on 1.16.0 (especially since it's the most recent version of Indy available).

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Awesome, I added the build from Berend. Did you test it with new urls? I've seen raw.githuberusercontent urls before, so want to make sure this is not a user download link that will break with pod install

@niall-shaw
Copy link
Contributor

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

@TimoGlastra
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@TimoGlastra TimoGlastra changed the title feat!: update iOS installation to Hyperledger and installation instructions feat!: update iOS installation to hyperledger Oct 19, 2022
@TimoGlastra TimoGlastra merged commit 73d770f into hyperledger-archives:main Oct 19, 2022
@JamesKEbert JamesKEbert deleted the chore/podspec branch October 20, 2022 07:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants