llama-cpp: Add new b4570 version #26477
Conversation
recipes/llama-cpp/all/conanfile.py
Outdated
| if is_apple_os(self): | ||
| self.cpp_info.components["common"].frameworks.extend(["Foundation", "Accelerate", "Metal"]) | ||
| elif self.settings.os in ("Linux", "FreeBSD"): | ||
| self.cpp_info.components["common"].system_libs.extend(["dl", "m", "pthread", "gomp"]) |
There was a problem hiding this comment.
I think this code is common for all versions?
recipes/llama-cpp/all/conanfile.py
Outdated
| self.cpp_info.components["common"].requires = ["llama"] | ||
| if self.options.with_curl: | ||
| self.cpp_info.components["common"].requires.append("libcurl::libcurl") | ||
| self.cpp_info.components["common"].defines.append("LLAMA_USE_CURL") |
There was a problem hiding this comment.
I think we don't need to define this in the package_info?
There was a problem hiding this comment.
Upstream's CMakeLists contains target_compile_definitions(${TARGET} PUBLIC LLAMA_USE_CURL) with TARGET being common
There was a problem hiding this comment.
And is this new from this version or should we add it to the old versions too?
recipes/llama-cpp/all/conanfile.py
Outdated
| if self.options.with_curl: | ||
| self.cpp_info.components["common"].requires.append("libcurl::libcurl") |
There was a problem hiding this comment.
shouldn't this be defined for both versions?
There was a problem hiding this comment.
No, because if no requires is explicitly defined, all components get all the library requirements. I can be more explicity to avoid overlinking in old versions
recipes/llama-cpp/all/conanfile.py
Outdated
| if Version(self.version) >= "b4570": | ||
| self.cpp_info.components["common"].includedirs = [os.path.join("include", "common")] | ||
| self.cpp_info.components["common"].libs = ["common"] | ||
| self.cpp_info.components["common"].requires = ["llama"] |
There was a problem hiding this comment.
Is this true? I'm not sure that llama requires common, this requires was not defined for previous versions
There was a problem hiding this comment.
target_link_libraries (${TARGET} PRIVATE ${LLAMA_COMMON_EXTRA_LIBS} PUBLIC llama Threads::Threads) with TARGET being common here too
There was a problem hiding this comment.
Seems like it was missing for the old versions too, as this same line is present in the previous version too
recipes/llama-cpp/all/conanfile.py
Outdated
| self.cpp_info.components["common"].includedirs = [os.path.join("include", "common")] | ||
| self.cpp_info.components["common"].libs = ["common"] |
There was a problem hiding this comment.
We can move this out from the if Version and use for all versions
recipes/llama-cpp/all/conanfile.py
Outdated
| if self.options.with_cuda and not self.options.shared: | ||
| self.cpp_info.builddirs.append(os.path.join("lib", "cmake")) | ||
| module_path = os.path.join("lib", "cmake", "llama-cpp-cuda-static.cmake") | ||
| self.cpp_info.set_property("cmake_build_modules", [module_path]) |
There was a problem hiding this comment.
this can also be shared between both versions
| "b4079": | ||
| - patch_file: "patches/b4079-001-curl-patch-targets.patch" | ||
| "b3542": | ||
| - patch_file: "patches/b3542-001-curl-patch-targets.patch" | ||
| "b3040": |
There was a problem hiding this comment.
This version had a slightly different logic, because did not have the ggml component so it the recipe was a bit cleaner removing this, and it's also quiet outdated
Summary
Changes to recipe: llama-cpp/b4570
Motivation
We wanted to use it in a future blogpost
Details
Lots of changes for this new version, so the package_info is split in two: the old one, and the new one. Maybe it would be better to remove the old versions from the conandata and just keep this new version, but thought that the current approach is a bit better until we can decide