Skip to content

ci : enable bindings java job #3070

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 25, 2025
Merged

ci : enable bindings java job #3070

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e00e432
ci : re-enable bindings-java (java) job
danbev Mar 27, 2025
f172fd0
build : add _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR macro
danbev Apr 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,22 @@ if (NOT TARGET ggml)
add_library(ggml ALIAS ggml::ggml)
else()
add_subdirectory(ggml)
if(WIN32)
# The following adds a _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR macro and is a workaround for
Copy link
Member Author

Choose a reason for hiding this comment

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

I have some notes gathered while looking into this:
https://github.com/danbev/learning-ai/blob/main/notes/whisper/win-java-bindings-issue.md

Copy link
Member Author

Choose a reason for hiding this comment

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

@ggerganov Does this look acceptable as a workaround for this? With this the bindings-java job is able to pass.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, is this a mutex that we use in ggml?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it seems to come from https://github.com/ggml-org/whisper.cpp/blob/master/ggml/src/ggml-threading.cpp.

Copy link
Member

@ggerganov ggerganov Apr 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Does it make a difference if this becomes:

static std::mutex ggml_critical_section_mutex;

I.e. is this CMake flag still necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I.e. is this CMake flag still necessary

Unfortunately, just this change still produces the error:

> git diff
warning: in the working copy of 'bindings/javascript/package.json', CRLF will be replaced by LF the next time Git touches it
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 34ef7958..070818e5 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -149,7 +149,7 @@ if (NOT TARGET ggml)
             #
             # Specifically to whisper.cpp this would cause a crash when using the Java bindings.
             # resulting in a Invalid memory access error.
-            target_compile_definitions(ggml-base PRIVATE _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR)
+            #target_compile_definitions(ggml-base PRIVATE _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR)
         endif()
     endif()
     # ... otherwise assume ggml is added by a parent CMakeLists.txt
diff --git a/ggml/src/ggml-threading.cpp b/ggml/src/ggml-threading.cpp
index 25a19eed..8f6b2b69 100644
--- a/ggml/src/ggml-threading.cpp
+++ b/ggml/src/ggml-threading.cpp
@@ -1,7 +1,7 @@
 #include "ggml-threading.h"
 #include <mutex>

-std::mutex ggml_critical_section_mutex;
+static std::mutex ggml_critical_section_mutex;

 void ggml_critical_section_start() {
     ggml_critical_section_mutex.lock();
whisper_init_from_file_with_params_no_state: loading model from '../../models/ggml-tiny.en.bin'
whisper_init_with_params_no_state: use gpu    = 1
whisper_init_with_params_no_state: flash attn = 0
whisper_init_with_params_no_state: gpu_device = 0
whisper_init_with_params_no_state: dtw        = 0

WhisperCppTest > initializationError FAILED
    java.lang.Error: Invalid memory access
        at com.sun.jna.Native.invokePointer(Native Method)
        at com.sun.jna.Function.invokePointer(Function.java:497)
        at com.sun.jna.Function.invoke(Function.java:441)
        at com.sun.jna.Function.invoke(Function.java:361)
        at com.sun.jna.Library$Handler.invoke(Library.java:270)
        at jdk.proxy3/jdk.proxy3.$Proxy12.whisper_init_from_file_with_params(Unknown Source)
        at io.github.ggerganov.whispercpp.WhisperCpp.initContextImpl(WhisperCpp.java:63)
        at io.github.ggerganov.whispercpp.WhisperCpp.initContext(WhisperCpp.java:39)
        at io.github.ggerganov.whispercpp.WhisperCppTest.init(WhisperCppTest.java:28)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for the detailed investigation. @slaren FYI

# the Windows C++ standard library which does not support constexpr mutexes.
# From the release notes://github.com/microsoft/STL/wiki/Changelog
# Disable constexpr mutex constructor on Windows
# Fixed mutex's constructor to be constexpr. #3824 #4000 #4339
# Note: Programs that aren't following the documented restrictions on binary compatibility may encounter
# null dereferences in mutex machinery. You must follow this rule:
# When you mix binaries built by different supported versions of the toolset, the Redistributable version
# must be at least as new as the latest toolset used by any app component.
# You can define _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR as an escape hatch.
#
# Specifically to whisper.cpp this would cause a crash when using the Java bindings.
# resulting in a Invalid memory access error.
target_compile_definitions(ggml-base PRIVATE _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR)
endif()
endif()
# ... otherwise assume ggml is added by a parent CMakeLists.txt
endif()
Expand Down
Loading