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

KTOR-7289: Initial support for android native targets #4236

Merged
merged 6 commits into from
Oct 7, 2024

Conversation

whyoleg
Copy link
Contributor

@whyoleg whyoleg commented Aug 21, 2024

Fixes https://youtrack.jetbrains.com/issue/KTOR-7289/Support-androidNative-targets

TODO:

  • needs kotlinx-html release with android native targets for ktor-server-html-builder
  • ISO_8859_1 charset is not supported - only UTF-8, because iconv is available only from Android API 28 (need hacks in cinterop)
  • there is a lot of copy-paste from nix mainly, because:
    • commonizer doesn't work well in some cases and so it's not possible to use some declarations from common code (f.e in ktor-network
    • it's not possible with current auto-module configuration via TargetsConfig class to setup, that in some modules androidNative targets should be like nix, while in others it should be on it's own

Otherwise, it compiles and links :)
Probably it could be tested via an approach shown in Kotlin/kotlinx-datetime#344

To test, run an Android emulator, and then, in the command line, ./gradlew androidNativeArm64TestBinaries && adb push core/build/bin/androidNativeArm64/debugTest/test.kexe /data/local/tmp/ && adb shell /data/local/tmp/test.kexe

Not sure still about the quality of those changes...
Feel free to suggest how to make it better.

But after this, it will be much simpler to depend on ktor in libraries which have all Kotlin/Native targets

Next thing on my list of "small contributions" is to check if it's possible to improve ktor build to be compatible with Configuration Cache.

@e5l e5l self-requested a review August 26, 2024 08:05
@vbarthel-fr
Copy link

vbarthel-fr commented Sep 11, 2024

I'm not knowledgeable enough to give any constructive feedback on those changes, but since I successfully managed to start a ktor server from an android native binary thanks to this work (whyoleg/android-native), I'm writing a little "thanks a lot" message :)

I just had to modify the publication script so that I can publish androidNativeXXX ktor artifacts locally to make them accessible from my test project.

diff --git a/buildSrc/src/main/kotlin/Publication.kt b/buildSrc/src/main/kotlin/Publication.kt
index 1b62c68b0..6d02ae1f9 100644
--- a/buildSrc/src/main/kotlin/Publication.kt
+++ b/buildSrc/src/main/kotlin/Publication.kt
@@ -47,6 +47,15 @@ fun isAvailableForPublication(publication: Publication): Boolean {
         "macosArm64"
     )
 
+    val androidPublication = setOf(
+        "androidNativeArm32",
+        "androidNativeArm64",
+        "androidNativeX64",
+        "androidNativeX86"
+    )
+
+    result = result || name in androidPublication
+
     result = result || (HOST_NAME == "macos" && name in macPublications)
 
     return result
Screenshot 2024-09-11 at 21 44 24

Thanks again 🙏

@whyoleg whyoleg changed the title Initial support for android native targets KTOR-7289: Initial support for android native targets Sep 21, 2024
@whyoleg
Copy link
Contributor Author

whyoleg commented Sep 24, 2024

Note: if this PR will be merged, TC configuration for publishing should be updated to include new targets

@e5l e5l requested review from osipxd and removed request for e5l September 24, 2024 10:49
@e5l
Copy link
Member

e5l commented Sep 24, 2024

@osipxd, could you check when you have time?

Copy link
Member

@osipxd osipxd left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

I would try to reuse nix sources for androidNative targets. I've already tried to do it, but faced with the problem you described in the PR description when some posix functions used in SocketUtilsNix cannot be commonized.
I'm trying to fix this problem by splitting nix to nix32 and nix64. I'll let you know about results of this experiment.

*/

import org.gradle.api.*

fun Project.posixTargets(): List<String> = nixTargets() + windowsTargets()
fun Project.posixTargets(): List<String> = nixTargets() + windowsTargets() + androidNativeTargets()
Copy link
Member

Choose a reason for hiding this comment

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

Should android native be a part of nixTargets as it is Unix-like OS? If so, we could drop almost all duplicating code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory - it should be, but on practice because of the issues with commonizer there will be a lot of unresolved declarations in nix source-set :(

Copy link
Contributor Author

@whyoleg whyoleg Sep 26, 2024

Choose a reason for hiding this comment

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

Probably the hierarchy should be similar to atomicfu: https://github.com/Kotlin/kotlinx-atomicfu/blob/92ff33b4319970b8ab164c61c8ebfb6a69702955/atomicfu/build.gradle#L154-L172

So:

  • posix (= native)
    • windows (= mingw)
    • nix
      • unix
        • linux
        • apple
      • androidNative
        • androidNative32
        • androidNative64

Copy link
Member

@osipxd osipxd Sep 27, 2024

Choose a reason for hiding this comment

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

Here is the PR needed for easy hierarchy changes: #4349

@osipxd
Copy link
Member

osipxd commented Sep 30, 2024

@whyoleg, I've managed to get it work without splitting into nix and unix source-sets.
Be careful, I've force-pushed into your branch.

Copy link
Contributor Author

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

Nice!
I do like how better it is now!
Thanks!

P.S. it's a bit strange to review my own PR :)

fun Project.nixTargets(): List<String> = darwinTargets() + linuxTargets()
fun Project.nixTargets(): List<String> = darwinTargets() + linuxTargets() + androidNativeTargets()

fun Project.supportsAndroidNative(): Boolean = project.targetIsEnabled("androidNative")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Q: probably the same should be done for watchOS above? In future PR (can do after this is merged)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is a good idea, feel free to open a PR

buildSrc/src/main/kotlin/TargetsConfig.kt Outdated Show resolved Hide resolved
# Copyright 2014-2024 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
#

# Needs support in kotlinx-html: https://github.com/Kotlin/kotlinx.html/pull/288
Copy link
Contributor Author

@whyoleg whyoleg Sep 30, 2024

Choose a reason for hiding this comment

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

the PR is already merged and new versions need to be released
cc @e5l

@osipxd
Copy link
Member

osipxd commented Oct 1, 2024

Not everything is so smooth 🥲

Task ':ktor-utils:androidNativeArm32MetadataElements' uses this output of task ':ktor-utils:commonizeCInterop' without declaring an explicit or implicit dependency.

I can't reproduce this problem locally (because of gradle/gradle#27576 I suppose).

@whyoleg
Copy link
Contributor Author

whyoleg commented Oct 2, 2024

I can't reproduce this problem locally (because of gradle/gradle#27576 I suppose).

Same at my side, can't reproduce :(

Interesting, that the issue happens with the same target for which hierarchy API is wrong :)

@osipxd
Copy link
Member

osipxd commented Oct 3, 2024

target for which hierarchy API is wrong

Do you mean withAndroidNative() and withApple() declared in groups containing other groups?

The same task is still failing :(

@@ -7,7 +7,7 @@ plugins {
}

kotlin {
createCInterop("threadUtils", nixTargets()) {
createCInterop("threadUtils", nixTargets() - androidNativeTargets()) {
Copy link
Member

@osipxd osipxd Oct 3, 2024

Choose a reason for hiding this comment

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

The problem exists only for this module where I excluded android native targets from C interop. Maybe it is related

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could try to split this interop per related targets.
E.g create threadUtilsLinux and threadUtilsDarwin with distinct sets of targets (or even different def files) - may be there is some bug in commonizer logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @osipxd!
I was able to overcome the issue here: bbd3151
Previously failed build is now green :)

@whyoleg
Copy link
Contributor Author

whyoleg commented Oct 3, 2024

Do you mean withAndroidNative() and withApple() declared in groups containing other groups?

Nope, I'm talking about the issue with withAndroidNativeArm32 - the same target task is failing on CI androidNativeArm32MetadataElements - not sure if it's related though

Copy link
Member

@osipxd osipxd left a comment

Choose a reason for hiding this comment

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

Great job, thank you!
Created a task to update CI configuration: KTOR-7535

@osipxd osipxd merged commit a34c260 into ktorio:main Oct 7, 2024
8 of 13 checks passed
@whyoleg whyoleg deleted the android-native branch October 12, 2024 11:47
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.

4 participants