Fix linking - #2546
Fix linking#2546
Conversation
WalkthroughConditionally include cppinyin only when SHERPA_ONNX_ENABLE_TTS is ON and add cppinyin_core to link inputs across iOS, macOS Swift, C examples, pkg-config, Windows MFC props, and Pascal API. Move core linking of cppinyin into the TTS block and make durations copying conditional in OfflineRecognizer::GetResult. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CMake as CMake
participant Core as sherpa-onnx-core
participant TTS as TTS Modules
Dev->>CMake: Configure (SHERPA_ONNX_ENABLE_TTS=ON/OFF)
alt TTS enabled
CMake->>TTS: include(cppinyin)
CMake->>Core: link piper_phonemize + cppinyin_core
else TTS disabled
CMake--xTTS: skip cppinyin include
CMake->>Core: do not link cppinyin_core
end
sequenceDiagram
participant Caller as API caller
participant Offline as OfflineRecognizer::GetResult
participant Result as InternalResult r
Caller->>Offline: GetResult()
Offline->>Result: obtain r (may be null)
alt r != null
Offline->>Caller: copy text, tokens, json, lang, emotion, event, durations
else r == null
Offline->>Caller: return empty/default fields (no durations access)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
dosubot
Bot
added
the
size:S
There was a problem hiding this comment.
Pull Request Overview
This PR fixes linking issues by ensuring the cppinyin_core library is properly included in builds. The fix addresses issue #2544 by moving the cppinyin dependency from being unconditionally linked to being linked only when TTS is enabled, while adding it to various build configurations that need it.
- Move
cppinyindependency from global scope to TTS-only scope in CMake configuration - Add
cppinyin_corelibrary to Pascal API linking - Update build scripts and configuration files to include
cppinyin_corelibrary
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Moves cppinyin include from global to TTS-specific scope |
| sherpa-onnx/csrc/CMakeLists.txt | Removes global cppinyin_core link and adds it under TTS condition |
| sherpa-onnx/pascal-api/sherpa_onnx.pas | Adds cppinyin_core library linking |
| mfc-examples/*.props | Adds cppinyin_core.lib to Windows build dependencies |
| cmake/sherpa-onnx-static.pc.in | Adds lcppinyin_core to pkg-config library list |
| c-api-examples/Makefile | Adds lcppinyin_core to linker flags |
| build-swift-macos.sh | Includes libcppinyin_core.a in static library build |
| build-ios.sh | Includes libcppinyin_core.a in iOS framework builds |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
c-api-examples/Makefile (1)
7-7: Optionally link cppinyin only for the TTS example to keep ASR example leanIf you want to avoid over-linking decode-file-c-api, link cppinyin only for offline-tts-c-api.
Apply this diff:
- LDFLAGS += -lsherpa-onnx-c-api -lsherpa-onnx-core -lkaldi-decoder-core -lsherpa-onnx-kaldifst-core -lsherpa-onnx-fstfar -lsherpa-onnx-fst -lkaldi-native-fbank-core -lkissfft-float -lpiper_phonemize -lespeak-ng -lucd -lcargs -lonnxruntime -lcppinyin_core + LDFLAGS += -lsherpa-onnx-c-api -lsherpa-onnx-core -lkaldi-decoder-core -lsherpa-onnx-kaldifst-core -lsherpa-onnx-fstfar -lsherpa-onnx-fst -lkaldi-native-fbank-core -lkissfft-float -lpiper_phonemize -lespeak-ng -lucd -lcargs -lonnxruntime @@ - $(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) + $(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) -lcppinyin_coreAlso applies to: 20-21
sherpa-onnx/pascal-api/sherpa_onnx.pas (1)
666-666: Static link adds cppinyin_core; verify build when TTS is OFFThis unconditionally adds {$linklib cppinyin_core}. Since cppinyin_core is now built/linked only when SHERPA_ONNX_ENABLE_TTS is ON, static linking may fail on configurations with TTS disabled if libcppinyin_core.a isn’t produced/installed. Consider guarding this with a conditional define aligned with SHERPA_ONNX_ENABLE_TTS, or ensure the build always produces cppinyin_core even when TTS is OFF.
build-ios.sh (1)
130-137: Add existence checks for per-arch archives before lipoIf TTS gets disabled or a sub-build skips cppinyin, these lipo steps will fail hard. Suggest checking file existence to provide a clearer error.
Apply this diff:
for f in libcppinyin_core.a libkaldi-native-fbank-core.a libkissfft-float.a libsherpa-onnx-c-api.a libsherpa-onnx-core.a \ libsherpa-onnx-fstfar.a libssentencepiece_core.a \ libsherpa-onnx-fst.a libsherpa-onnx-kaldifst-core.a libkaldi-decoder-core.a \ libucd.a libpiper_phonemize.a libespeak-ng.a; do + if [[ ! -f build/simulator_arm64/lib/${f} || ! -f build/simulator_x86_64/lib/${f} ]]; then + echo "Missing ${f} for one or more simulator arches. Ensure SHERPA_ONNX_ENABLE_TTS=ON and libs are built." >&2 + exit 1 + fi lipo -create build/simulator_arm64/lib/${f} \ build/simulator_x86_64/lib/${f} \ -output build/simulator/lib/${f} donebuild-swift-macos.sh (1)
29-42: build-swift-macos.sh: enforce TTS artifacts & harden script
- Append
-DSHERPA_ONNX_ENABLE_TTS=ONto your CMake invocation (or wrap thelibtoolblock inif [ -f install/lib/libcppinyin_core.a ]; then … fi) to avoid failures when TTS is disabled.- Replace
set -exwithset -euxo pipefailand changemkdir -p $dir/cd $dirto quote"$dir"for more robust error handling.mfc-examples/NonStreamingSpeechRecognition/sherpa-onnx-deps.props (1)
18-24: Added cppinyin_core.lib to Windows deps — ensure TTS-built artifacts exist in install/lib.These props unconditionally list TTS libs; builds where SHERPA_ONNX_ENABLE_TTS=OFF will link-fail. If that scenario matters, consider a TTS-specific props file or documenting the TTS=ON requirement for these examples.
mfc-examples/StreamingSpeechRecognition/sherpa-onnx-deps.props (1)
18-24: Mirrored addition of cppinyin_core.lib — consistent with other props; same TTS-on caveat applies.If non-TTS builds of these examples are expected, split or gate the deps; otherwise, document TTS=ON.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)build-ios.sh(3 hunks)build-swift-macos.sh(1 hunks)c-api-examples/Makefile(1 hunks)cmake/sherpa-onnx-static.pc.in(1 hunks)mfc-examples/NonStreamingSpeechRecognition/sherpa-onnx-deps.props(1 hunks)mfc-examples/NonStreamingTextToSpeech/sherpa-onnx-deps.props(1 hunks)mfc-examples/StreamingSpeechRecognition/sherpa-onnx-deps.props(1 hunks)sherpa-onnx/csrc/CMakeLists.txt(1 hunks)sherpa-onnx/pascal-api/sherpa_onnx.pas(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-06T04:23:50.237Z
Learnt from: litongjava
PR: k2-fsa/sherpa-onnx#2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:23:50.237Z
Learning: The sherpa-onnx JNI library files are stored in Hugging Face repository at https://huggingface.co/csukuangfj/sherpa-onnx-libs under versioned directories like jni/1.12.7/, and the actual Windows JNI library filename is "sherpa-onnx-jni.dll" as defined in Core.java constants.
Applied to files:
mfc-examples/NonStreamingSpeechRecognition/sherpa-onnx-deps.propssherpa-onnx/csrc/CMakeLists.txtmfc-examples/NonStreamingTextToSpeech/sherpa-onnx-deps.propsbuild-ios.shmfc-examples/StreamingSpeechRecognition/sherpa-onnx-deps.props
📚 Learning: 2025-08-06T04:18:47.981Z
Learnt from: litongjava
PR: k2-fsa/sherpa-onnx#2440
File: sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/core/Core.java:4-6
Timestamp: 2025-08-06T04:18:47.981Z
Learning: In sherpa-onnx Java API, the native library names in Core.java (WIN_NATIVE_LIBRARY_NAME = "sherpa-onnx-jni.dll", UNIX_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.so", MACOS_NATIVE_LIBRARY_NAME = "libsherpa-onnx-jni.dylib") are copied directly from the compiled binary filenames and should not be changed to match other libraries' naming conventions.
Applied to files:
sherpa-onnx/csrc/CMakeLists.txtbuild-ios.sh
🔇 Additional comments (6)
c-api-examples/Makefile (1)
7-7: cppinyin_core correctly added; link order is safeAppending -lcppinyin_core after -lsherpa-onnx-core is correct for resolving Pinyin symbols. No issues seen.
sherpa-onnx/csrc/CMakeLists.txt (1)
313-315: Scoped TTS linking is correctLinking cppinyin_core (and piper_phonemize) only inside the SHERPA_ONNX_ENABLE_TTS block aligns with the reported undefined symbol and avoids pulling it into non-TTS builds.
build-ios.sh (1)
142-155: Merging libcppinyin_core into the fat archives fixes the iOS undefined symbolIncluding libcppinyin_core.a in both simulator and OS64 libtool merges is the right fix for the reported PinyinEncoder::Load() missing symbol.
Also applies to: 156-170
mfc-examples/NonStreamingTextToSpeech/sherpa-onnx-deps.props (1)
18-18: Windows MFC: cppinyin_core.lib correctly addedAppropriate for TTS; link order in the property list is fine.
CMakeLists.txt (1)
399-406: Conditionally including cppinyin under SHERPA_ONNX_ENABLE_TTS is correct.This aligns the dependency with the feature flag and prevents pulling it into non-TTS builds.
cmake/sherpa-onnx-static.pc.in (1)
25-25: Include -lcppinyin_core in static pkg-config — ordering looks correct for static linkers.It appears after libraries that may reference it, which is appropriate for --static link lines.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
sherpa-onnx/c-api/cxx-api.cc (4)
163-178: Fix potential null dereference in OnlineRecognizer::GetResultr is dereferenced without a null check; guard all accesses.
Apply:
OnlineRecognizerResult ans; - ans.text = r->text; - - ans.tokens.resize(r->count); - for (int32_t i = 0; i != r->count; ++i) { - ans.tokens[i] = r->tokens_arr[i]; - } - - if (r->timestamps) { - ans.timestamps.resize(r->count); - std::copy(r->timestamps, r->timestamps + r->count, ans.timestamps.data()); - } - - ans.json = r->json; + if (r) { + ans.text = r->text ? r->text : ""; + + ans.tokens.resize(r->count); + for (int32_t i = 0; i != r->count; ++i) { + ans.tokens[i] = r->tokens_arr[i]; + } + + if (r->timestamps) { + ans.timestamps.resize(r->count); + std::copy(r->timestamps, r->timestamps + r->count, + ans.timestamps.data()); + } + + ans.json = r->json ? r->json : ""; + }
571-573: Same loop bug in KeywordSpotter::Decode (multi-stream)Increment i, not n.
- for (int32_t i = 0; i != n; ++n) { + for (int32_t i = 0; i != n; ++i) { streams[i] = ss[i].Get(); }
303-305: Destroy() ignores argument; use p not p_This can free the wrong pointer in move scenarios.
-void OfflineRecognizer::Destroy(const SherpaOnnxOfflineRecognizer *p) const { - SherpaOnnxDestroyOfflineRecognizer(p_); -} +void OfflineRecognizer::Destroy(const SherpaOnnxOfflineRecognizer *p) const { + SherpaOnnxDestroyOfflineRecognizer(p); +}
151-154: Replace++nwith++iin both loops
- sherpa-onnx/c-api/cxx-api.cc:152
- sherpa-onnx/c-api/cxx-api.cc:571
-for (int32_t i = 0; i != n; ++n) { +for (int32_t i = 0; i != n; ++i) {
🧹 Nitpick comments (2)
sherpa-onnx/c-api/cxx-api.cc (2)
373-392: Add durations to GetResultPtr for parity with GetResultPopulate ans->durations when available.
if (r) { ans->text = r->text; @@ ans->event = r->event ? r->event : ""; + if (r->durations) { + ans->durations.resize(r->count); + std::copy(r->durations, r->durations + r->count, + ans->durations.data()); + } }
561-563: Avoid returning the result of a void call (clarity)Call then return; reduces confusion in void functions.
-void KeywordSpotter::Decode(const OnlineStream *s) const { - return SherpaOnnxDecodeKeywordStream(p_, s->Get()); -} +void KeywordSpotter::Decode(const OnlineStream *s) const { + SherpaOnnxDecodeKeywordStream(p_, s->Get()); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
sherpa-onnx/c-api/cxx-api.cc(1 hunks)
🔇 Additional comments (2)
sherpa-onnx/c-api/cxx-api.cc (2)
358-361: Durations copy under r-guard — LGTMMoving durations handling inside the if (r) block avoids dereferencing a null result. Looks good.
579-596: Consider null-checking r in KeywordSpotter::GetResultIf the C API can return null, current code would dereference it.
If null is possible, apply:
- KeywordResult ans; - ans.keyword = r->keyword; + KeywordResult ans; + if (!r) { + return ans; + } + ans.keyword = r->keyword ? r->keyword : ""; @@ - ans.json = r->json; + ans.json = r->json ? r->json : "";
Fixes #2544. The issue is introduced by #2487
Summary by CodeRabbit
Bug Fixes
Chores