Skip to content

Use libindy with rusql fix for ios build#306

Merged
mirgee merged 11 commits intomasterfrom
update/libindy-rusqlfix
Jun 28, 2021
Merged

Use libindy with rusql fix for ios build#306
mirgee merged 11 commits intomasterfrom
update/libindy-rusqlfix

Conversation

@Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Jun 23, 2021

Includes new version of libindy with this changes which fixes a bug present on iOS where thread panics on wallet opening.

In order to successfully build for iOS however, we have to use the vendored feature for openssl dependency. This, in turn, breaks compilation for the musl C environment on Alpine. Hence, X86_64_ALPINE_LINUX_MUSL_OPENSSL_NO_VENDOR environment variable was set as a way to not use the vendored flag on Alpine. This is a temporary fix, as the plan is to remove the dependency on openssl completely.

Signed-off-by: Patrik Stas patrik.stas@absa.africa
Signed-off-by: Miroslav Kovar miroslavkovar@protonmail.com

@Patrik-Stas Patrik-Stas requested a review from a team June 23, 2021 12:06
@Patrik-Stas Patrik-Stas force-pushed the update/libindy-rusqlfix branch from 1cdb47d to 2347094 Compare June 23, 2021 12:08
@Patrik-Stas
Copy link
Contributor Author

We would have to release indy-sdk to get into android, as for Android we rely on prebuilt libindy binaries

@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2021

Codecov Report

Merging #306 (f6fb54e) into master (2714cef) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #306   +/-   ##
=======================================
  Coverage   49.66%   49.66%           
=======================================
  Files         154      154           
  Lines       19607    19606    -1     
  Branches     6190     6199    +9     
=======================================
  Hits         9738     9738           
- Misses       5072     5076    +4     
+ Partials     4797     4792    -5     
Flag Coverage Δ
integration 22.10% <ø> (+<0.01%) ⬆️
unittests 45.48% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
agency_client/src/error.rs 16.80% <0.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2714cef...f6fb54e. Read the comment docs.


INDY_VERSION="v1.16.0"
#INDY_VERSION="efb7215" # this is "v1.16.0" + rusql update fix + (number of other commits on master branch)
INDY_VERSION="b4b330ef3" # this is "v1.16.0" tag
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of changing this from v1.16.0 to the tag v1.16.0 commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Experiment ^_^

@mirgee
Copy link
Contributor

mirgee commented Jun 23, 2021

Moreover, why do you link the binary against a different version of the indy crate?

...
indy = "1.16.0"
indy-sys = "1.16.0"
...

@Patrik-Stas
Copy link
Contributor Author

Moreover, why do you link the binary against a different version of the indy crate?

...
indy = "1.16.0"
indy-sys = "1.16.0"
...

Good catch 👍 thanks

@mirgee mirgee force-pushed the update/libindy-rusqlfix branch from 3ccdf15 to 9d34882 Compare June 24, 2021 13:06
Patrik-Stas and others added 8 commits June 24, 2021 20:22
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@mirgee mirgee force-pushed the update/libindy-rusqlfix branch from 997a598 to b3f2b7e Compare June 24, 2021 18:22
mirgee added 3 commits June 24, 2021 21:11
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
@mirgee
Copy link
Contributor

mirgee commented Jun 25, 2021

Successfully verified that the error on wallet opening is not present on iOS device with this build anymore.

rmp-serde = "0.13.7"
base64 = "0.8.0"
openssl = { version = "0.10.29" }
openssl = { version = "0.10.35", features = ["vendored"] }
Copy link
Contributor Author

@Patrik-Stas Patrik-Stas Jun 28, 2021

Choose a reason for hiding this comment

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

I was looking at this and got concerned about the feature description

The vendored copy will not be configured to automatically find the system’s root certificates, but the openssl-probe crate can be used to do that instead.

So I went through actual usage of openssl crate and it turn out it might be an overkill. What's being used is:

use openssl::sha::sha256
use openssl::bn::BigNum;

I somewhat presumed openssl is there because reqwest needs it, but reqwest comes has its own tls dependency https://github.com/seanmonstar/reqwest/blob/master/Cargo.toml

So our openssl usage is not actually ever dealing with certificates, so the quote on top of this comment doesn't matter. I think ultimate solution will be to get rid of openssl dependency.
Perhaps:

But lets do that separately

@Patrik-Stas
Copy link
Contributor Author

@mirgee I am the original submitter of this PR, so I can't click approve as you mostly did the heavy lifting.

So please feel free to approve on my behalf, just note the comments I've left.

@mirgee mirgee added update hotfix High priority fixes of unexpected issues labels Jun 28, 2021
Copy link
Contributor

@mirgee mirgee left a comment

Choose a reason for hiding this comment

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

Approved on behalf of @Patrik-Stas

@mirgee mirgee merged commit 7775022 into master Jun 28, 2021
@mirgee mirgee deleted the update/libindy-rusqlfix branch June 28, 2021 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hotfix High priority fixes of unexpected issues pre-release update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments