-
Notifications
You must be signed in to change notification settings - Fork 10
feat: android interop #1646
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
base: main
Are you sure you want to change the base?
feat: android interop #1646
Conversation
db8ff8c to
fe45bc1
Compare
|
8debb0b appears to be empty |
fewerner
left a comment
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.
Great job! I think my main concern is the CI runtime. We should think about testing ios and android separately and in parallel. This would save time in setup and test run and have clear CI dependencies.
crypto-ffi/bindings/android-interop/src/main/java/com/wire/androidinterop/MainActivity.kt
Outdated
Show resolved
Hide resolved
|
Nice commit sequencing and commit messages! 💜 |
|
Shouldn't we add all the android-interop files under We already have Swift interop client here so it would make sense to have Android too. |
|
There's a warning in https://github.com/wireapp/core-crypto/actions/runs/19966647475/job/57260303169: |
I had it in the interop directory initially but that doesn't very well when I want to include the core crypto bindings as a project dependency, it needs to be in the same gradle project (crypto-ffi/bindings). My plan is to also move the iOS and web interop clients to the bindings directory. |
That is surprising. Doesn't gradle support path dependencies?
I would rather not we do that. I think we should retain a clear separation between bindings and interop. |
7a9475f to
1709179
Compare
interop/src/clients/android-interop/src/main/java/com/wire/androidinterop/InteropAction.kt
Show resolved
Hide resolved
istankovic
left a comment
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.
This is a good start, but it still needs more work. In particular, we should keep the interop code separate from the bindings.
When I run `make android` on a macOS system I don't want to build dylib libraries because android always uses .so libraries.
The checkout step resets the target folder making it impossible to combine this action with other actions which download something into the target folder.
… running the interop
e5d3ac1 to
6fbd19b
Compare
|
Could you please split commit The changes touching only |
| includeGroupByRegex("com\\.android.*") | ||
| includeGroupByRegex("com\\.google.*") | ||
| includeGroupByRegex("androidx.*") | ||
| } |
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.
This block seems to be unnecessary.
| } | ||
| } | ||
| dependencyResolutionManagement { | ||
| repositoriesMode.set(RepositoriesMode.FAIL_ON_PROJECT_REPOS) |
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.
Nice!
| } | ||
| compileOptions { | ||
| sourceCompatibility = JavaVersion.VERSION_11 | ||
| targetCompatibility = JavaVersion.VERSION_11 |
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.
These should be set to VERSION_17 to be consistent with crypto-ffi.
| compilerOptions { | ||
| jvmTarget.set(JvmTarget.JVM_11) | ||
| } | ||
| } |
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.
Why do we need this kotlin block?
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.
It was part of the default project. Are you saying it's unnecessary specify which jvm we are targeting?
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.
Right, we can just omit the whole kotlin block. 👍
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.
Getting errors about conflicting JVM versions if I remove it.
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.
Strange, it works for me. Did you make sure to sync the compileOptions above with those of crypto-ffi?
I have this:
diff --git a/interop/src/clients/android-interop/build.gradle.kts b/interop/src/clients/android-interop/build.gradle.kts
index 45518ebfc..0aca03fdd 100644
--- a/interop/src/clients/android-interop/build.gradle.kts
+++ b/interop/src/clients/android-interop/build.gradle.kts
@@ -25,13 +25,8 @@ android {
}
}
compileOptions {
- sourceCompatibility = JavaVersion.VERSION_11
- targetCompatibility = JavaVersion.VERSION_11
- }
- kotlin {
- compilerOptions {
- jvmTarget.set(JvmTarget.JVM_11)
- }
+ sourceCompatibility = JavaVersion.VERSION_17
+ targetCompatibility = JavaVersion.VERSION_17
}
}| # Enables namespacing of each library's R class so that its R class includes only the | ||
| # resources declared in the library itself and none from the library's dependencies, | ||
| # thereby reducing the size of the R class for that library | ||
| android.nonTransitiveRClass=true |
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.
This is not needed with AGP 8.0+ so we can drop this block.
| android.enableJetifier=true | ||
|
|
||
| # Kotlin code style for this project: "official" or "obsolete": | ||
| kotlin.code.style=official |
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.
I don't think this is necessary to explicitly write, I think official is the default.
|
|
||
| <application | ||
| android:allowBackup="false" | ||
| android:supportsRtl="true"> |
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.
Nit: we don't need to set these two attributes.
| android:label="AndroidInterop"> | ||
| <intent-filter> | ||
| <action android:name="android.intent.action.MAIN"/> | ||
| <category android:name="android.intent.category.LAUNCHER"/> |
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.
It looks like we don't actually need action.MAIN and category.LAUNCHER -- this is essentially a background service, not a regular app that has a launcher etc.
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.
We need it because you can't launch the application without this intent.
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.
How is it possible then that the tests pass without it?
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.
OK maybe you right the interop doesn't need it, but for debugging purposes it's very convenient to be able to launch the interop client in the debugger.
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.
Ah, you're using a graphical debugger? I guess it makes sense to keep it then, if it makes debugging easier. 👍
| @@ -0,0 +1,434 @@ | |||
| use std::{ | |||
| cell::{Cell, RefCell}, | |||
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.
Cell import should be guarded via #[cfg(feature = "proteus")] in order to silence the unused import warning.
| @@ -0,0 +1,434 @@ | |||
| use std::{ | |||
| cell::{Cell, RefCell}, | |||
| io::{BufRead, BufReader, Read}, | |||
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.
Traits should be imported ... as _, e.g. BufRead as _, ....
| match process.try_wait() { | ||
| Ok(None) => {} | ||
| Ok(Some(exit_status)) => { | ||
| if boot_device && exit_status.code() == Some(149) { |
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.
What does 149 mean here?
| } | ||
|
|
||
| fn boot_device(device: &str) -> std::io::Result<Output> { | ||
| Command::new("xcrun").args(["simctl", "boot", device]).output() |
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.
xcrun is only available with XCode, how is this supposed to work on other platforms?
| .execute(format!( | ||
| "--es action init-mls --es client_id {} --ei ciphersuite {}", | ||
| client_id_base64, ciphersuite | ||
| )) |
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.
Nit: this could also be written as
.execute(format!(
"--es action init-mls --es client_id {client_id_base64} --ei ciphersuite {ciphersuite}"
))| use base64::{Engine as _, engine::general_purpose}; | ||
| use core_crypto::{KeyPackageIn, Keypackage}; | ||
| use thiserror::Error; | ||
| use tls_codec::Deserialize; |
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.
Nit: traits should be imported .. as _.
| pid.as_str(), | ||
| "-v", | ||
| "raw", | ||
| "System.out:I *:S", |
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.
Nice, I didn't know one can get system logs so easily!
Nit: it would be nice to have a short comment explaining the meaning of
System.out:I *:S
| Failure { message: String }, | ||
| } | ||
|
|
||
| #[derive(Error, Debug)] |
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.
Nit: I'd prefer thiserror::Error here rather than just plain Error, which is one of std traits.


What's new in this PR
PR Submission Checklist for internal contributors
SQPIT-764feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.