search

openwallet-foundation / askar

Secure storage designed for Hyperledger Aries agents.
Apache License 2.0
63 stars 51 forks source link

Update Rust dependencies and versions #312

Open andrewwhitehead opened 2 months ago

swcurran commented 2 months ago

@andrewwhitehead -- I'm guessing you have found this by now, but in the build.yaml workflow, in the Android section, there is a hard-coded reference to RUST_VERSION set to 1.67 rather than using the environment variable. Hopefully that will fix the failed test.

andrewwhitehead commented 2 months ago

Rust 1.67 is currently the highest version we can use for Android to keep compatibility with NDK 17. Unfortunately sqlx does not support this version (1.74 may be the minimum) in its 0.8 series, and the 0.7 series has a significant security issue for which a fix isn't being backported. I'm not sure if there's other issues preventing a newer Rust version from being used for the Android build. @berendsliedrecht ?

swcurran commented 2 months ago

So that would imply dropping support for an (more than one?) older version of Android if we bumped the Rust version? Sounds necessary. What versions are we talking about. @cvarjao @jleach — heads up on this one — see Andrew’s comment above.

jleach commented 2 months ago

@andrewwhitehead @cvarjao No expert but I think we can update the NDK well past r17. The Bifold wallet uses r25 and a minimum API version of 23 which is Android 6. It looks like NDK 25 goes all the way back to support Android 4 / API 19.

        buildToolsVersion = "33.0.2"
        minSdkVersion = 23
        compileSdkVersion = 34
        targetSdkVersion = 34
        ndkVersion = "25.1.8937393"

We might be able to bump the NDK version to r27. They dropped KitKat (APIs 19 and 20) support in NDK r26 which is well within our minimum requirements and NDK r27 dropped nothing.

Thoughts?

andrewwhitehead commented 2 months ago

I haven't been able to make it build yet. There's a cross issue related to the updated NDK support but it seems they haven't published a new version yet. Using a suggested ref from the git repository does not appear to fix the problem, which makes me think it may need to be fixed in the aries-builder-images image instead.

swcurran commented 2 months ago

HELP NEEDED

Can anyone help with getting Android to work with the ready-to-release version of Askar? I don't think it would be ideal to drop Android support -- there must be some solution.

@wadeking98 @berendsliedrecht @TimoGlastra @dbluhm @cvarjao @dave-promulgare @jleach -- or anyone else that knows Android.

From what I can tell, we need to thread the needle of supporting a version of the NDK that supports a given version of Rust, and vice versa. Surely others have solved this.

cvarjao commented 2 months ago

I had created https://github.com/hyperledger/aries-builder-images with the intention to be shared among rust libraries. That one fixes that issue. I am not sure about support for rust 1.74, but that can be updated for the cross builder image

swcurran commented 2 months ago

So is a PR needed (or an update to this one?) to make use of aries-builder-images? I don’t understand what needs to happen so that we can move forward.

cvarjao commented 1 month ago

@andrewwhitehead , if you can update the Cross.toml to use the default build images, you should be unlocked to go ahead. I don't have a clear PR back to your branch, but check the changes in Cross.toml: https://github.com/andrewwhitehead/aries-askar/compare/upd/deps-2...cvarjao:aries-askar:upd/deps-2?expand=1

swcurran commented 1 month ago

w00t!!! Can we merge? And then release?

andrewwhitehead commented 1 month ago

I think this is okay to merge, but it would be nice if somebody could verify the Android builds are compatible.

swcurran commented 1 month ago

@cvarjao — can you or someone from the BC Wallet team test this out? Or should we push ahead and fix things after?

cvarjao commented 1 month ago

it is cumbersome to test the same artifact, @andrewwhitehead, can we add a publish to github packages or something? or I have added a task to publish to a local registry and and then upload the artifact: https://github.com/andrewwhitehead/aries-askar/compare/upd/deps-2...cvarjao:aries-askar:upd/deps-2?expand=1#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721R401

andrewwhitehead commented 1 month ago

@cvarjao That seems quite complicated, can we use npm pack or do the binary artifacts require a registry?

cvarjao commented 1 month ago

@andrewwhitehead , I tried but that didn't work with pnpm/lerna. I am hesitant with packing in a way that is not the actual one used at the end

ryjones commented 1 month ago

@cvarjao feel free to publish to GHCR (GitHub Packages) if it helps unblock you

swcurran commented 1 month ago

I think the only thing we can do is move forward with a merge and then a release. Unfortunately, we don’t have either nightly builds or release candidates like we do in ACA-Py, so I guess we just “move fast and fix”. Not ideal.

@andrewwhitehead — can you do what is needed for a release, as I don’t know what that is.