Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

cmake : enable curl by default #12761

Merged
merged 25 commits into from
Apr 7, 2025
Merged

Conversation

ngxson
Copy link
Collaborator

@ngxson ngxson commented Apr 4, 2025

When curl was first added to the project, we decided to leave it disabled by default because there was no guarantee that the target system has libcurl installed.

But lot of things changed since then, curl now become part of the overall UX of llama.cpp examples. Most of the pre-built version (including own docker images and release binary) have it enabled, so I think it's now a good time to turn it on by default.

@ngxson ngxson requested a review from ggerganov April 4, 2025 21:56
@github-actions github-actions bot added build Compilation issues examples devops improvements to build systems and github actions server labels Apr 4, 2025
@ngxson
Copy link
Collaborator Author

ngxson commented Apr 4, 2025

I have no idea how to make the build-linux-cross to work, so I'll disable curl for those (unless someone knows how to fix it)

For windows build, I'll have a look to see how we can setup libcurl without copying the same code everywhere.

@github-actions github-actions bot added testing Everything test related SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language labels Apr 5, 2025
Comment on lines +4 to +12
# TODO: avoid copying this code block from common/CMakeLists.txt
set(LLAMA_RUN_EXTRA_LIBS "")
if (LLAMA_CURL)
find_package(CURL REQUIRED)
target_compile_definitions(${TARGET} PUBLIC LLAMA_USE_CURL)
include_directories(${CURL_INCLUDE_DIRS})
find_library(CURL_LIBRARY curl REQUIRED)
set(LLAMA_RUN_EXTRA_LIBS ${LLAMA_RUN_EXTRA_LIBS} ${CURL_LIBRARY})
endif ()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ericcurtin FYI, seems like llama-run was not linked against curl on windows, which will result in compilation error (cannot find curl/curl.h header). I made this quick fix which should work for now.

By default, only libcommon is linked against curl.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @ngxson FWIW we are moving away from using llama-run in RamaLama, I wrote a kinda client server thing that automatically terminates both processes on completion instead (the experience is similar to llama-run but better). I was starting to dislike the duplication of efforts in llama-run and llama-server. So I want to try and consolidate on llama-server.

It's a neat tool to test llama-server with all kinds of models, I'm almost ready to push the full version of it to main in RamaLama.

I'll still be happy to look at PRs around llama-run, etc. but it probably won't be actively used in RamaLama anymore at least.

@ngxson
Copy link
Collaborator Author

ngxson commented Apr 5, 2025

@ggerganov This should be ready for review. Btw, I think it will be more convenient to statically link libcurl on windows as curl is not installed at system level. I can give a try in a follow-up PR.

CMakeLists.txt Outdated
@@ -168,6 +168,10 @@ add_subdirectory(src)
# utils, programs, examples and tests
#

if (NOT LLAMA_BUILD_EXAMPLES OR NOT LLAMA_BUILD_COMMON)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (NOT LLAMA_BUILD_EXAMPLES OR NOT LLAMA_BUILD_COMMON)
if (NOT LLAMA_BUILD_EXAMPLES AND NOT LLAMA_BUILD_COMMON)

Also add a CMake log message when this branch is executed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On second thought, I think this condition should be just if NOT LLAMA_BUILD_COMMON, because we cannot build (most of the) examples without libcommon

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 533e0dc

@ggerganov ggerganov requested a review from slaren April 6, 2025 08:51
@ggerganov
Copy link
Member

Would the Windows binaries in the releases now have curl support after this change?

@@ -19,12 +19,15 @@ jobs:
sudo apt-get install -y --no-install-recommends \
build-essential \
gcc-14-riscv64-linux-gnu \
g++-14-riscv64-linux-gnu
g++-14-riscv64-linux-gnu \
libcurl4-openssl-dev
Copy link
Member

Choose a reason for hiding this comment

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

Does this work?

Suggested change
libcurl4-openssl-dev
libcurl4-openssl-dev:riscv64

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ngxson
Copy link
Collaborator Author

ngxson commented Apr 6, 2025

Would the Windows binaries in the releases now have curl support after this change?

Yeah that's a good question, I thought that we always copy the libcurl-x64.dll to the release artifact, but turns out we don't (we only use libcurl-x64.dll on server CI). For now, I'll add this to the windows release.

@slaren
Copy link
Member

slaren commented Apr 6, 2025

Redistributing curl may also require including the copyright notice.

@ngxson
Copy link
Collaborator Author

ngxson commented Apr 6, 2025

Yeah I think we're also missing some licenses for 3rd-party deps like httplib or json.hpp (also license for linenoise doesn't seem to be included in windows build, it's only included on linux builds)

I think what would be nicer is to package these licenses directly into the binary, and allow them to be shown via a CLI switch like --license

For now, to keep this PR simple, I'll simply create the license file on windows build via cmakefile

@github-actions github-actions bot added the android Issues specific to Android label Apr 6, 2025
@ngxson
Copy link
Collaborator Author

ngxson commented Apr 6, 2025

@slaren I added a script in cmakelists to copy all licenses into build/bin if it's running on github actions: 02257b7

Now I have another problem, ./build-xcframework.sh doesn't seem to respect the cmake config. Unfortunately I don't know who to work with xcode. @ggerganov Do you have any idea how to fix this?

@ggerganov
Copy link
Member

Now I have another problem, ./build-xcframework.sh doesn't seem to respect the cmake config.

This works on my end:

diff --git a/build-xcframework.sh b/build-xcframework.sh
index 2ce3939c4..1b9091d28 100755
--- a/build-xcframework.sh
+++ b/build-xcframework.sh
@@ -399,6 +399,7 @@ cmake -B build-ios-sim -G Xcode \
     -DCMAKE_XCODE_ATTRIBUTE_SUPPORTED_PLATFORMS=iphonesimulator \
     -DCMAKE_C_FLAGS="${COMMON_C_FLAGS}" \
     -DCMAKE_CXX_FLAGS="${COMMON_CXX_FLAGS}" \
+    -DLLAMA_CURL=OFF \
     -S .
 cmake --build build-ios-sim --config Release -- -quiet
 
@@ -411,6 +412,7 @@ cmake -B build-ios-device -G Xcode \
     -DCMAKE_XCODE_ATTRIBUTE_SUPPORTED_PLATFORMS=iphoneos \
     -DCMAKE_C_FLAGS="${COMMON_C_FLAGS}" \
     -DCMAKE_CXX_FLAGS="${COMMON_CXX_FLAGS}" \
+    -DLLAMA_CURL=OFF \
     -S .
 cmake --build build-ios-device --config Release -- -quiet
 
@@ -421,6 +423,7 @@ cmake -B build-macos -G Xcode \
     -DCMAKE_OSX_ARCHITECTURES="arm64;x86_64" \
     -DCMAKE_C_FLAGS="${COMMON_C_FLAGS}" \
     -DCMAKE_CXX_FLAGS="${COMMON_CXX_FLAGS}" \
+    -DLLAMA_CURL=OFF \
     -S .
 cmake --build build-macos --config Release -- -quiet
 
@@ -434,6 +437,7 @@ cmake -B build-visionos -G Xcode \
     -DCMAKE_XCODE_ATTRIBUTE_SUPPORTED_PLATFORMS=xros \
     -DCMAKE_C_FLAGS="-D_XOPEN_SOURCE=700 ${COMMON_C_FLAGS}" \
     -DCMAKE_CXX_FLAGS="-D_XOPEN_SOURCE=700 ${COMMON_CXX_FLAGS}" \
+    -DLLAMA_CURL=OFF \
     -S .
 cmake --build build-visionos --config Release -- -quiet
 
@@ -447,6 +451,7 @@ cmake -B build-visionos-sim -G Xcode \
     -DCMAKE_XCODE_ATTRIBUTE_SUPPORTED_PLATFORMS=xrsimulator \
     -DCMAKE_C_FLAGS="-D_XOPEN_SOURCE=700 ${COMMON_C_FLAGS}" \
     -DCMAKE_CXX_FLAGS="-D_XOPEN_SOURCE=700 ${COMMON_CXX_FLAGS}" \
+    -DLLAMA_CURL=OFF \
     -S .
 cmake --build build-visionos-sim --config Release -- -quiet
 
@@ -462,6 +467,7 @@ cmake -B build-tvos-sim -G Xcode \
     -DCMAKE_XCODE_ATTRIBUTE_SUPPORTED_PLATFORMS=appletvsimulator \
     -DCMAKE_C_FLAGS="${COMMON_C_FLAGS}" \
     -DCMAKE_CXX_FLAGS="${COMMON_CXX_FLAGS}" \
+    -DLLAMA_CURL=OFF \
     -S .
 cmake --build build-tvos-sim --config Release -- -quiet
 
@@ -476,6 +482,7 @@ cmake -B build-tvos-device -G Xcode \
     -DCMAKE_XCODE_ATTRIBUTE_SUPPORTED_PLATFORMS=appletvos \
     -DCMAKE_C_FLAGS="${COMMON_C_FLAGS}" \
     -DCMAKE_CXX_FLAGS="${COMMON_CXX_FLAGS}" \
+    -DLLAMA_CURL=OFF \
     -S .
 cmake --build build-tvos-device --config Release -- -quiet

@ngxson ngxson requested a review from slaren April 7, 2025 08:57
@ngxson
Copy link
Collaborator Author

ngxson commented Apr 7, 2025

Oh ok thanks, no idea why earlier I couldn't find the correct thing in build-xcframework.sh to be fixed, I think I opened the wrong file 🥲

Comment on lines +251 to +265
#
# copy the license files
#

# Check if running in GitHub Actions
if(DEFINED ENV{GITHUB_ACTIONS} AND "$ENV{GITHUB_ACTIONS}" STREQUAL "true")
message(STATUS "Running inside GitHub Actions - copying license files")

# Copy all files from licenses/ to build/bin/
file(GLOB LICENSE_FILES "${CMAKE_SOURCE_DIR}/licenses/*")
foreach(LICENSE_FILE ${LICENSE_FILES})
get_filename_component(FILENAME ${LICENSE_FILE} NAME)
configure_file(${LICENSE_FILE} "${CMAKE_BINARY_DIR}/bin/${FILENAME}" COPYONLY)
endforeach()
endif()
Copy link
Member

Choose a reason for hiding this comment

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

This is a hack, but I guess it is ok for now. We could look into using CPack to generate the packages, then these files would be added with install and would be automatically included in all packages.

@ngxson ngxson merged commit bd3f59f into ggml-org:master Apr 7, 2025
51 checks passed
@ngxson
Copy link
Collaborator Author

ngxson commented Apr 7, 2025

I have no idea why merging this also affect opening PRs. But if you see compilation error regarding Could NOT find CURL, simply rebase or merge master into your local branch and it will fix the problem.

felladrin added a commit to felladrin/MiniSearch that referenced this pull request Apr 7, 2025
tastelikefeet added a commit to tastelikefeet/llama.cpp that referenced this pull request Apr 10, 2025
* master: (123 commits)
  cuda : add f32 to bf16 copy op (ggml-org#12806)
  llava: improve clip_ctx destructor to not memleak load_image_size (ggml-org#12834)
  llama : fix FA when KV cache is not used (i.e. embeddings) (ggml-org#12825)
  server : fix thread.join() on exit (ggml-org#12831)
  llava: add more helper functions to check projector types in clip context (ggml-org#12824)
  arg : Including limits file on AIX (ggml-org#12822)
  server : webui : Improve Chat Input with Auto-Sizing Textarea (ggml-org#12785)
  Revert "sycl:remove redundant memcopy in function ggml_backend_sycl_buffer_set_tensor" (ggml-org#12812)
  gguf-py : support lazy tensor splitting (ggml-org#12809)
  llama : Support llama 4 text-only (ggml-org#12791)
  opencl: better identify Adreno GPU (ggml-org#12760)
  hellaswag: display estimated score confidence interval (ggml-org#12797)
  cuda : fix HIP and MUSA BF16 (#0)
  sync : ggml
  ggml : simplify Arm fp16 CPU logic (ggml/1177)
  CUDA: don't convert BF16 weights to FP32 (ggml/1174)
  cpu: move all the operators into a separate c++ file (except mul_mat) (ggml/1167)
  sycl: remove redundant memcopy in function ggml_backend_sycl_buffer_set_tensor (ggml-org#12734)
  ci : no curl on ggml-ci (ggml-org#12796)
  cmake : enable curl by default (ggml-org#12761)
  ...

# Conflicts:
#	common/arg.cpp
#	common/common.cpp
#	common/common.h
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
android Issues specific to Android build Compilation issues devops improvements to build systems and github actions examples server SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language testing Everything test related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants