-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: add kotlin for android client #556
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Fachry Maulana Muhsinin <[email protected]>
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.
looking great to me!
flipt-client-kotlin-android/src/androidTest/java/io/flipt/client/TestFliptEvaluationClient.kt
Outdated
Show resolved
Hide resolved
Signed-off-by: Fachry Maulana Muhsinin <[email protected]>
WithDirectory("/src", t.dir.Directory("flipt-client-kotlin-android"), dagger.ContainerWithDirectoryOpts{ | ||
Exclude: []string{"./.idea/", ".gradle/", "build/"}, | ||
}). | ||
WithFile(fmt.Sprintf("/src/main/cpp/libs/%s/libfliptengine.so", path), t.test.File(libFile)). |
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.
is src/main/cpp/libs
the standard path for android / kotlin to look for shared C libs? or is this configured somewhere else?
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.
sirce I'm using .a file , it is configured in the CMakeLists.txt. there's no standard for .a file, unlike .so file that must be stored in jniLibs directory
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 see are you saying this is just for the tests? just wondering why its at src/main/cpp/libs/%s/libfliptengine.so
if its a .a
file
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #556 +/- ##
==========================================
+ Coverage 85.48% 85.65% +0.17%
==========================================
Files 8 7 -1
Lines 3933 3905 -28
==========================================
- Hits 3362 3345 -17
+ Misses 571 560 -11 ☔ View full report in Codecov by Sentry. |
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.
look great! what do we have left to get this across the finish line? how can I help?
- Linux x86_64 | ||
- Linux x86_64 (musl) | ||
- Linux arm64 | ||
- Linux arm64 (musl) | ||
- MacOS x86_64 | ||
- MacOS arm64 | ||
- Windows x86_64 |
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.
- Linux x86_64 | |
- Linux x86_64 (musl) | |
- Linux arm64 | |
- Linux arm64 (musl) | |
- MacOS x86_64 | |
- MacOS arm64 | |
- Windows x86_64 | |
- Linux x86_64 | |
- Linux arm64 |
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.
Actually it needs Android x86_64 and android arm64
### Glibc vs Musl | ||
|
||
Most Linux distributions use [Glibc](https://en.wikipedia.org/wiki/Glibc), but some distributions like Alpine Linux use [Musl](https://en.wikipedia.org/wiki/Musl). If you are using Alpine Linux, you will need to install the `musl` version of the client. | ||
|
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.
### Glibc vs Musl | |
Most Linux distributions use [Glibc](https://en.wikipedia.org/wiki/Glibc), but some distributions like Alpine Linux use [Musl](https://en.wikipedia.org/wiki/Musl). If you are using Alpine Linux, you will need to install the `musl` version of the client. |
I dont think we are doing a MUSL 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.
agree on this.
return resp.result ?: throw EvaluationException(resp.errorMessage ?: "Unknown Error") | ||
} | ||
|
||
fun readFlags(ptr: String): Result<ArrayList<Flag>> { |
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 dont think we want this function exposed publicly. can we just inline it in listFlags
?
|
||
|
||
@Serializable | ||
data class InternalEvaluationRequest( |
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.
can we make this private?
WithDirectory("/src", t.dir.Directory("flipt-client-kotlin-android"), dagger.ContainerWithDirectoryOpts{ | ||
Exclude: []string{"./.idea/", ".gradle/", "build/"}, | ||
}). | ||
WithFile(fmt.Sprintf("/src/main/cpp/libs/%s/libfliptengine.so", path), t.test.File(libFile)). |
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 see are you saying this is just for the tests? just wondering why its at src/main/cpp/libs/%s/libfliptengine.so
if its a .a
file
add client SDK for Kotlin Android using FFI
Fixes: #264 #471
TODO