Skip to content

Commit 1d875b1

Browse files
committed
build: add patches/ mechanism + fix Windows JNI arg parse (llama.cpp #24779)
Introduce a generic source-patch mechanism for the FetchContent'd llama.cpp tree so fixes apply to every C++ build (all CI jobs + local) from one place: - patches/ : drop *.patch / *.diff here (applied in filename order) - cmake/apply-llama-patches.cmake : cross-platform (cmake -P), idempotent (git apply --reverse --check skips already-applied), fail-loud (a stale patch aborts configure so it can't be silently dropped from a release build) - CMakeLists.txt : wired as the llama.cpp FetchContent PATCH_COMMAND First patch, 0001-win32-arg-parse-embed-guard.patch, fixes the b9739 Windows JNI regression from upstream #24779: common_params_parse unconditionally replaced the caller's argv with the process command line (GetCommandLineW), so an embedded java.exe lost its --model args -> "Failed to parse model parameters". The guard adopts the process command line only when the re-derived arg count equals argc: true for the standalone llama-* tools (UTF-8 CLI fix preserved), false for a JVM host (our already-UTF-8 argv kept). Verified the patch applies cleanly to b9739 and the applier is idempotent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfvSZ76NW4e1qX1PjL4RKq
1 parent cf6d2a3 commit 1d875b1

5 files changed

Lines changed: 132 additions & 3 deletions

File tree

CLAUDE.md

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -331,9 +331,35 @@ siblings; why (and why the `DEPOT_TOKEN` org secret and the README "Build cache
331331
are kept jllama-only) is explained in the cross-repo status under "Deliberate non-parity":
332332
[`../workspace/crossrepostatus.md`](../workspace/crossrepostatus.md).
333333

334+
## Local llama.cpp source patches (`patches/`)
335+
336+
The fetched llama.cpp source is patched before it compiles, via a generic mechanism:
337+
338+
- **`patches/`** (repo root) — drop any number of `*.patch` / `*.diff` files here. They are applied
339+
in **filename order** (use a numeric prefix, e.g. `0001-`, `0002-`), so keep them independent or
340+
ordered. Each must be a `git apply`-compatible unified diff with paths relative to the llama.cpp
341+
source root (`a/common/arg.cpp` / `b/common/arg.cpp`, i.e. `-p1`).
342+
- **`cmake/apply-llama-patches.cmake`** — the applier. Cross-platform (`cmake -P`, so identical on
343+
Linux/macOS/Windows), **idempotent** (`git apply --reverse --check` skips already-applied patches
344+
so a reconfigure never double-applies) and **fail-loud** (a patch that no longer applies aborts
345+
the configure — a stale patch can't be silently dropped from a release build).
346+
- **`CMakeLists.txt`** — wired as the llama.cpp `FetchContent_Declare(... PATCH_COMMAND ...)`, so it
347+
runs for **every** C++ build (all CI jobs *and* local `cmake -B build`) from one place — no
348+
per-build-step plumbing.
349+
350+
**On a llama.cpp version bump, every patch must still apply** — if a bump shifts the patched code,
351+
the configure fails with an "does not apply cleanly" error; refresh the diff against the new source
352+
and recommit. Treat `patches/` as part of the upgrade checklist below.
353+
354+
Current patches:
355+
356+
| Patch | Fixes |
357+
|-------|-------|
358+
| `0001-win32-arg-parse-embed-guard.patch` | Windows JNI regression from llama.cpp **#24779** (b9739): `common_params_parse` unconditionally replaced the caller's argv with the process command line (`GetCommandLineW`), so an embedded/JNI caller (`java.exe`) lost its `--model …` args → "Failed to parse model parameters". The patch guards the override to fire **only when the re-derived arg count equals `argc`** — true for the standalone `llama-*` tools (their UTF-8 CLI fix is preserved), false for a JVM host (our already-UTF-8 argv is kept). This is also the shape to PR upstream. |
359+
334360
## Upgrading/Downgrading llama.cpp Version
335361

336-
To change the llama.cpp version, update the following **three** files:
362+
To change the llama.cpp version, update the following **three** files (and re-verify `patches/`):
337363

338364
1. **CMakeLists.txt** — the `GIT_TAG` line for llama.cpp: `GIT_TAG b8831`
339365
2. **README.md** — the badge and link line with the version number

CMakeLists.txt

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,10 +136,18 @@ set(GGML_AVX512 OFF CACHE BOOL "" FORCE)
136136
set(LLAMA_BUILD_UI OFF CACHE BOOL "" FORCE)
137137
# b9284 flipped LLAMA_BUILD_APP default to ON; we don't build the unified binary
138138
set(LLAMA_BUILD_APP OFF CACHE BOOL "" FORCE)
139+
# Local source patches for the fetched llama.cpp tree. Every patches/*.patch|*.diff is applied
140+
# (sorted, idempotently, fail-loud) by cmake/apply-llama-patches.cmake — see that file's header.
141+
# This runs for every C++ build (all CI jobs + local) from one place. <SOURCE_DIR> is substituted
142+
# by FetchContent/ExternalProject to the fetched llama.cpp source root.
139143
FetchContent_Declare(
140144
llama.cpp
141145
GIT_REPOSITORY https://github.com/ggerganov/llama.cpp.git
142146
GIT_TAG b9739
147+
PATCH_COMMAND ${CMAKE_COMMAND}
148+
-DPATCH_DIR=${CMAKE_CURRENT_SOURCE_DIR}/patches
149+
-DLLAMA_SRC=<SOURCE_DIR>
150+
-P ${CMAKE_CURRENT_SOURCE_DIR}/cmake/apply-llama-patches.cmake
143151
)
144152
FetchContent_MakeAvailable(llama.cpp)
145153

TODO.md

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,8 +137,13 @@ proving Ninja Multi-Config + MSVC works on the same tree). The two builds produc
137137

138138
### Known regression (b9739) — Windows JNI: `common_params_parse` ignores caller argv
139139

140-
**Status: root-caused, fix deferred.** Surfaced while bringing PR #248 green (the b9739 build fixes let
141-
the Windows Java jobs run to completion and exposed this).
140+
**Status: FIXED via local source patch (`patches/0001-win32-arg-parse-embed-guard.patch`).** Surfaced
141+
while bringing PR #248 green (the b9739 build fixes let the Windows Java jobs run to completion and
142+
exposed this). Resolved by **fix option 1 below** — the count-guard — applied through the generic
143+
`patches/` mechanism (see CLAUDE.md "Local llama.cpp source patches"), so it covers every C++ build
144+
and re-applies on each clean build. Still worth upstreaming (the guard, or a `common_params_parse_argv`
145+
companion) so the patch can eventually be dropped; until then it must be re-verified on each llama.cpp
146+
bump (the applier fails loud if it no longer applies).
142147

143148
**Symptom.** On **Windows x86_64 only**, every Java test that loads a real model fails in
144149
`LlamaModel.loadModel` (native) with `LlamaException: "Failed to parse model parameters"`

cmake/apply-llama-patches.cmake

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
# SPDX-License-Identifier: MIT
2+
#
3+
# apply-llama-patches.cmake — applies every patch in the repo-root `patches/` directory to the
4+
# llama.cpp source tree fetched by FetchContent. Wired as the llama.cpp `PATCH_COMMAND` in the
5+
# top-level CMakeLists.txt, so it runs for EVERY C++ build (all CI jobs + local) from one place,
6+
# rather than per-build-step.
7+
#
8+
# Design:
9+
# * Cross-platform: invoked via `cmake -P`, so it behaves identically on Linux, macOS and
10+
# Windows (the dockcross/native/MSVC jobs all call the same code path).
11+
# * Every `patches/*.patch` and `patches/*.diff` is applied, sorted by filename (so a numeric
12+
# prefix like 0001-, 0002- defines a deterministic order).
13+
# * Idempotent: `git apply --reverse --check` detects an already-applied patch and skips it, so
14+
# a CMake reconfigure over an already-patched source tree does not fail.
15+
# * Fail-loud: a patch that no longer applies (e.g. after a llama.cpp version bump shifts the
16+
# context) aborts the configure with a clear message, so a stale patch can never be silently
17+
# dropped from a release build.
18+
#
19+
# Invoked as:
20+
# cmake -DPATCH_DIR=<repo>/patches -DLLAMA_SRC=<fetched-src> -P cmake/apply-llama-patches.cmake
21+
22+
if(NOT DEFINED PATCH_DIR OR NOT DEFINED LLAMA_SRC)
23+
message(FATAL_ERROR "apply-llama-patches: both PATCH_DIR and LLAMA_SRC must be defined")
24+
endif()
25+
26+
find_program(GIT_EXECUTABLE NAMES git)
27+
if(NOT GIT_EXECUTABLE)
28+
message(FATAL_ERROR "apply-llama-patches: 'git' not found on PATH (required to apply patches)")
29+
endif()
30+
31+
file(GLOB patch_files "${PATCH_DIR}/*.patch" "${PATCH_DIR}/*.diff")
32+
list(SORT patch_files)
33+
34+
if(NOT patch_files)
35+
message(STATUS "apply-llama-patches: no patches in ${PATCH_DIR} (nothing to apply)")
36+
return()
37+
endif()
38+
39+
foreach(patch IN LISTS patch_files)
40+
get_filename_component(patch_name "${patch}" NAME)
41+
42+
# Already applied? A successful reverse-apply check means the change is present already.
43+
execute_process(
44+
COMMAND "${GIT_EXECUTABLE}" -C "${LLAMA_SRC}" apply --reverse --check "${patch}"
45+
RESULT_VARIABLE reverse_rc
46+
OUTPUT_QUIET ERROR_QUIET)
47+
if(reverse_rc EQUAL 0)
48+
message(STATUS "apply-llama-patches: ${patch_name} already applied — skipping")
49+
continue()
50+
endif()
51+
52+
# Not applied yet — confirm it applies cleanly before touching the tree.
53+
execute_process(
54+
COMMAND "${GIT_EXECUTABLE}" -C "${LLAMA_SRC}" apply --check "${patch}"
55+
RESULT_VARIABLE check_rc
56+
OUTPUT_QUIET ERROR_QUIET)
57+
if(NOT check_rc EQUAL 0)
58+
message(FATAL_ERROR
59+
"apply-llama-patches: ${patch_name} does not apply cleanly to ${LLAMA_SRC}.\n"
60+
" A llama.cpp version bump probably shifted the patched code — refresh the patch "
61+
"against the new source and recommit it.")
62+
endif()
63+
64+
execute_process(
65+
COMMAND "${GIT_EXECUTABLE}" -C "${LLAMA_SRC}" apply "${patch}"
66+
RESULT_VARIABLE apply_rc)
67+
if(NOT apply_rc EQUAL 0)
68+
message(FATAL_ERROR "apply-llama-patches: failed to apply ${patch_name}")
69+
endif()
70+
message(STATUS "apply-llama-patches: applied ${patch_name}")
71+
endforeach()
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
diff --git a/common/arg.cpp b/common/arg.cpp
2+
--- a/common/arg.cpp
3+
+++ b/common/arg.cpp
4+
@@ -924,7 +924,14 @@ bool common_params_parse(int argc, char ** argv, common_params & params, llama_e
5+
bool common_params_parse(int argc, char ** argv, common_params & params, llama_example ex, void(*print_usage)(int, char **)) {
6+
#ifdef _WIN32
7+
auto utf8 = make_utf8_argv();
8+
- if (!utf8.ptrs.empty()) {
9+
+ // java-llama.cpp patch (PR #248): only adopt the process command line (GetCommandLineW) when
10+
+ // the caller actually passed THIS process's own argv -- i.e. the re-derived argument count
11+
+ // matches argc. For the standalone llama-* tools that is always true, so their UTF-8 CLI fix
12+
+ // (upstream llama.cpp #24779) is preserved. For an embedded JNI caller the process is java.exe
13+
+ // (many more args), so the counts differ and our already-UTF-8 argv (from GetStringUTFChars)
14+
+ // is kept instead of being silently discarded -- which otherwise makes common_params_parse_ex
15+
+ // parse java.exe's command line and fail with "Failed to parse model parameters".
16+
+ if (!utf8.ptrs.empty() && static_cast<int>(utf8.buf.size()) == argc) {
17+
argc = static_cast<int>(utf8.buf.size());
18+
argv = utf8.ptrs.data();
19+
}

0 commit comments

Comments
 (0)