Tell the dependent crates where the ggml CMake package is - #1091
Merged
Conversation
The CMake build installs a ggml package config, but no crate can find it. The build script now prints the path to that config. Cargo gives the path to each dependent crate in the DEP_LLAMA_GGML_CMAKE_DIR variable. A crate that also builds ggml can put this path in CMAKE_PREFIX_PATH. Then it uses this ggml, and the program contains only one ggml. Two copies of ggml in one program cause duplicate symbols and unsafe behavior. This is the same method as the backends_dir value above it. The change adds no feature and changes no build.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The CMake build already installs a ggml package config, but no crate can find
it, because the build script does not print the path. This change prints it:
Cargo gives the value to dependent crates in
DEP_LLAMA_GGML_CMAKE_DIR. Thisis the same method as the
backends_dirvalue above it. The build script looksin
lib64/cmake/ggmland thenlib/cmake/ggml, and prints nothing if it findsneither.
Why
A program that uses
llama-cpp-2andwhisper-rstogether contains two copiesof ggml, and they are not the same:
GGML_TYPE_COUNTis 42 in recent llama.cppand 40 in
whisper-rs-sys 0.15.0. Users must give the linker--allow-multiple-definitionor/FORCE:MULTIPLE. The linker then discards onecopy, and half of the program uses a 42-entry table through code that expects
40 entries. We saw this stop a program with
STATUS_HEAP_CORRUPTION.whisper-rshas an open PR forWHISPER_USE_SYSTEM_GGML(#260), but it needs the
path of an installed ggml. This change supplies that path.
Test
We made a program that starts a
LlamaBackendand whisper, and built it twotimes on Linux with
rust-lldand no linker flag.Without
whisper-rs/system-ggml:With
whisper-rs/system-ggmlandCMAKE_PREFIX_PATHset to the new path, itlinks and runs.
whisper-rs-sysbuilds 0 ggml archives instead of 6, andnm --defined-only <bin> | grep -cw ggml_initgives 1.Limits and risk
Cargo gives
DEP_LLAMA_GGML_CMAKE_DIRonly to direct dependents.whisper-rs-sysis not one today, so users must setCMAKE_PREFIX_PATH, as inthe test. A subsequent change to
whisper-rs-syscan add an optional dependencyon this crate and read the variable.
Risk is low: no new feature, no new dependency, no change to a build step.
cargo fmtandcargo clippyare clean.