Skip to content
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

Update Rust dependencies and versions #312

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

andrewwhitehead
Copy link
Contributor

No description provided.

Signed-off-by: Andrew Whitehead <[email protected]>
Signed-off-by: Andrew Whitehead <[email protected]>
Signed-off-by: Andrew Whitehead <[email protected]>
Signed-off-by: Andrew Whitehead <[email protected]>
Signed-off-by: Andrew Whitehead <[email protected]>
Signed-off-by: Andrew Whitehead <[email protected]>
swcurran
swcurran previously approved these changes Sep 17, 2024
swcurran
swcurran previously approved these changes Sep 17, 2024
@swcurran
Copy link
Contributor

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

andrewwhitehead commented Sep 18, 2024

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

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

jleach commented Sep 18, 2024

@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?

@swcurran swcurran mentioned this pull request Sep 19, 2024
@andrewwhitehead
Copy link
Contributor Author

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

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

cvarjao commented Sep 23, 2024

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

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

cvarjao commented Oct 7, 2024

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

swcurran commented Oct 7, 2024

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

@andrewwhitehead
Copy link
Contributor Author

andrewwhitehead commented Oct 7, 2024

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

@swcurran
Copy link
Contributor

swcurran commented Oct 8, 2024

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

@cvarjao
Copy link

cvarjao commented Oct 8, 2024

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

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

@cvarjao
Copy link

cvarjao commented Oct 8, 2024

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

ryjones commented Oct 9, 2024

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

@swcurran
Copy link
Contributor

swcurran commented Oct 9, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants