Skip to content

vendor : add missing llama_add_compile_flags#19322

Merged
CISC merged 10 commits intomasterfrom
cisc/fix-vendor-sanitizer-build
Feb 5, 2026
Merged

vendor : add missing llama_add_compile_flags#19322
CISC merged 10 commits intomasterfrom
cisc/fix-vendor-sanitizer-build

Conversation

@CISC
Copy link
Member

@CISC CISC commented Feb 4, 2026

Hopefully fixes CIEnsure httplib and boringssl/libressl are built with sanitizer options, see #19291 (comment)

@CISC CISC requested a review from ggerganov as a code owner February 4, 2026 10:23
@CISC CISC requested a review from ngxson February 4, 2026 10:25
@ggerganov
Copy link
Member

I am not yet 100% sure that this is necessary. The workflows on master are passing.

I think the one that are failing such as https://github.com/ggml-org/llama.cpp/actions/runs/21664340188/job/62455945468?pr=19296 are when the PR is created before the new workflow file and Github Actions gets confused in some way.

@CISC
Copy link
Member Author

CISC commented Feb 4, 2026

I am not yet 100% sure that this is necessary. The workflows on master are passing.

I think the one that are failing such as https://github.com/ggml-org/llama.cpp/actions/runs/21664340188/job/62455945468?pr=19296 are when the PR is created before the new workflow file and Github Actions gets confused in some way.

Possibly, though it makes sense to add it here as we are building httplib independently.

@CISC
Copy link
Member Author

CISC commented Feb 4, 2026

Cool, "works", but BoringSSL isn't compatible with -Werror I guess:
https://github.com/ggml-org/llama.cpp/actions/runs/21667745579/job/62467477296?pr=19322#step:4:1835

@CISC
Copy link
Member Author

CISC commented Feb 4, 2026

Well, this is annoying, not sure how to fix this...

@danbev
Copy link
Member

danbev commented Feb 4, 2026

@CISC I was able to reproduce the locally on my M3 and perhaps this will work for x64 as well:

diff --git a/vendor/cpp-httplib/CMakeLists.txt b/vendor/cpp-httplib/CMakeLists.txt
index 3d938d9f3..19cad0800 100644
--- a/vendor/cpp-httplib/CMakeLists.txt
+++ b/vendor/cpp-httplib/CMakeLists.txt
@@ -31,6 +31,16 @@ target_compile_definitions(${TARGET} PRIVATE
 set(OPENSSL_NO_ASM ON CACHE BOOL "Disable OpenSSL ASM code when building BoringSSL or LibreSSL")

 if (LLAMA_BUILD_BORINGSSL)
+
+    if(LLAMA_FATAL_WARNINGS)
+        set(LLAMA_FATAL_WARNINGS_SAVED ${CMAKE_CXX_FLAGS})
+        string(REPLACE "-Werror" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
+        string(REPLACE "-Werror" "" CMAKE_C_FLAGS "${CMAKE_C_FLAGS}")
+
+        set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wunused-parameter")
+        set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wunused-parameter")
+    endif()
+
     set(FIPS OFF CACHE BOOL "Enable FIPS (BoringSSL)")

     set(BORINGSSL_GIT "https://boringssl.googlesource.com/boringssl" CACHE STRING "BoringSSL git repository")
@@ -73,6 +83,10 @@ if (LLAMA_BUILD_BORINGSSL)
     set(CPPHTTPLIB_OPENSSL_SUPPORT TRUE)
     target_link_libraries(${TARGET} PUBLIC ssl crypto)

+    if(LLAMA_FATAL_WARNINGS)
+        set(CMAKE_CXX_FLAGS ${LLAMA_FATAL_WARNINGS_SAVED})
+    endif()
+
 elseif (LLAMA_BUILD_LIBRESSL)
     set(LIBRESSL_VERSION "4.2.1" CACHE STRING "LibreSSL version")

Setting the string replacements to -Wno-unused-parameter allowed the build to pass.

@CISC
Copy link
Member Author

CISC commented Feb 4, 2026

@CISC I was able to reproduce the locally on my M3 and perhaps this will work for x64 as well:

Thanks, normally you aren't supposed to fiddle with CMAKE_CXX_FLAGS, but nothing else seems to work. :P

@CISC
Copy link
Member Author

CISC commented Feb 4, 2026

Setting the string replacements to -Wno-unused-parameter allowed the build to pass.

So in theory just adding that should make the build pass?

@CISC
Copy link
Member Author

CISC commented Feb 4, 2026

@danbev Doesn't seem to work. :(

@danbev
Copy link
Member

danbev commented Feb 4, 2026

@danbev Doesn't seem to work. :(

Sorry about wasting your time. I really thought that would work :(

I gave this a try on my fork and it seems to work there.

@CISC
Copy link
Member Author

CISC commented Feb 4, 2026

@danbev Doesn't seem to work. :(

Sorry about wasting your time. I really thought that would work :(

I gave this a try on my fork and it seems to work there.

No worries, I see the reason it worked for you though, you didn't add llama_add_compile_flags(). :)

I'll experiment some more...

@CISC CISC merged commit 11fb327 into master Feb 5, 2026
74 of 79 checks passed
@CISC CISC deleted the cisc/fix-vendor-sanitizer-build branch February 5, 2026 01:27
liparetejas pushed a commit to liparetejas/llama.cpp that referenced this pull request Feb 23, 2026
* add missing llama_add_compile_flags

* disable all warnings for ssl, crypto and fipsmodule
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants