-
Notifications
You must be signed in to change notification settings - Fork 164
Add linuxArm64 target #1051
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 linuxArm64 target #1051
Conversation
|
It would be nice if this PR could be the first one to be reviewed. That helps with building compose-multiplatform-core, as building skiko locally for all platforms seems to be quite complicated. |
|
Rebased on master so the GitHub Ubuntu runner works again |
.github/workflows/ci.yml
Outdated
| export DISPLAY=:0 | ||
| ./gradlew --no-daemon --stacktrace --info -Pskiko.native.enabled=true -Pkotlin.native.cacheKind.linuxX64=none -Pskiko.test.onci=true :skiko:linuxX64Test | ||
|
|
||
| # Linux arm64 tests cannot be run but are tested for building |
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.
If use a linux-arm64 runner, can we run the tests then?
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 would be a bit complicated as Kotlin cannot be built on Linux arm64. So we would need to compile the tests on x64, then copy them over to arm64, and them run them somehow. This would require some investigation as I am not sure how to do this or if this is even possible.
As none of the official Kotlin libraries run tests on arm64, I don't think it is that important for skiko. The best we can do is just to check if compilation succeeds, which I guess should pick up most arm64 specific issues.
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.
Compose on arm64 device with this skiko PR built locally is working for me, https://kotlinlang.slack.com/archives/CJLTWPH7S/p1747853260514819?thread_ts=1743758275.030769&cid=CJLTWPH7S
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.
Oh, got it. thanks for explaining.
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.
run them somehow
Tests are a .kexe you can execute. Mosaic runs its tests on Linux ARM: https://github.com/JakeWharton/mosaic/blob/trunk/.github/workflows/build.yaml#L72-L183
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 a lot for sharing! Now tests are up and running for arm64. https://github.com/Thomas-Vos/skiko/actions/runs/15261348943
One test failed due to GL, which should be EGL instead on Linux arm64 (as far as I know). So I disabled this test on arm64 which can be re-enabled after EGL support is added.
PRs for adding EGL support: JetBrains/skia-pack#68, #1052
skiko/buildSrc/src/main/kotlin/tasks/configuration/NativeTasksConfiguration.kt
Outdated
Show resolved
Hide resolved
skiko/docker/linux-amd64/Dockerfile
Outdated
| apt-get install gcc-9 g++-9 -y && \ | ||
| update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-9 60 --slave /usr/bin/g++ g++ /usr/bin/g++-9 && \ | ||
| update-alternatives --config gcc | ||
| update-alternatives --config gcc \ |
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.
To build a snapshot of skiko on Teamcity from this PR, we would need the new docker image sooner than this PR is merged.
Would you like to make a separate PR with docker change?
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 can open a separate PR with only the Dockerfile changes.
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 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'll build a new docker image now.
But I see that our Teamcity build configuration will need to have an extra step added to build for linuxArm64 - it won't build and publish automatically.
The build configuration is not open sourced unfortunately. So I'll need to make that change. I'll try to do that by the end of this week.
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.
And to make the new CI step work we actually need your PR to merged before it runs. It means we can't have a snapshot build of skiko for your PR.
I'll test your changes locally on my linux machine tomorrow. And we'll merge it without waiting for Snapshot build.
| with: | ||
| name: test-binary-linuxArm64 | ||
| path: ./skiko/build/bin/linuxArm64 | ||
| - shell: bash |
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 don't need to specify the shell here. I do it on Mosaic because my job is a matrix that also runs on Windows where the default is power shell.
…iling linuxArm64 target (#1061) #1051 (comment)
| OS.Linux -> when (arch) { | ||
| Arch.X64 -> "g++" | ||
| Arch.Arm64 -> "clang++" | ||
| Arch.Arm64 -> if (hostArch == Arch.Arm64) "g++" else "aarch64-linux-gnu-g++" |
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.
A quick update.
Tried to build a snapshot on CI. Two tasks failed:
- jvm build for linux arm64 - complains about missing
g++. It's using its own docker image, so probably that's why. But I'm recalling some discussions within the team, that there were some performance benefits when using clang. So we might need to stick to clang at least for jvm builds. We can try make it configurable. So for k/native CI would choose g++. - k/native linux arm 64 build failed too due to artifact signing issue. I'll figure it out next week.
Btw, would it complicate the build a lot if we try to stick to clang++ for all cases? Or it configuration would be better? (So for jvm linux arm64 we'll use clang)
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.
@eymar It should be possible to cross compile with clang++ as well. If it increases performance I am all for it. I only selected g++ because linux x64 used it, and didn't know this change would affect the JVM.
However, I would suggest to do this in another PR in the future. I think it would make more sense to use clang++ for all Linux targets (also x64), it would also help simplify the build setup.
For now, I updated the PR to fix the jvm arm build by making it use clang++ again. Also added the jvm arm tests to GitHub actions, as that would have helped spot this issue earlier.
Let me know what you think about this.
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.
nit: can isJvm have a default value false? It's okay to keep it as is for now and to change it in a different PR (e.g. about clang++ for all linux targets).
An update from me: I tried to debug the issue with the artifacts signature on CI, but I didn't find a cause. So I'll debug it locally on my linux machine later this week.
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.
Built it from my branch. It's in our dev maven now - https://maven.pkg.jetbrains.space/public/p/compose/dev/org/jetbrains/skiko/skiko-linuxarm64/
Merging your PR now. Thank you for contributing :)
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, thank you for reviewing!
This PR adds support for the linuxArm64 target. It can be cross compiled from a Linux x64 host.
The
multistrappackage is used for downloading some required arm64 dependencies.Testing
GitHub run: https://github.com/Thomas-Vos/skiko/actions/runs/15261348943
Also using it in a compose app.
I cannot test TeamCity, could someone please trigger a build and share the results?