-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
release : pack /lib in the packages #11392
Conversation
.github/workflows/build.yml
Outdated
@@ -88,7 +88,7 @@ jobs: | |||
run: | | |||
cp LICENSE ./build/bin/ | |||
cp examples/run/linenoise.cpp/LICENSE ./build/bin/LICENSE.linenoise.cpp | |||
zip -r llama-${{ steps.tag.outputs.name }}-bin-macos-arm64.zip ./build/bin/* | |||
zip -r llama-${{ steps.tag.outputs.name }}-bin-macos-arm64.zip ./build/bin/* ./build/lib/* ./build/include/* |
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 build
directory is the the cmake build directory, but I don't think there is a cmake install
or similar run at any moment, so I am not sure that it will contain lib
and include
directories. Am I missing something?
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 missed that. Fixing.
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 possible to make cmake
output the libraries to the bin
directory by adding set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
to the top level CMakeLists.txt
. That should make easier copying the libraries, since they all over the place in some systems (in Windows, cmake
outputs the dlls to the bin
directory already).
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.
This does not seem to make a difference on Mac - the libs still go in /lib
:
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 42caed48..7e41a44d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -16,6 +16,7 @@ endif()
list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/")
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
+set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
set(LLAMA_STANDALONE ON)
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.
Does it work with CMAKE_ARCHIVE_OUTPUT_DIRECTORY
instead? I guess it's another OS difference, in Windows shared libraries are output to the RUNTIME directory instead (which is why they end in bin
by default).
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 - still end up in /lib
.
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.
For me CMAKE_LIBRARY_OUTPUT_DIRECTORY
works as expected, the .dylib
files end in build/bin
, which is the intention. Without it they are in each project directory.
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 see. I was expecting the install
target to put them in install-prefix/bin
. Got it.
Do we have a way to make release packages before merging to |
If you modify the pack and upload artifacts steps so that they are run on PRs, I believe the artifact files will be accessible from the details page. |
Seeing the same #11321 issues on mac with the latest artifacts (de764d5) on this PR:
|
The release artifact turns out to be a zip inside another zip, not sure why. I'm testing this on a VM but running into another issue, not related but will have a look later:
|
This reverts commit 4decf2c.
Ok so the problem was that Should we downgrade it to ubuntu 22 instead? (Or make a new job for it?). I imagine that build compatible with ubuntu 22 will be useful for many users. In my case, ubuntu 20 is quite too old, but upgrading to 22 make more sense to me. |
Ok sorry it's a normal behavior, as I'm downloading the artifact from a specific job (one job can have multiple artifacts), not from the release page. |
@ngxson I also tried now on Ubuntu 22.04 and the GLIBC version is incompatible: $ ▶ ./llama-cli
./llama-cli: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.38' not found (required by ./llama-cli)
./llama-cli: /lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.32' not found (required by ./llama-cli)
./llama-cli: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.38' not found (required by libllama.so)
./llama-cli: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.38' not found (required by libggml-base.so)
./llama-cli: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.38' not found (required by libggml-rpc.so)
$ ▶ getconf GNU_LIBC_VERSION
glibc 2.35 @Satertek Do you know how to fix this? I think we need to do some configuration of the RPATH in the CMake files, but not really sure. @slaren Maybe this requires to actually pack the artifacts from an |
For the GLIBC version incompatible, can we simply try downgrading version of
|
For the RPATH issue, adding this option seems to work on linux at least: |
@ngxson Ok. I am even thinking to go down to 20.04. We would cover even more users. |
Yes ubuntu 20.04 is even better. I'm a bit worry if it may affect the build process, but let's downgrade and see if it has any problem. |
Distributing binaries for linux is always an issue due to that. Building with |
From my experience in my past job, I had to build apps that are compat with ubuntu 18. It's ok with stdc++ linked statically, but glibc linked statically produces some weird segfault sometime, so I would stay away from it. |
Running a new artifact-enabled build with a couple of attempts:
I removed the 3rd MacOS build because I think it is redundant? |
Word of warning they are deprecating the 20.04 images. |
Ok, it fails anyway with 20.04 - not worth it. Just wondering if we remove this build from the release package all together? Feel like it would just cause us headaches without real benefit. |
For linux maybe we can add some .sh file that people can download with |
Something not specifically related to this issue, but relevant to the releases. It might make sense to move the write permissions into the job level since only the "Create release" and "Upload release" jobs need access to the secrets. I am wondering if this has an effect on forks being able to run CI jobs locally in their forks. Here's the relevant note about the forked repositories. By having it in the top level, I don't think that forks can run CI jobs. This could make it easier to test CI changes before creating a pull request. I can create a new issue for this if you'd prefer, but it seemed small enough to squeeze it in here so figured I would try 😊 |
Setting
This works but still has a lingering (unused) RPATH:
|
Do not change the install RPATH though, the libs should still be installed in the standard OS directories when installing llama.cpp. The current releases are very janky, at some point we will need to make a proper macOS package. |
We did remove it in the past for a while, but had to add back because some people still rely on it. Maybe for one-time usage of tools like I think we're now left with 2 options: either build on ubuntu 22.04 (instead of latest or 20), or link glibc statically |
I found an interesting example in the cmake docs to add a build step for copying the windows runtime dependencies:
I think as @slaren mentioned there's perhaps more complexity than this immediate PR! For example, I might use macports or homebrew to build this with a Gnu toolchain on OSX, or XCode; I might use MSys2 on Windows, or Visual Studio, or another MinGW, and so forth. 😅 |
Just to note that, in the future, we can also try |
The macOS package is working now: https://github.com/ggerganov/llama.cpp/actions/runs/12951873896/artifacts/2481371815 @ngxson Just pushed 22.04 ubuntu build |
Latest release artifact (3df9e97) for macos arm64 runs for me. I will note that the additional RPATH is still being appended (albeit ignored)
|
This reverts commit 537b09e.
If someone can confirm that this works on Ubuntu 24.04, I think we are good to merge: https://github.com/ggerganov/llama.cpp/actions/runs/12952200870/artifacts/2481428812 |
Testing it now.. |
Yes, it works.
|
Works on ubuntu 22.04 too |
Thanks for fixing the library packaging. Works for me on Ubuntu 22.04 and 24.04 on CPU. But on a system with Nvidia Cuda, the GPU is not being detected by llama.cpp. Are these pre-built binaries supposed to support Cuda out of the box? Or do we need to compile ourselves?
|
Currently the Linux pre-build packages are CPU-only. |
Got it; thanks! I was able to compile and run on GPU successfully. Side note: One stumbling block was that I was not aware of |
* release : pack /lib and /include in the packages * cmake : put libs in /bin * TMP : push artifacts * Revert "TMP : push artifacts" This reverts commit 4decf2c. * ci : fix HIP cmake compiler options to be on first line * ci : restore the original HIP commands * ci : change ubuntu build from latest to 20.04 * ci : try to fix macos build rpaths * ci : remove obsolete MacOS build * TMP : push artifacts * ci : change back to ubuntu latest * ci : macos set build rpath to "@loader_path" * ci : fix typo * ci : change ubuntu package to 22.04 * Revert "TMP : push artifacts" This reverts commit 537b09e.
I think this should fix: #11091, #11321, #11267