[native_toolchain_c] Setup Android RISCV64 toolchain. #165
Conversation
|
The failing tests are about Target.androidRiscv64 not being defined, but it is defined in this PR. Possibly the tests for native_toolchain_c are not using the version of native_assets_cli in this repository? |
They don't have path dependencies on each other, they have dependencies via a published version on pub on each other. |
So this PR needs to be split into two PRs. |
| @@ -38,6 +38,7 @@ class Architecture { | |||
| Abi.androidArm64: Architecture.arm64, | |||
| Abi.androidIA32: Architecture.ia32, | |||
| Abi.androidX64: Architecture.x64, | |||
| Abi.androidRiscv64: Architecture.riscv64, | |||
There was a problem hiding this comment.
This file needs to be it's own PR + a version bump to 0.3.1 so that it can be published before the native_toolchain_c PR.
|
Hm, actually, maybe this is premature because it would require a canary NDK to test and I'm not sure how the GitHub actions testing gets its NDK. |
native/.github/workflows/dart.yaml Lines 47 to 49 in 18bff8c |
github-actions
bot
added
type-infra
rmacnak-google
changed the title
.github/workflows/dart.yaml
Outdated
| @@ -46,7 +46,7 @@ jobs: | |||
|
|
|||
| - uses: nttld/setup-ndk@dbacc5871a0fac6eef9a09d2ca86bc8bf79432c3 | |||
| with: | |||
| ndk-version: r25b | |||
| ndk-version: r27 | |||
There was a problem hiding this comment.
Didn't work / not available from the source. Only increasing to r26b now.
I'm surprised the tests pass now. Are they only looking for the toolchain binaries but not invoking them?
There was a problem hiding this comment.
This should be invoking the tools you could double check the dylibs locally to see that it's build for the right arch.
There was a problem hiding this comment.
Oh, it uses -nostartfiles and links with nothing, so it's okay will all the standard libraries not existing yet :)
There was a problem hiding this comment.
Please file an issue that we should add tests for that on this repo.
github-actions
bot
added
type-infra
Addresses: #165 (comment) This is in line with what CMake does, so it's probably a more reasonable thing to do than `-nostartfiles`. The documentation says the sysroot should be auto-discovered on NDKs newer than 22, but it doesn't seem to break the newer versions, so maybe lets just keep the logic consistent for various NDK versions. We probably need a workaround for #165 temporarily.
github-actions
bot
removed
the
type-infra
Addresses: #165 (comment) This is in line with what CMake does, so it's probably a more reasonable thing to do than `-nostartfiles`. The documentation says the sysroot should be auto-discovered on NDKs newer than 22, but it doesn't seem to break the newer versions, so maybe lets just keep the logic consistent for various NDK versions. We probably need a workaround for #165 temporarily.
| @@ -17,20 +17,24 @@ void main() { | |||
| Target.androidArm64, | |||
| Target.androidIA32, | |||
| Target.androidX64, | |||
| // TODO(rmacnak): Enable when stable NDK 27 is available. | |||
| // Target.androidRiscv64, | |||
There was a problem hiding this comment.
@rmacnak-google Can this PR be landed or are we blocked on having NDK 27?
It looks like that will be only in the new year: https://github.com/android/ndk/wiki#ndk-r27-lts
There was a problem hiding this comment.
We can land this if you're okay with leaving this test disabled, otherwise we need to wait for NDK 27.
There was a problem hiding this comment.
Okay lets land it then 👍 The changes eyeballed look good.
No description provided.