Repository navigation
Conversation
madsmtm
left a comment
madsmtm
left a comment
There was a problem hiding this comment.
Is there any way this could be fixed in bindgen instead? It seems like a problem that every Android + bindgen user would have?
As far as I understand bindgen doesn't have any Android specific logic and doesn't know the API level. The fix at the level of cargo-ndk is great because it solves the problem for all the crates using bindgen. I'm using tauri, I guess cargo-mobile2 should be fixed too. Still llama-cpp-rs already sets all the flags and the build knows the API level, it makes sense to also set this. |
Honestly, with the speed of changes these days... the PR to fix the issue in cargo-ndk was opened by Copilot. |
|
It's clearly wrong currently and confusing. I think it would worth to do something, so might as well merge this to fix it. |
This branch has not been deployed
2 participants
Recent NDK (seems r30) requires API level passed in triples to clang:
ndk/30.0.16248370/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/sys/cdefs.h:365:2: error: Unversioned target triples are not supported!Although a previous PR (#796) removed passing
--targetto clang, I think that was a mistake to remove for Android as bindgen doesn't pass the API level. Some architecture triples can differ between rust and clang, but this is not the case for any currently supported Android target.cargo-ndk sets BINDGEN_EXTRA_CLANG_ARGS, I'm not sure if the removed code ever worked, but now it's surely broken because it's missing the API level. cargo-ndk is also fixing the issue (bbqsrc/cargo-ndk#216), but I would argue that it makes sense to fix it in llama-cpp-rs, since it already has logic to detect NDK version, passes it's own
--sysrootand-isystem. Having these it makes sense to also pass the correct versioned target triple. This fix should make it possible to build llama-cpp-rs with older cargo-ndk and the current main (unreleased). It's also helps builds where cargo-ndk is not used.