Skip to content
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

Merged
merged 15 commits into from
Jan 24, 2025
Merged

Conversation

ggerganov
Copy link
Owner

I think this should fix: #11091, #11321, #11267

@github-actions github-actions bot added the devops improvements to build systems and github actions label Jan 24, 2025
@@ -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/*
Copy link
Collaborator

@slaren slaren Jan 24, 2025

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?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that. Fixing.

Copy link
Collaborator

@slaren slaren Jan 24, 2025

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).

Copy link
Owner Author

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)

Copy link
Collaborator

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).

Copy link
Owner Author

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.

Copy link
Collaborator

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.

Copy link
Owner Author

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.

@github-actions github-actions bot added the build Compilation issues label Jan 24, 2025
@ggerganov ggerganov changed the title release : pack /lib and /include in the packages release : pack /lib in the packages Jan 24, 2025
@ggerganov
Copy link
Owner Author

Do we have a way to make release packages before merging to master, in order to inspect the contents produced by this branch?

@slaren
Copy link
Collaborator

slaren commented Jan 24, 2025

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.

@Satertek
Copy link

Satertek commented Jan 24, 2025

Seeing the same #11321 issues on mac with the latest artifacts (de764d5) on this PR:

dyld[17755]: Library not loaded: @rpath/libllama.dylib
  Referenced from: <D6F6B3A0-7588-3A0D-AB44-6185C7601DD8> /Users/brian/Downloads/build 2/bin/llama-server
  Reason: tried: '/Users/runner/work/llama.cpp/llama.cpp/build/bin/libllama.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/Users/runner/work/llama.cpp/llama.cpp/build/bin/libllama.dylib' (no such file), '/Users/runner/work/llama.cpp/llama.cpp/build/bin/libllama.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/Users/runner/work/llama.cpp/llama.cpp/build/bin/libllama.dylib' (no such file)
zsh: abort      ./llama-server

otool -l output includes:

Load command 24
          cmd LC_RPATH
      cmdsize 64
         path /Users/runner/work/llama.cpp/llama.cpp/build/bin (offset 12)

@ngxson
Copy link
Collaborator

ngxson commented Jan 24, 2025

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:

./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.32' not found (required by ./llama-quantize)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.38' not found (required by ./llama-quantize)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by ./llama-quantize)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.33' not found (required by ./llama-quantize)
./llama-quantize: /lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.32' not found (required by ./llama-quantize)
./llama-quantize: /lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.30' not found (required by ./llama-quantize)
./llama-quantize: /lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.29' not found (required by ./llama-quantize)
./llama-quantize: /lib/x86_64-linux-gnu/libstdc++.so.6: version `CXXABI_1.3.13' not found (required by ./llama-quantize)
./llama-quantize: /lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.29' not found (required by libllama.so)
./llama-quantize: /lib/x86_64-linux-gnu/libstdc++.so.6: version `CXXABI_1.3.13' not found (required by libllama.so)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.32' not found (required by libllama.so)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by libllama.so)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.38' not found (required by libllama.so)
./llama-quantize: /lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.29' not found (required by libggml-base.so)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.38' not found (required by libggml-base.so)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.32' not found (required by libggml.so)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by libggml.so)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.32' not found (required by libggml-cpu.so)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by libggml-cpu.so)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.33' not found (required by libggml-cpu.so)
./llama-quantize: /lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.29' not found (required by libggml-rpc.so)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.38' not found (required by libggml-rpc.so)
./llama-quantize: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.32' not found (required by libggml-rpc.so)

@ngxson
Copy link
Collaborator

ngxson commented Jan 24, 2025

Ok so the problem was that ubuntu-latest-cmake is run with ubuntu-latest while my VM is based on ubuntu 20.

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.

@ngxson
Copy link
Collaborator

ngxson commented Jan 24, 2025

The release artifact turns out to be a zip inside another zip, not sure why.

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.

@ggerganov
Copy link
Owner Author

@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 install target, that would probably do the RPATH modification that is necessary?

@ngxson
Copy link
Collaborator

ngxson commented Jan 24, 2025

For the GLIBC version incompatible, can we simply try downgrading version of ubuntu-latest-cmake? In build.yml:

  ubuntu-latest-cmake:
    runs-on: ubuntu-latest ---> change this to ubuntu-22.04

@slaren
Copy link
Collaborator

slaren commented Jan 24, 2025

For the RPATH issue, adding this option seems to work on linux at least: -DCMAKE_BUILD_RPATH_USE_ORIGIN=ON

@ggerganov
Copy link
Owner Author

@ngxson Ok. I am even thinking to go down to 20.04. We would cover even more users.

@ngxson
Copy link
Collaborator

ngxson commented Jan 24, 2025

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.

@slaren
Copy link
Collaborator

slaren commented Jan 24, 2025

Distributing binaries for linux is always an issue due to that. Building with GGML_STATIC (and adding a similar option for LLAMA_STATIC) so that the stdlib is linked statically might work, but there may be other issues. Ultimately considering how easy it is to build a CPU only version llama.cpp on linux, I am not sure that it is worth the trouble.

@ngxson
Copy link
Collaborator

ngxson commented Jan 24, 2025

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.

@ggerganov
Copy link
Owner Author

Running a new artifact-enabled build with a couple of attempts:

  • Go to ubuntu 20.04 to see if this will work
  • Add RPATH origin thing to the MacOS builds

I removed the 3rd MacOS build because I think it is redundant?

@bandoti
Copy link
Collaborator

bandoti commented Jan 24, 2025

@ngxson Ok. I am even thinking to go down to 20.04. We would cover even more users.

Word of warning they are deprecating the 20.04 images.

@ggerganov
Copy link
Owner Author

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.

@slaren
Copy link
Collaborator

slaren commented Jan 24, 2025

For linux maybe we can add some .sh file that people can download with curl and pipe to sh that downloads and builds llama.cpp automatically.

@bandoti
Copy link
Collaborator

bandoti commented Jan 24, 2025

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 😊

@Satertek
Copy link

Setting CMAKE_BUILD_RPATH and CMAKE_INSTALL_RPATH manually I am able to get it working:

if (APPLE)
    set(CMAKE_BUILD_RPATH "@loader_path")
    set(CMAKE_INSTALL_RPATH "@loader_path")
    message(STATUS "Setting RPATH to @loader_path")
elseif (LINUX)
    # untested, not sure if needed
    #set(CMAKE_BUILD_RPATH "$ORIGIN;")
    #set(CMAKE_INSTALL_RPATH "$ORIGIN")
    #message(STATUS "Setting RPATH to $ORIGIN")
else()
    # do nothing
endif()

This works but still has a lingering (unused) RPATH:

Load command 24
          cmd LC_RPATH
      cmdsize 32
         path @loader_path (offset 12)
Load command 25
          cmd LC_RPATH
      cmdsize 56
         path /Users/brian/git/llama.cpp/build/bin (offset 12)

-DCMAKE_BUILD_RPATH_USE_ORIGIN=ON did not appear to have any effect.

@slaren
Copy link
Collaborator

slaren commented Jan 24, 2025

set(CMAKE_INSTALL_RPATH "@loader_path")

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.

@ngxson
Copy link
Collaborator

ngxson commented Jan 24, 2025

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.

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 llama-quantize ?

I think we're now left with 2 options: either build on ubuntu 22.04 (instead of latest or 20), or link glibc statically

@bandoti
Copy link
Collaborator

bandoti commented Jan 24, 2025

I found an interesting example in the cmake docs to add a build step for copying the windows runtime dependencies:

find_package(foo CONFIG REQUIRED) # package generated by install(EXPORT)

add_executable(exe main.c)
target_link_libraries(exe PRIVATE foo::foo foo::bar)
add_custom_command(TARGET exe POST_BUILD
  COMMAND ${CMAKE_COMMAND} -E copy -t $<TARGET_FILE_DIR:exe> $<TARGET_RUNTIME_DLLS:exe>
  COMMAND_EXPAND_LISTS
)

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. 😅

@ngxson
Copy link
Collaborator

ngxson commented Jan 24, 2025

Just to note that, in the future, we can also try musl which is know to be static-linking-friendly. (I'll have a look later)

@ggerganov
Copy link
Owner Author

The macOS package is working now: https://github.com/ggerganov/llama.cpp/actions/runs/12951873896/artifacts/2481371815

@ngxson Just pushed 22.04 ubuntu build

@Satertek
Copy link

Latest release artifact (3df9e97) for macos arm64 runs for me.

I will note that the additional RPATH is still being appended (albeit ignored)

Load command 23
          cmd LC_RPATH
      cmdsize 32
         path @loader_path (offset 12)
Load command 24
          cmd LC_RPATH
      cmdsize 64
         path /Users/runner/work/llama.cpp/llama.cpp/build/bin (offset 12)

@ggerganov
Copy link
Owner Author

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

@ngxson
Copy link
Collaborator

ngxson commented Jan 24, 2025

Testing it now..

@slaren
Copy link
Collaborator

slaren commented Jan 24, 2025

Yes, it works.

Description:    Ubuntu 24.04.1 LTS

@ngxson
Copy link
Collaborator

ngxson commented Jan 24, 2025

Works on ubuntu 22.04 too

@t1u1
Copy link

t1u1 commented Jan 25, 2025

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?

nvidia-smi reports the GPU so the Cuda toolkit is installed properly.

@ggerganov
Copy link
Owner Author

Currently the Linux pre-build packages are CPU-only.

@t1u1
Copy link

t1u1 commented Jan 26, 2025

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 -ngl option being required. On MacOS the GPU is used automatically, and needs to be explicitly disabled. However on Linux+Cuda it seems to be opposite. Spent several hours trying to understand why the Nvidia GPU was not being used inspite of being detected by llama.cpp

anagri pushed a commit to BodhiSearch/llama.cpp that referenced this pull request Jan 26, 2025
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Compilation issues devops improvements to build systems and github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Again, the releases don't have the libraries.
6 participants