-
Notifications
You must be signed in to change notification settings - Fork 155
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
Conversation
@@ -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()) { |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this 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?
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Codecov ReportAll modified lines are covered by tests ✅
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. |
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
There was a problem hiding this 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!
Type of change
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
Tested devices
Checklist