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

Add script to build the rust sdk locally and from different repo / branch #1582

Merged
merged 3 commits into from
Oct 18, 2023

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Oct 16, 2023

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Add a script to build the Rust SDK.

Motivation and context

Ease the way to test Rust branch, even from forks, or to test a local version of the rust sdk source

Screenshots / GIFs

NA

Tests

  • run the script and see if it works as expected

Tested devices

  • MacOS

Checklist

@bmarty bmarty requested a review from a team as a code owner October 16, 2023 11:33
@bmarty bmarty requested review from jmartinesp and removed request for a team October 16, 2023 11:33
@@ -29,8 +29,12 @@ anvil {
}

dependencies {
// implementation(projects.libraries.rustsdk)
implementation(libs.matrix.sdk)
if (file("${rootDir.path}/libraries/rustsdk/matrix-rust-sdk.aar").exists()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for safety, could we make it so that this if works only for debug builds? (i.e. release builds will always use implementation(libs.matrix.sdk) no matter what?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. It's not possible to commit the sdk: *.aar file are git ignored, so I think it's fine. The idea is to avoid editing this file (and so committing it by mistake)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but adding a releaseImplementation() and haveing the if using only debugImplementation could be an additional layer of safety

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean having something like:

    releaseImplementation(libs.matrix.sdk)
    if (file("${rootDir.path}/libraries/rustsdk/matrix-rust-sdk.aar").exists()) {
        println("\nNote: Using local binary of the Rust SDK.\n")
        debugImplementation(projects.libraries.rustsdk)
    } else {
        debugImplementation(libs.matrix.sdk)
    }

?

I am wondering now if we may want to test the release build with a local SDK :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. That's what I had in mind. In the rare case we'd like to test a release build with a local sdk we'll have to manually change the gradle file.
This seems to me an added level of safety. But if you feel I'm being paranoid don't be afraid to say it, sometimes I am.

Copy link
Member Author

Choose a reason for hiding this comment

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

Works for me. Better safe than sorry!

Copy link
Contributor

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

Thanks for the script, it's a lot easier than doing it all manually! I found a couple of issues, though.

Also, maybe we should check if both ANDROID_NDK_HOME is set and >= v25?

tools/sdk/build_rust_sdk.sh Outdated Show resolved Hide resolved
tools/sdk/build_rust_sdk.sh Outdated Show resolved Hide resolved
git pull

printf "\nBuilding the SDK...\n\n"
./scripts/build.sh -p ${rustSdkPath} -m sdk -t aarch64-linux-android -o ../element-x-android/libraries/rustsdk
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this only work for ARM64 emulators/devices? Maybe the user who builds this wants to use an x86-64 emulator in Linux/Windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'll take this as a limitation of the script, but i will update the message so that it will be clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could use uname -m to get the architecture and invoke the correct option.
We could reasonably assume, being this for local use, crosscompilation is not a goal.

So if uname -m is x86_64 we build the x86_64 version.
If uname -m is amd64 we build the aarch64 version.

Copy link
Contributor

Choose a reason for hiding this comment

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

@julioromano
Copy link
Contributor

Thanks for the script, it's a lot easier than doing it all manually! I found a couple of issues, though.

Also, maybe we should check if both ANDROID_NDK_HOME is set and >= v25?

There's v26 since a while and it compiles correctly with it, should we bother keeping the NDK requirement up to date?

@jmartinesp
Copy link
Contributor

Thanks for the script, it's a lot easier than doing it all manually! I found a couple of issues, though.
Also, maybe we should check if both ANDROID_NDK_HOME is set and >= v25?

There's v26 since a while and it compiles correctly with it, should we bother keeping the NDK requirement up to date?

The bindings will fail to build with NDK <23, but maybe it's not worth it since that version is pretty old, and it would also be difficult to check.

@sonarcloud
Copy link

sonarcloud bot commented Oct 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (7fc81ac) 59.01% compared to head (21e2499) 59.01%.
Report is 5 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1582   +/-   ##
========================================
  Coverage    59.01%   59.01%           
========================================
  Files         1192     1192           
  Lines        30844    30844           
  Branches      6338     6338           
========================================
  Hits         18204    18204           
  Misses        9896     9896           
  Partials      2744     2744           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link
Contributor

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/n4v41L

Copy link
Contributor

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes!

@bmarty bmarty merged commit c4bbc4e into develop Oct 18, 2023
19 checks passed
@bmarty bmarty deleted the feature/bma/buildSdk branch October 18, 2023 09:03
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.

None yet

3 participants