-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
move ndk code to a new library #6951
Conversation
@@ -37,7 +37,7 @@ FetchContent_MakeAvailable(llama) | |||
# used in the AndroidManifest.xml file. | |||
add_library(${CMAKE_PROJECT_NAME} SHARED | |||
# List C/C++ source files with relative paths to this CMakeLists.txt. | |||
llama-android.cpp) | |||
llama-android.cpp) |
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.
was this tab here intentional?
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.
no, androidstudio must have done it
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.
Ah fair enough. If it looks better with the space in your opinion then that's fine.
CI is throwing up some error so you got some opportunities to fix that nitpick if you feel like it.
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.
The CI is likely failing because the Android project pull llama.cpp
from the master
branch:
llama.cpp/examples/llama.android/app/src/main/cpp/CMakeLists.txt
Lines 9 to 21 in e0f5561
# Declares the project name. The project name can be accessed via ${ PROJECT_NAME}, | |
# Since this is the top level CMakeLists.txt, the project name is also accessible | |
# with ${CMAKE_PROJECT_NAME} (both CMake variables are in-sync within the top level | |
# build script scope). | |
project("llama-android") | |
include(FetchContent) | |
FetchContent_Declare( | |
llama | |
GIT_REPOSITORY https://github.com/ggerganov/llama.cpp | |
GIT_TAG master | |
) | |
So it is mixing changes from this PR and what is currently on master
. Not familiar enough with Android build system to figure out how to fix this
Let's merge and see if it works
double checking to see if this changes touches any non android stuff. Looks safe. So merging now and fingers crossed it's as gg said that ci will pass in master for the android build |
@eltonkola CI failed for your android code in main branch, so can you see if you can make a correction for it as a separate PR?
|
Will wait a day or two if we can figure this out, else will revert this PR to avoid polluting the CI output of the main branch. |
) This reverts commit efc8f76.
move ndk code to a new module, so it can be published later on as an android library and easily integrated ion third party apps