Repository navigation
Split sherpa-onnx Python package - #2521
Conversation
dosubot
Bot
added
the
size:XXL
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds split packaging (sherpa-onnx-core / sherpa-onnx-bin), updates build/install logic and CMake for split mode, upgrades pybind11 and cibuildwheel to v3.1.4, restructures CI into core/test/build jobs across OS/arch with artifact passing, and adds gated HuggingFace/PyPI publishing and wheel tooling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant GH as GitHub Actions
participant Core as core job
participant Test as test job
participant Build as build_wheels job
participant HF as HuggingFace
participant PyPI as PyPI
User->>GH: Trigger workflow (input: publish_sherpa_onnx_bin?)
GH->>Core: run core (build libs, ALSA, produce core wheel artifact)
Core->>GH: upload wheels-core-* artifact
GH->>Test: needs: core (download artifact)
Test->>Test: install core wheels, run verification
Test-->>HF: publish core wheels (if HF token)
Test-->>PyPI: publish bin wheels (if input true)
GH->>Build: needs: core, test
Build->>Build: run cibuildwheel v3.1.4, produce final wheels
Build->>GH: upload wheel artifacts
Build-->>HF: publish final wheels (conditional)
Build-->>PyPI: publish final wheels (gated)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120–180 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
|
To try it before we release a new version, you can run |
There was a problem hiding this comment.
Pull Request Overview
This PR implements the splitting of the monolithic sherpa-onnx Python package into three separate packages to address malware flagging issues and reduce storage duplication. The split creates sherpa-onnx-core (shared libraries), sherpa-onnx-bin (executables), and sherpa-onnx (Python API) packages.
Key changes include:
- Refactored import paths to use
sherpa_onnx.lib._sherpa_onnxinstead of direct_sherpa_onnximports - Added new package structures and setup scripts for sherpa-onnx-core and sherpa-onnx-bin
- Updated build system to support package splitting with
SHERPA_ONNX_SPLIT_PYTHON_PACKAGEenvironment variable
Reviewed Changes
Copilot reviewed 35 out of 37 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sherpa-onnx/python/sherpa_onnx/*.py | Updated import paths to use new package structure |
| setup.py | Added split package logic and dependency on sherpa-onnx-core |
| scripts/wheel/sherpa-onnx-core/ | New package setup for core shared libraries |
| scripts/wheel/sherpa-onnx-bin/ | New package setup for binary executables |
| cmake/cmake_extension.py | Enhanced build system to support package splitting |
| .github/workflows/*.yaml | Updated CI workflows to build and test split packages |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| namespace sherpa_onnx { | ||
|
|
There was a problem hiding this comment.
[nitpick] The macro C_CONTIGUOUS was removed but the namespace declaration remains orphaned after the removal. This creates inconsistent formatting and could indicate incomplete refactoring.
| namespace sherpa_onnx { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
scripts/dotnet/run.sh (1)
126-143: Windows x64: copy path still points to legacy .data/data/bin layout; core wheels place DLLs under sherpa_onnx/lib.Unzipping the core wheel yields DLLs under sherpa_onnx/lib (same as Linux/macOS), not under sherpa_onnx-${version}.data/data/bin. The current cp will miss the DLLs.
Apply this minimal fix:
- cp -v sherpa_onnx-${SHERPA_ONNX_VERSION}.data/data/bin/*.dll ../ + cp -v sherpa_onnx/lib/*.dll ../If we still produce .data-based wheels for Windows in some lanes, consider a fallback copy:
+ cp -v sherpa_onnx/lib/*.dll ../ || cp -v sherpa_onnx-${SHERPA_ONNX_VERSION}.data/data/bin/*.dll ../scripts/go/release.sh (1)
141-156: Windows (amd64/win32): cp source still points to .data/data/bin; core wheel lays out DLLs under sherpa_onnx/lib.After unzipping the core wheels, DLLs are under sherpa_onnx/lib. The current cp will fail.
Patch both blocks:
- cp -v sherpa_onnx-${SHERPA_ONNX_VERSION}.data/data/bin/*.dll $dst + cp -v sherpa_onnx/lib/*.dll $dstand
- cp -v sherpa_onnx-${SHERPA_ONNX_VERSION}.data/data/bin/*.dll $dst + cp -v sherpa_onnx/lib/*.dll $dstcmake/cmake_extension.py (1)
134-136: Ensure out_bin_dir exists before copying binaries.If not created elsewhere, shutil.copy to out_bin_dir will fail.
Add:
out_bin_dir = Path(self.build_lib).parent / "sherpa_onnx" / "bin" install_dir = Path(self.build_lib).resolve() / "sherpa_onnx" +os.makedirs(out_bin_dir, exist_ok=True)scripts/dart/release.sh (1)
53-56: Windows extraction path likely outdated: copy from sherpa_onnx/lib, not .data/data/bin.Core wheels place DLLs under sherpa_onnx/lib across platforms (consistent with Linux/macOS blocks). The current path will miss files and break the release step on Windows.
Apply this diff:
- cp -v sherpa_onnx-${SHERPA_ONNX_VERSION}.data/data/bin/*.dll ../windows + cp -v sherpa_onnx/lib/*.dll ../windowscmake/onnxruntime-win-x64.cmake (1)
85-94: Ensure Windows CLI executables can locate onnxruntime DLLs in split‐package modeWith SHERPA_ONNX_SPLIT_PYTHON_PACKAGE enabled, the onnxruntime DLLs are only installed into the Python wheel’s site-packages directory, not alongside the executables in bin. We didn’t find any calls to SetDefaultDllDirectories, AddDllDirectory, or os.add_dll_directory in the codebase to adjust the DLL search path, so pure C++ CLI binaries on Windows will fail at runtime with “missing DLL” errors.
Please choose one of the following fixes and update CI to launch a couple of executables on Windows in split mode:
Modify the CMake install logic in cmake/onnxruntime-win-x64.cmake (lines 85–94) so that when building the sherpa-onnx-bin wheel, DLLs are also installed into bin. For example:
-if(NOT SHERPA_ONNX_SPLIT_PYTHON_PACKAGE) +if(NOT SHERPA_ONNX_SPLIT_PYTHON_PACKAGE OR SHERPA_ONNX_BIN_PACKAGE) if(SHERPA_ONNX_ENABLE_PYTHON) install(FILES ${onnxruntime_lib_files} DESTINATION ..) else() install(FILES ${onnxruntime_lib_files} DESTINATION lib) endif() install(FILES ${onnxruntime_lib_files} DESTINATION bin) endif()(Set SHERPA_ONNX_BIN_PACKAGE when packaging sherpa-onnx-bin.)
Adjust the executables’ startup code on Windows to call SetDefaultDllDirectories and AddDllDirectory (or use os.add_dll_directory in Python) so they include the site-packages “lib” path at runtime.
sherpa-onnx/python/sherpa_onnx/offline_recognizer.py (1)
6-29: Update stale_sherpa_onnximports and remove unusedOfflineCtcFstDecoderConfigThe scan uncovered two test modules still importing the old C-extension directly. We need to:
- Remove the unused
OfflineCtcFstDecoderConfigimport in the recognizer.- Update any remaining
import _sherpa_onnxto import fromsherpa_onnx.lib._sherpa_onnx.• sherpa-onnx/python/sherpa_onnx/offline_recognizer.py
Remove the unused import to silence flake8 F401:from sherpa_onnx.lib._sherpa_onnx import ( FeatureExtractorConfig, HomophoneReplacerConfig, OfflineCanaryModelConfig, - OfflineCtcFstDecoderConfig, OfflineDolphinModelConfig, OfflineFireRedAsrModelConfig, OfflineLMConfig, OfflineModelConfig, OfflineMoonshineModelConfig, OfflineNemoEncDecCtcModelConfig, OfflineParaformerModelConfig, )• sherpa-onnx/python/tests/test_feature_extractor_config.py (line 11)
- import _sherpa_onnx + from sherpa_onnx.lib import _sherpa_onnx• sherpa-onnx/python/tests/test_online_transducer_model_config.py (line 11)
- import _sherpa_onnx + from sherpa_onnx.lib import _sherpa_onnxPlease apply these changes and verify no other modules import
_sherpa_onnxdirectly..github/workflows/build-wheels-win64.yaml (1)
347-360: Make PyPI publish Python-3.7-safe (pin pip and twine appropriately)Twine 5.x and pip 24+ don’t support Python 3.7; current logic will break on 3.7. Pin compatible versions.
- name: Publish wheels to PyPI shell: bash env: TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} run: | python3 -m pip install --upgrade pip - if [[ ${{ matrix.python-version }} == "3.7" ]]; then - python3 -m pip install wheel twine setuptools + if [[ ${{ matrix.python-version }} == "3.7" ]]; then + python3 -m pip install "pip<24" "setuptools<70" wheel "twine<5" else python3 -m pip install wheel twine==5.0.0 setuptools fi - twine upload ./wheelhouse/*.whl.github/workflows/build-wheels-macos-x64.yaml (1)
339-345: Remove unnecessary pip flag and upgrade pip firstAs above for macOS, drop --break-system-packages and upgrade pip first.
- opts='--break-system-packages' - - python3 -m pip install $opts --upgrade pip - python3 -m pip install $opts wheel twine==5.0.0 setuptools + python3 -m pip install --upgrade pip + python3 -m pip install wheel twine==5.0.0 setuptools
♻️ Duplicate comments (4)
sherpa-onnx/csrc/CMakeLists.txt (3)
471-476: Same rpath concerns apply here (ALSA binaries).Mirror the additions for dist-packages and Python 3.15, and consider centralizing the version list to avoid drift across blocks.
570-575: Same rpath concerns apply here (PortAudio binaries).Please align with the first comment’s suggestions (dist-packages fallback, add 3.15, centralize versions, optionally use target_link_options()).
632-637: Same rpath concerns apply here (WebSocket binaries).Replicate the improvements noted above for consistency across all executable groups.
cmake/onnxruntime-win-x86.cmake (1)
85-94: Same Windows DLL loading concern as x64.The same risk applies to Win32 builds. Please adopt the same remediation (conditional install for the bin package or AddDllDirectory in exes) to prevent missing-DLL runtime errors.
🧹 Nitpick comments (71)
.gitignore (1)
149-149: LGTM; adds broad ignore for egg-info artifacts.This complements the existing
sherpa_onnx.egg-info/entry and will catch egg-info across subpackages, which is useful with the split-package layout. Minor nit: there are a few duplicated globs elsewhere (e.g.,*.zipappears twice). Not a blocker.scripts/wheel/sherpa-onnx-core/MANIFEST.in (1)
1-2: Ensure non-Python assets land in the wheelIt looks like you already have in
scripts/wheel/sherpa-onnx-core/setup.py:include_package_data=True, data_files=[("Scripts", get_binaries())],However,
data_files=[("Scripts", …)]will place binaries under a top-levelScripts/directory in the wheel, not insidesherpa_onnx/lib/orsherpa_onnx/include/. To be sure:
Verify that
get_binaries()actually returns the files undersherpa_onnx/lib/andsherpa_onnx/include/.Build a wheel locally and inspect its contents. For example:
python -m build --wheel --outdir dist . for whl in dist/*.whl; do echo "Inspecting $whl" unzip -l "$whl" \ | rg -nP 'sherpa_onnx/(lib|include)/' \ || echo "→ Missing lib/ or include/ assets!" doneIf you find the directories are missing or misplaced, you can explicitly include them with
package_datainstead of (or in addition to)data_files, e.g.:setup( …, include_package_data=True, package_data={ "sherpa_onnx": ["lib/**/*", "include/**/*"], }, # keep data_files if you still need top-level Scripts/ )Finally, don’t forget to bundle any third-party license files for the native binaries. You might add to
MANIFEST.in:recursive-include sherpa_onnx/licenses *This will make downstream compliance simpler.
sherpa-onnx/python/csrc/fast-clustering.cc (2)
35-54: Prefer auto-ensuring C-contiguity via pybind11 flags instead of manual check.Rather than rejecting non-contiguous arrays, construct a C-contiguous, float32 view/copy. This is more user-friendly and robust with NumPy 2.x changes.
Apply:
@@ - if (!(features.flags() & py::array::c_style)) { - throw py::value_error( - "input features should be contiguous. Please use " - "np.ascontiguousarray(features)"); - } + // Ensure C-contiguous float32; copies only if needed + py::array_t<float, py::array::c_style | py::array::forcecast> contig = features; @@ - int num_dim = features.ndim(); + int num_dim = contig.ndim(); @@ - int32_t num_rows = features.shape(0); - int32_t num_cols = features.shape(1); - float *p = features.mutable_data(); + int32_t num_rows = contig.shape(0); + int32_t num_cols = contig.shape(1); + float *p = contig.mutable_data();Optional follow-ups:
- If you want to avoid mutating caller memory during normalization, copy to a temporary buffer and pass that to
Cluster.- If mutation is intended for performance, add a docstring note indicating the input array may be modified in-place.
Also consider checking writability for clearer errors:
if (!(contig.flags() & py::array::writeable)) { throw py::value_error("input features must be writeable (not read-only)"); }
49-53: In-place normalization likely mutates the Python input; confirm intent.
Cluster()normalizes rows via Eigen, which will mutate the passed memory. Withmutable_data()this can change the caller’sfeatures. If that’s not desirable, take a local copy before callingCluster.Example (no diff; touches lines outside the changed hunk):
// After obtaining contig as above py::buffer_info info = contig.request(); auto* src = static_cast<float*>(info.ptr); std::vector<float> buf(src, src + (info.shape[0] * info.shape[1])); py::gil_scoped_release release; return self.Cluster(buf.data(), static_cast<int32_t>(info.shape[0]), static_cast<int32_t>(info.shape[1]));If mutation is intentional for performance, please document it in the Python docstring and type stub.
sherpa-onnx/python/csrc/offline-source-separation.cc (3)
83-87: Contiguity check switched to py::array::c_style — verify intent and consider enforcing via type flagsUsing
samples.flags() & py::array::c_stylealso implies alignment, making the check stricter than the priorC_CONTIGUOUS-only test. If that’s intended, great; if not, this may reject arrays that are contiguous but not aligned.To push this guarantee to the binding boundary and drop the runtime check (cleaner and less error-prone), consider requiring C-style contiguity in the parameter type:
- [](const PyClass &self, int32_t sample_rate, - const py::array_t<float> &samples) { - if (!(samples.flags() & py::array::c_style)) { - throw py::value_error( - "input samples should be contiguous. Please use " - "np.ascontiguousarray(samples)"); - } + [](const PyClass &self, int32_t sample_rate, + const py::array_t<float, py::array::c_style> &samples) {If you also want to accept convertible dtypes (e.g., float64) with an automatic copy, add
py::array::forcecast. In either case, consider updating the error message to mention “C-contiguous and aligned” for clarity if you keep the runtime check.
13-13: Remove unused C_CONTIGUOUS macro
#define C_CONTIGUOUS ...is now unused after switching topy::array::c_style. Please remove it to avoid confusion.
1-1: Header comment path looks incorrectThe top-of-file comment says
offline-source-separation-config.cc, but this file isoffline-source-separation.cc. Minor, but worth correcting to aid grepability.new-release.sh (1)
27-29: Version bump propagation to wheel setup.py files — works; minor hardening suggestionsThe
find ... -exec sed -i.bak ...additions will correctly sweep versions across the split packages. Two small niceties you may consider:
- Use a delimiter that won’t collide with paths, e.g.
|instead of/, to simplify escaping.- Add
set -euo pipefail(keep-xif you like) for slightly safer scripting.No blockers.
scripts/wheel/patch_wheel.py (3)
52-71: RPATH set construction — unify variants and include lib64/.libs consistentlyFor the
py_version is Nonebranch, you only emit two paths per version and omitlib64and.libsvariants you include in the exact-version branch. Recommend consolidating to a single helper to build all variants to avoid surprises at runtime.Minimal in-place improvement:
@@ - if py_version: - rpath_list = [ - f"$ORIGIN/../lib/python{py_version}/site-packages/sherpa_onnx/lib", - f"$ORIGIN/../lib/python{py_version}/dist-packages/sherpa_onnx/lib", - # - f"$ORIGIN/../lib/python{py_version}/site-packages/sherpa_onnx/lib64", - f"$ORIGIN/../lib/python{py_version}/dist-packages/sherpa_onnx/lib64", - # - f"$ORIGIN/../lib/python{py_version}/site-packages/sherpa_onnx.libs", - ] - else: - rpath_list = [] - for p in ["3.8", "3.9", "3.10", "3.11", "3.12", "3.13", "3.14"]: - rpath_list.extend( - [ - f"$ORIGIN/../lib/python{p}/site-packages/sherpa_onnx/lib", - f"$ORIGIN/../lib/python{p}/dist-packages/sherpa_onnx/lib", - ] - ) + def paths_for(p: str): + return [ + f"$ORIGIN/../lib/python{p}/site-packages/sherpa_onnx/lib", + f"$ORIGIN/../lib/python{p}/dist-packages/sherpa_onnx/lib", + f"$ORIGIN/../lib/python{p}/site-packages/sherpa_onnx/lib64", + f"$ORIGIN/../lib/python{p}/dist-packages/sherpa_onnx/lib64", + f"$ORIGIN/../lib/python{p}/site-packages/sherpa_onnx.libs", + ] + + if py_version: + rpath_list = paths_for(py_version) + else: + rpath_list = [] + for p in ["3.8", "3.9", "3.10", "3.11", "3.12", "3.13", "3.14"]: + rpath_list.extend(paths_for(p))
74-85: Avoid double “:” when existing rpath is empty and prefer arg list over shell=TrueTwo tweaks:
- Handle empty
existing_rpathto avoid trailing “:”.- Prefer passing args as a list to
subprocessfor safety.@@ - for filename in glob.glob(f"{tmp_dir}/sherpa_onnx*data/data/bin/*", recursive=True): + for filename in glob.glob(f"{tmp_dir}/sherpa_onnx*data/data/bin/*", recursive=True): print(filename) @@ - target_rpaths = rpaths + ":" + existing_rpath - subprocess.check_call( - f"patchelf --force-rpath --set-rpath '{target_rpaths}' {filename}", - shell=True, - ) + target_rpaths = rpaths if not existing_rpath else f"{rpaths}:{existing_rpath}" + subprocess.check_call( + ["patchelf", "--force-rpath", "--set-rpath", target_rpaths, filename] + )Optional: also convert the earlier
unzipcall to a list form.
30-33: Make tmp dir unique per wheel to survive partial failures and parallelism
tmp_dir = out_dir / "tmp"works serially but can clash across multiple wheels or if a prior run didn’t clean up. Consider using a per-wheel temp dir (e.g.,tmp-{whl.stem}) ortempfile.mkdtemp().If you choose
mkdtemp, add an import and change:import tempfile tmp_dir = Path(tempfile.mkdtemp(prefix=f"patch-{whl.stem}-", dir=out_dir))This keeps runs isolated and more robust.
scripts/wheel/sherpa-onnx-bin/setup.py (3)
11-13: Remove debug print to keep pip/build logs clean
print("bin_files", bin_files)is noisy during packaging and installation. Recommend removing it.bin_files = glob.glob("bin/*") -print("bin_files", bin_files)
24-27: data_files target is OK; add a guard for empty bin/ and consider entry points later
- The
("Scripts" if Windows else "bin")target looks correct for placing executables on PATH.- Add a simple guard to fail fast if
bin_filesis empty, preventing publishing an effectively empty wheel.Example:
if not bin_files: raise RuntimeError("No files found under bin/; sherpa-onnx-bin would be empty.")Longer-term, if some tools are Python entry points,
console_scriptsmight be preferable; but for prebuilt native executables,data_filesis appropriate.
7-9: Minor duplication: is_windows() exists elsewhere
is_windows()is also defined in other packaging helpers. Not a blocker, but a tiny shared util (or inlining theplatform.system()check) would reduce duplication.scripts/go/release.sh (1)
94-95: Hard-coding libonnxruntime.1.17.1.dylib is brittle; create/update the unversioned alias dynamically.Pinning the exact ORT version in the script will drift. Safer to symlink the first matching versioned file to the unversioned name.
Suggested change:
- cp -v libonnxruntime.1.17.1.dylib libonnxruntime.dylib + ver=$(ls -1 libonnxruntime.*.dylib | head -n1) + test -n "$ver" && ln -sf "$(basename "$ver")" libonnxruntime.dylibRepeat the same for the arm64 block.
Also applies to: 112-113
cmake/cmake_extension.py (3)
146-164: Split-mode gating of CMake args is sound; minor nit on comments.The conditional suppression of INSTALL_PREFIX, C API, and WebSocket in split mode is appropriate. Small nit: comment on Line 170 spells “onverride”.
Apply:
-# so they can onverride the "defaults" stored in `extra_cmake_args` +# so they can override the "defaults" stored in `extra_cmake_args`
173-201: Windows build: printed heredoc ‘build_cmd’ isn’t executed; only the explicit os.system calls are.This is fine, but the big heredoc can drift from the actual commands used right below. Consider removing the unused ‘build_cmd’ or executing it to avoid duplication.
209-222: Ninja/Make branches are correct; consider capturing non-zero exit to show stderr for easier debugging.Today failures route through the combined ret check. If diagnosing flakes in CI, you might want to print the last 100 lines of the build dir logs if present.
Also applies to: 223-239
scripts/wheel/sherpa-onnx-core/sherpa_onnx/_info.py (1)
8-13: Library order looks correct; ensure unversioned ORT symlink exists in wheels.For -lonnxruntime to resolve on Linux/macOS, libonnxruntime.{so,dylib} (unversioned) must be present or symlinked. Your Go script currently creates the macOS alias manually, suggesting the core wheel may only include the versioned file.
- Please confirm the core wheel ships an unversioned libonnxruntime.{so,dylib} next to the versioned one.
- If not, we should either add the alias during packaging or switch the CLI to print -l:libonnxruntime..{so,dylib} where supported.
Optionally make these tuples to discourage accidental mutation:
-onnxruntime_lib = ["onnxruntime"] -c_lib = ["sherpa-onnx-c-api"] + onnxruntime_lib -cxx_lib = ["sherpa-onnx-cxx-api"] + c_lib +onnxruntime_lib = ("onnxruntime",) +c_lib = ("sherpa-onnx-c-api",) + onnxruntime_lib +cxx_lib = ("sherpa-onnx-cxx-api",) + c_libscripts/wheel/sherpa-onnx-core/sherpa_onnx/__main__.py (2)
5-11: Add --help and exit 0 on help; current usage shows only on empty args.Small UX tweak: support -h/--help and exit 0 to integrate better with scripts.
- if not args: - print( + if not args or args[0] in ("-h", "--help"): + print( "Usage: python3 -m sherpa_onnx [--cflags|--c-api-libs|--c-api-libs-only-L|--c-api-libs-only-l|--cxx-api-libs|--cxx-api-libs-only-L|--cxx-api-libs-only-l]" ) - sys.exit(1) + sys.exit(0 if args else 1)
13-31: Windows note: emitted -l flags won’t work with MSVC; consider documenting or offering MSVC-friendly output.Current output is POSIX-style. If this tool is used on Windows with cl.exe, consider a flag that prints .lib names and link directories (e.g., /link /LIBPATH:… onnxruntime.lib).
Happy to add a --msvc option that emits suitable flags if desired.
scripts/dart/release.sh (3)
28-34: Remove unused variables to satisfy shellcheck SC2034 and avoid confusion.linux_wheel, macos_wheel, and windows_x64_wheel are never used.
Apply this diff:
-linux_wheel=$src_dir/$linux_wheel_filename +# +# Intentionally keep only filenames; full paths are constructed at download time. +# -macos_wheel=$src_dir/$macos_wheel_filename - -windows_x64_wheel=$src_dir/$windows_x64_wheel_filename
21-22: Make cleanup tolerant to missing files.rm will stop the script (set -e) if notes.md is absent.
Apply this diff:
-rm notes.md +rm -f notes.md
36-68: Harden the script a bit (optional).
- Ensure destination dirs exist before cp (linux/windows/macos).
- Quote variable expansions in cp/curl/unzip to avoid globbing/path surprises.
- Consider set -euo pipefail for stricter error handling.
I can send a follow-up patch adding mkdir -p linux windows macos and quotes if you want.
sherpa-onnx/python/sherpa_onnx/__init__.py (2)
1-76: Silence F401 re-export warnings or declare all.init commonly re-exports symbols; flake8 flags them as unused. Add a targeted noqa on the import line or declare all.
Apply this minimal diff to the import line:
-from sherpa_onnx.lib._sherpa_onnx import ( +from sherpa_onnx.lib._sherpa_onnx import ( # noqa: F401Alternatively, define an explicit all with the imported names to make intent clear.
1-1: Optional: fallback import for developer builds without installed core wheel.If someone imports from a source checkout without sherpa-onnx-core installed, this import will fail. A backward-compat fallback keeps dev workflows smoother.
Example:
+try: + from sherpa_onnx.lib._sherpa_onnx import ( # noqa: F401 + ... + ) +except ImportError: + # Fallback for in-tree builds or older layouts + from _sherpa_onnx import ( # type: ignore # noqa: F401 + ... + )sherpa-onnx/python/sherpa_onnx/keyword_spotter.py (3)
18-21: Avoid using assert for runtime validation; raise explicit exceptions instead.asserts can be stripped with -O, removing safety checks in production.
Apply this refactor:
-def _assert_file_exists(f: str): - assert Path(f).is_file(), f"{f} does not exist" +def _assert_file_exists(f: str) -> None: + if not Path(f).is_file(): + raise FileNotFoundError(f"{f} does not exist")
98-99: Use ValueError instead of assert for parameter validation.Keep checks active regardless of optimization flags.
Apply this diff:
- assert num_threads > 0, num_threads + if num_threads <= 0: + raise ValueError(f"num_threads must be > 0, got {num_threads}")
137-141: Add return type for create_stream.Improves type checking and IDE help.
Apply this diff:
- def create_stream(self, keywords: Optional[str] = None): + def create_stream(self, keywords: Optional[str] = None) -> OnlineStream:scripts/wheel/README.md (2)
9-21: Specify a language for fenced code blocks.Fixes markdownlint MD040 and improves readability.
Apply this diff:
-``` +```bash python3 setup.py bdist_wheel --plat-name=macosx_10_15_x86_64 python3 setup.py bdist_wheel --plat-name=macosx_11_0_arm64 python3 setup.py bdist_wheel --plat-name=macosx_11_0_universal2 python3 setup.py bdist_wheel --plat-name=macosx_10_15_universal2 python3 setup.py bdist_wheel --plat-name=win_amd64 python3 setup.py bdist_wheel --plat-name=win32 python3 setup.py bdist_wheel --plat-name=manylinux2014_x86_64 python3 setup.py bdist_wheel --plat-name=manylinux2014_aarch64 python3 setup.py bdist_wheel --plat-name=linux_armv7l--- `23-24`: **Fill in sherpa-onnx-bin section with a brief build note or placeholder.** Right now it’s an empty header; add a one-liner or TODO to avoid confusion. I can draft a short section mirroring the core wheel commands if helpful. </blockquote></details> <details> <summary>.github/workflows/test-build-wheel.yaml (3)</summary><blockquote> `103-106`: **Fix ccache key variable name.** matrix.python_version is undefined; use matrix.python-version to avoid cache key collisions. Apply this diff: ```diff - key: ${{ matrix.os }}-${{ matrix.python_version }} + key: ${{ matrix.os }}-${{ matrix.python-version }}
109-112: Python 3.7 job may break pip/setuptools upgrades.pip >= 24 and recent setuptools/wheel have dropped 3.7. Either drop 3.7 from the matrix or pin tools for that leg.
Option A (drop 3.7): remove the 3.7 entry.
Option B (pin for 3.7). Example:
run: | - python3 -m pip install --upgrade pip - python3 -m pip install wheel twine setuptools + if [ "${{ matrix.python-version }}" = "3.7" ]; then + python3 -m pip install "pip<24" "setuptools<70" "wheel<0.43" twine + else + python3 -m pip install --upgrade pip + python3 -m pip install wheel twine setuptools + fi
149-154: Test step assumes 'sherpa-onnx' CLI is present.With the split, CLIs may move to sherpa-onnx-bin. Consider testing the Python API here and running CLI tests in a dedicated job that builds/installs sherpa-onnx-bin.
For this job, add a lightweight import/version check:
run: | which sherpa-onnx sherpa-onnx --help + python - <<'PY' +import sherpa_onnx as so +print("sherpa_onnx.version:", so.version()) +PYOr install sherpa-onnx-bin as part of the test if the intent is to validate CLIs.
sherpa-onnx/csrc/CMakeLists.txt (1)
393-396: Split-package rpaths: good direction; add dist-packages fallback and centralize versionsThe new rpaths for split packaging make sense. Two improvements:
- Add a dist-packages fallback for Debian/Ubuntu system Python layouts.
- Include Python 3.15 to future-proof, and centralize the version list to avoid repetition across blocks. Also consider target_link_options() instead of passing linker flags via target_link_libraries() (keep as-is if you need older CMake compatibility).
Apply this localized tweak here (same idea applies to the other rpath blocks below):
- elseif(SHERPA_ONNX_SPLIT_PYTHON_PACKAGE) - foreach(ver in ITEMS 3.8 3.9 3.10 3.11 3.12 3.13 3.14) - target_link_libraries(${exe} "-Wl,-rpath,${SHERPA_ONNX_RPATH_ORIGIN}/../lib/python${ver}/site-packages/sherpa_onnx/lib") - endforeach() + elseif(SHERPA_ONNX_SPLIT_PYTHON_PACKAGE) + foreach(ver in ITEMS 3.8 3.9 3.10 3.11 3.12 3.13 3.14 3.15) + target_link_libraries(${exe} "-Wl,-rpath,${SHERPA_ONNX_RPATH_ORIGIN}/../lib/python${ver}/site-packages/sherpa_onnx/lib") + # Debian/Ubuntu system Python layout + target_link_libraries(${exe} "-Wl,-rpath,${SHERPA_ONNX_RPATH_ORIGIN}/../lib/python${ver}/dist-packages/sherpa_onnx/lib") + endforeach()If you want to DRY this up, define a cached list (e.g., SHERPA_ONNX_PY_VERSIONS) once and reuse it in all blocks.
.github/workflows/export-piper.yaml (1)
163-166: New Polish glos-medium model variants added — minor hardening suggestionThe additions look consistent. As a small robustness tweak for the publishing loop:
- Consider
set -euo pipefailat the top of the shell blocks to fail fast.- Optionally verify the target HF repos exist (or create them) before pushing to reduce retries.
No blockers from this change.
scripts/wheel/sherpa-onnx-core/setup.py (2)
6-7: Consider reusing existing utility function.The
is_windows()function duplicates logic already present incmake/cmake_extension.py(lines 29-30). Consider importing it from the shared module to maintain DRY principle.-def is_windows(): - return platform.system() == "Windows" +from cmake.cmake_extension import is_windows
10-21: Verify binary file paths and consider error handling.The function assumes binary files exist at
./sherpa_onnx/libbut doesn't validate their presence. Consider adding file existence checks to provide clearer error messages if files are missing during packaging.def get_binaries(): if not is_windows(): return None libs = [ "onnxruntime.dll", "sherpa-onnx-c-api.dll", "sherpa-onnx-cxx-api.dll", "sherpa-onnx-c-api.lib", "sherpa-onnx-cxx-api.lib", ] prefix = "./sherpa_onnx/lib" + # Validate files exist + import os + missing_files = [] + for lib in libs: + if not os.path.exists(f"{prefix}/{lib}"): + missing_files.append(f"{prefix}/{lib}") + if missing_files: + raise FileNotFoundError(f"Missing required files: {missing_files}") return [f"{prefix}/{lib}" for lib in libs].github/workflows/build-wheels-win64.yaml (4)
285-295: Use python -m pip on Windows and ensure pip is presentOn Windows runners, python is guaranteed; python3 is not. Also install/upgrade pip once before build.
- - name: Build wheels (cmd) + - name: Build wheels (cmd) shell: bash run: | - python3 -m pip install setuptools wheel twine + python -m pip install --upgrade pip + python -m pip install setuptools wheel twine @@ - python3 setup.py bdist_wheel + python setup.py bdist_wheel
292-292: Remove duplicated CMake flag-DSHERPA_ONNX_ENABLE_C_API=OFF appears twice.
- export SHERPA_ONNX_CMAKE_ARGS="-DSHERPA_ONNX_ENABLE_BINARY=OFF -DSHERPA_ONNX_BUILD_C_API_EXAMPLES=OFF -DSHERPA_ONNX_ENABLE_C_API=OFF -DSHERPA_ONNX_ENABLE_C_API=OFF -DSHERPA_ONNX_ENABLE_WEBSOCKET=OFF" + export SHERPA_ONNX_CMAKE_ARGS="-DSHERPA_ONNX_ENABLE_BINARY=OFF -DSHERPA_ONNX_BUILD_C_API_EXAMPLES=OFF -DSHERPA_ONNX_ENABLE_C_API=OFF -DSHERPA_ONNX_ENABLE_WEBSOCKET=OFF"
146-155: Harden unzip with -- to avoid option confusion when glob expandsPrevents files with leading dashes from being treated as options (SC2035).
- unzip -l ./scripts/wheel/sherpa-onnx-core/dist/*.whl + unzip -l -- ./scripts/wheel/sherpa-onnx-core/dist/*.whl echo "---" - unzip -l ./scripts/wheel/sherpa-onnx-bin/dist/*.whl + unzip -l -- ./scripts/wheel/sherpa-onnx-bin/dist/*.whl
300-305: Harden unzip in Display wheelsSame SC2035 concern; add “--”.
- unzip -l ./wheelhouse/*.whl + unzip -l -- ./wheelhouse/*.whl.github/workflows/build-wheels-macos-universal2.yaml (4)
276-284: Trim trailing space in CIBW_BUILD patternThe trailing space after "* " is unnecessary and could confuse pattern matching.
- CIBW_BUILD: "${{ matrix.python-version}}-* " + CIBW_BUILD: "${{ matrix.python-version}}-*"
289-294: Harden unzip with -- for safetyPrevents misinterpretation if a wheel filename starts with a dash.
- unzip -l ./wheelhouse/*.whl + unzip -l -- ./wheelhouse/*.whl
186-193: Quote command substitutions to avoid word splittingQuote $(which …) targets for robustness (SC2046).
- ls -lh $(which sherpa-onnx) - file $(which sherpa-onnx) - otool -L $(which sherpa-onnx) - otool -l $(which sherpa-onnx) + binpath="$(which sherpa-onnx)" + ls -lh "$binpath" + file "$binpath" + otool -L "$binpath" + otool -l "$binpath"
130-137: SC2035: Use explicit destination on cp globMinor: avoid ambiguous cp targets when glob expands to multiple files.
- cp -v ./scripts/wheel/sherpa-onnx-core/dist/*.whl . - cp -v ./scripts/wheel/sherpa-onnx-bin/dist/*.whl . + cp -v ./scripts/wheel/sherpa-onnx-core/dist/*.whl ./ + cp -v ./scripts/wheel/sherpa-onnx-bin/dist/*.whl ./.github/workflows/build-wheels-aarch64.yaml (4)
198-203: Nit: Step title says “Linux x64” but this is aarch64Avoid confusion in logs.
- - name: Retrieve artifact from Linux x64 + - name: Retrieve artifact from Linux aarch64
349-353: Trim trailing space in CIBW_BUILD patternConsistent with other workflows; removes superfluous whitespace.
- name: wheel-${{ matrix.python-version }}-${{ matrix.manylinux }}-linux-aarch64 + name: wheel-${{ matrix.python-version }}-${{ matrix.manylinux }}-linux-aarch64And:
- CIBW_BUILD: "${{ matrix.python-version}}-* " + CIBW_BUILD: "${{ matrix.python-version}}-*"
360-376: Quote/glob safety in wheel inspection and add “--” to unzipAvoid SC2035/SC2046 pitfalls and unexpected word splitting.
- ls -lh wheelhouse/*.whl - unzip -l wheelhouse/*.whl + ls -lh wheelhouse/*.whl + unzip -l -- wheelhouse/*.whl @@ - mkdir t - cp wheelhouse/*.whl ./t - cd ./t - unzip ./*.whl - ls -lh + mkdir -p t + cp wheelhouse/*.whl ./t/ + cd ./t + unzip -- ./*.whl + ls -lh . @@ - readelf -d *.so + readelf -d -- *.so
138-155: Quote paths and add unzip “--” in core artifact stepsMinor robustness improvements; also avoid permission surprises after docker step.
- mkdir wheelhouse - cp -v ./scripts/wheel/sherpa-onnx-core/dist/*.whl ./wheelhouse - cp -v ./scripts/wheel/sherpa-onnx-bin/dist/*.whl ./wheelhouse + mkdir -p wheelhouse + cp -v ./scripts/wheel/sherpa-onnx-core/dist/*.whl ./wheelhouse/ + cp -v ./scripts/wheel/sherpa-onnx-bin/dist/*.whl ./wheelhouse/ @@ - unzip -l ./scripts/wheel/sherpa-onnx-core/dist/*.whl + unzip -l -- ./scripts/wheel/sherpa-onnx-core/dist/*.whl echo "---" - unzip -l ./scripts/wheel/sherpa-onnx-bin/dist/*.whl + unzip -l -- ./scripts/wheel/sherpa-onnx-bin/dist/*.whl.github/workflows/build-wheels-macos-arm64.yaml (3)
287-289: Harden unzip with “--”Safety nit.
- unzip -l ./wheelhouse/*.whl + unzip -l -- ./wheelhouse/*.whl
188-193: Quote $(which …) usageAvoid word splitting in paths.
- ls -lh $(which sherpa-onnx) - file $(which sherpa-onnx) - otool -L $(which sherpa-onnx) - otool -l $(which sherpa-onnx) + binpath="$(which sherpa-onnx)" + ls -lh "$binpath" + file "$binpath" + otool -L "$binpath" + otool -l "$binpath"
130-137: SC2035: cp glob to explicit destinationMinor cleanliness.
- cp -v ./scripts/wheel/sherpa-onnx-core/dist/*.whl . - cp -v ./scripts/wheel/sherpa-onnx-bin/dist/*.whl . + cp -v ./scripts/wheel/sherpa-onnx-core/dist/*.whl ./ + cp -v ./scripts/wheel/sherpa-onnx-bin/dist/*.whl ./.github/workflows/build-wheels-macos-x64.yaml (5)
269-271: Quote $GITHUB_ENV when echoing env varAvoids SC2086; minor robustness improvement.
- run: echo "MACOSX_DEPLOYMENT_TARGET=10.15" >> $GITHUB_ENV + run: echo "MACOSX_DEPLOYMENT_TARGET=10.15" >> "$GITHUB_ENV"
275-283: Trim trailing space in CIBW_BUILD and keep env tidySame nit as other workflows.
- CIBW_BUILD: "${{ matrix.python-version}}-* " + CIBW_BUILD: "${{ matrix.python-version}}-*"
288-292: Add “--” to unzipSafety nit.
- unzip -l ./wheelhouse/*.whl + unzip -l -- ./wheelhouse/*.whl
189-193: Quote $(which …) usagePrevents word splitting.
- ls -lh $(which sherpa-onnx) - file $(which sherpa-onnx) - otool -L $(which sherpa-onnx) - otool -l $(which sherpa-onnx) + binpath="$(which sherpa-onnx)" + ls -lh "$binpath" + file "$binpath" + otool -L "$binpath" + otool -l "$binpath"
134-138: SC2035: cp glob to explicit destinationMinor cp robustness.
- cp -v ./scripts/wheel/sherpa-onnx-core/dist/*.whl . - cp -v ./scripts/wheel/sherpa-onnx-bin/dist/*.whl . + cp -v ./scripts/wheel/sherpa-onnx-core/dist/*.whl ./ + cp -v ./scripts/wheel/sherpa-onnx-bin/dist/*.whl ./.github/workflows/build-wheels-linux.yaml (13)
8-13: Boolean input default should be unquoted true; avoid stringly-typed booleans.For workflow_dispatch boolean inputs, prefer
default: true(bare) to reduce confusion down the line when parsing the value. You already use a booleantype, so keep it boolean throughout.workflow_dispatch: inputs: publish_sherpa_onnx_bin: description: "Publish sherpa-onnx-bin" required: false - default: "true" + default: true type: boolean
98-101: Consider parallelism based on available CPUs.
make -j2underutilizes runners that often have more cores. Prefer-j"$(nproc)"with a sensible cap.- make -j2 + make -j"$(nproc || echo 2)"
141-145: Quote $USER in chown to satisfy shellcheck and avoid word-splitting.Static analysis flagged SC2086. Quote variable expansions.
- sudo chown -R $USER ./scripts/wheel + sudo chown -R "$USER" ./scripts/wheel ... - sudo chown -R $USER ./scripts/wheel + sudo chown -R "$USER" ./scripts/wheelAlso applies to: 154-157
158-161: Prefer safer globbing with explicit directory and guard against unzip on multiple files.To appease shellcheck and avoid odd filenames, use
./-prefixed globs and loop.- unzip -l ./scripts/wheel/sherpa-onnx-core/dist/*.whl + for f in ./scripts/wheel/sherpa-onnx-core/dist/*.whl; do unzip -l "$f"; done echo "---" - unzip -l ./scripts/wheel/sherpa-onnx-bin/dist/*.whl + for f in ./scripts/wheel/sherpa-onnx-bin/dist/*.whl; do unzip -l "$f"; done
169-178: Drop sudo when patching wheels to avoid root-owned artifacts.You already fix ownership before; writing patched wheels as root can reintroduce ownership issues.
- sudo ./scripts/wheel/patch_wheel.py --in-dir ./wheelhouse --out-dir ./wheels + ./scripts/wheel/patch_wheel.py --in-dir ./wheelhouse --out-dir ./wheels
205-208: Run tests on more than one Python to catch ABI/env issues.Your test job validates CLI on only Python 3.10. Consider a small matrix (e.g., 3.8 and latest) to verify compatibility across the range, especially since CLI/bin wheels are
py3-none.- - name: Setup Python + - name: Setup Python uses: actions/setup-python@v5 with: - python-version: "3.10" + python-version: ${{ matrix.py }} + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest] + py: ["3.8", "3.10", "3.12"]
231-236: Quote command substitutions to address SC2046 and avoid word-splitting.Static analysis flagged SC2046. Assign to a var and quote its uses.
- ls -lh $(which sherpa-onnx) - file $(which sherpa-onnx) - readelf -d $(which sherpa-onnx) - - ldd $(which sherpa-onnx) + bin="$(command -v sherpa-onnx)" + ls -lh "$bin" + file "$bin" + readelf -d "$bin" + + ldd "$bin"
243-278: Consider gating HuggingFace publishing to manual runs or tags to avoid noisy pushes.Right now every push to branch “wheel” publishes to HF. If that’s intentional, ignore this. Otherwise, gate on workflow_dispatch or tags.
- - name: Publish to huggingface + - name: Publish to huggingface + if: ${{ github.event_name == 'workflow_dispatch' || startsWith(github.ref, 'refs/tags/') }}
299-301: cp314 may not be available in cibuildwheel image yet; add a guard or remove until released.Including
cp314will fail until Manylinux images and cibuildwheel support it. Consider dropping it for now or feature-gating via an input.- python-version: ["cp38", "cp39", "cp310", "cp311", "cp312", "cp313", "cp314"] + python-version: ["cp38", "cp39", "cp310", "cp311", "cp312", "cp313"]Alternatively:
if: ${{ always() && (matrix.python-version != 'cp314' || env.ALLOW_PY314 == '1') }}
333-341: Validate CIBW_ENVIRONMENT paths; avoid referring to non-existent build dirs.
LD_LIBRARY_PATH=/project/build/bdist.linux-x86_64/wheel/sherpa_onnx/liblikely doesn’t exist at build time and can mask loader errors. Prefer shipping needed libs withauditwheeland avoid settingLD_LIBRARY_PATHglobally in cibuildwheel.- LD_LIBRARY_PATH=/project/build/bdist.linux-x86_64/wheel/sherpa_onnx/lib:$SHERPA_ONNX_ALSA_LIB_DIR + # Avoid non-existent paths; rely on auditwheel to bundle needed libs + LD_LIBRARY_PATH=$SHERPA_ONNX_ALSA_LIB_DIR
347-362: Safer listing/extraction and handle “no match” globs.Use
./-prefixed globs; avoid bareunzipon multiple files; iterate to keep logs readable and avoid SC2035.- ls -lh wheelhouse/*.whl - unzip -l wheelhouse/*.whl + ls -lh ./wheelhouse/*.whl + for f in ./wheelhouse/*.whl; do unzip -l "$f"; done ... - cp wheelhouse/*.whl ./t + cp ./wheelhouse/*.whl ./t ... - unzip ./*.whl + for f in ./*.whl; do unzip "$f"; done ... - readelf -d *.so + find . -name '*.so' -print -exec readelf -d {} \;
400-410: Double-check that you only publish on tags or manual dispatch.Like the
sherpa-onnx-binpublication, consider guarding the Python API wheels publication to tags or workflow_dispatch to avoid accidental releases from branch pushes.- - name: Publish wheels to PyPI + - name: Publish wheels to PyPI + if: ${{ github.event_name == 'workflow_dispatch' || startsWith(github.ref, 'refs/tags/') }}
411-426: Build sdist withpython -m buildand upload once, outside the matrix.You currently build sdist in one matrix cell. It’s fine, but modern best practice is to use
python -m buildonce after wheels succeed. Optional suggestion.Example:
- - name: Build sdist - if: matrix.python-version == 'cp38' && matrix.manylinux == 'manylinux2014' + - name: Build sdist + if: ${{ matrix.python-version == 'cp38' && matrix.manylinux == 'manylinux2014' }} shell: bash run: | - python3 setup.py sdist + python3 -m pip install build + python3 -m build --sdist ls -l dist/*
| - name: Build sherpa-onnx | ||
| uses: addnab/docker-run-action@v3 | ||
| with: | ||
| image: quay.io/pypa/manylinux2014_x86_64 | ||
| options: | | ||
| --volume ${{ github.workspace }}/:/home/runner/work/sherpa-onnx/sherpa-onnx | ||
| shell: bash | ||
| run: | | ||
| uname -a | ||
| gcc --version | ||
| cmake --version | ||
| cat /etc/*release | ||
| id | ||
| pwd | ||
|
|
||
| CIBW_BUILD: "${{ matrix.python-version}}-* " | ||
| CIBW_SKIP: "cp27-* cp35-* cp36-* *-win32 pp* *-musllinux* *-manylinux_i686" | ||
| CIBW_BUILD_VERBOSITY: 3 | ||
| CIBW_MANYLINUX_X86_64_IMAGE: quay.io/pypa/${{ matrix.manylinux }}_x86_64 | ||
| cd /home/runner/work/sherpa-onnx/sherpa-onnx | ||
|
|
||
| find /opt -name "python*" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the supported /opt/python/cp310-*/bin path instead of the private /opt/_internal path.
Relying on /opt/_internal/cpython-3.10.18/bin is brittle and can break when the manylinux image updates. The manylinux images guarantee /opt/python/cp310-*/bin. Select it dynamically.
- export PATH=/opt/_internal/cpython-3.10.18/bin:$PATH
- which python3
- python3 --version
-
- python3 -m venv my
+ PYBIN=$(ls -d /opt/python/cp310-*/bin | head -n1)
+ export PATH="$PYBIN:$PATH"
+ which python3 || true
+ python3 --version
+
+ python3 -m venv myIf you want to be future-proof, consider parameterizing the CPython minor version or detecting it based on a matrix input.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Build sherpa-onnx | |
| uses: addnab/docker-run-action@v3 | |
| with: | |
| image: quay.io/pypa/manylinux2014_x86_64 | |
| options: | | |
| --volume ${{ github.workspace }}/:/home/runner/work/sherpa-onnx/sherpa-onnx | |
| shell: bash | |
| run: | | |
| uname -a | |
| gcc --version | |
| cmake --version | |
| cat /etc/*release | |
| id | |
| pwd | |
| CIBW_BUILD: "${{ matrix.python-version}}-* " | |
| CIBW_SKIP: "cp27-* cp35-* cp36-* *-win32 pp* *-musllinux* *-manylinux_i686" | |
| CIBW_BUILD_VERBOSITY: 3 | |
| CIBW_MANYLINUX_X86_64_IMAGE: quay.io/pypa/${{ matrix.manylinux }}_x86_64 | |
| cd /home/runner/work/sherpa-onnx/sherpa-onnx | |
| find /opt -name "python*" | |
| - name: Build sherpa-onnx | |
| uses: addnab/docker-run-action@v3 | |
| with: | |
| image: quay.io/pypa/manylinux2014_x86_64 | |
| options: | | |
| --volume ${{ github.workspace }}/:/home/runner/work/sherpa-onnx/sherpa-onnx | |
| shell: bash | |
| run: | | |
| uname -a | |
| gcc --version | |
| cmake --version | |
| cat /etc/*release | |
| id | |
| pwd | |
| cd /home/runner/work/sherpa-onnx/sherpa-onnx | |
| find /opt -name "python*" | |
| # Dynamically pick the manylinux-provided Python 3.10 interpreter | |
| PYBIN=$(ls -d /opt/python/cp310-*/bin | head -n1) | |
| export PATH="$PYBIN:$PATH" | |
| which python3 || true | |
| python3 --version | |
| python3 -m venv my |
| git clone --depth 1 --branch v1.2.12 https://github.com/alsa-project/alsa-lib | ||
| pushd alsa-lib | ||
| ./gitcompile | ||
| popd | ||
|
|
||
| export CPLUS_INCLUDE_PATH=$PWD/alsa-lib/include:$CPLUS_INCLUDE_PATH | ||
| export SHERPA_ONNX_ALSA_LIB_DIR=$PWD/alsa-lib/src/.libs | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm ALSA license handling and keep LICENSE with redistributed binaries.
You vendor and ship alsa-lib artifacts. LGPL requires including the license text and offering relinking. At minimum, copy alsa-lib’s COPYING into the wheel and reference it in METADATA.
export CPLUS_INCLUDE_PATH=$PWD/alsa-lib/include:$CPLUS_INCLUDE_PATH
export SHERPA_ONNX_ALSA_LIB_DIR=$PWD/alsa-lib/src/.libs
+
+# Place third-party licenses into the wheel tree (will be included below)
+ mkdir -p ./third_party_licenses
+ cp -v ./alsa-lib/COPYING ./third_party_licenses/alsa-lib.LICENSEAnd before building each wheel:
echo 'sherpa-onnx-core'
mkdir -p ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/lib
cp -v ./install/lib/lib*.so ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/lib
+ mkdir -p ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/licenses
+ cp -v ./third_party_licenses/* ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/licenses/
mkdir -p ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/include/sherpa-onnx/c-api
cp -v ./install/include/sherpa-onnx/c-api/*.h ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/include/sherpa-onnx/c-apiHappy to follow up with a minimal setup.cfg/MANIFEST.in tweak so these are included in dist-info and wheel metadata.
🏁 Script executed:
#!/bin/bash
set -e
# Locate the build-wheels-linux.yaml workflow file
wf=$(find .github/workflows -type f -name "build-wheels-linux.yaml" | head -n1)
echo "Workflow file: $wf"
# Print lines 70-100 to confirm absence of license-handling steps
sed -n '70,100p' "$wf"Length of output: 1221
Include ALSA’s LGPL License Text in Distributed Wheels
The current build workflow compiles and vendors alsa-lib but does not package its COPYING (LGPL) file alongside the binaries. Per LGPL requirements, any redistributed alsa-lib binaries must include the full license text and offer relinking. Please:
• In .github/workflows/build-wheels-linux.yaml (after cloning and building alsa-lib) add steps to copy alsa-lib’s COPYING into a third-party licenses directory.
• In the wheel-assembly steps (under scripts/wheel/sherpa-onnx-core/…) include that directory in the wheel, e.g. sherpa_onnx/licenses/.
• Update your packaging config (setup.cfg / MANIFEST.in) to ensure these license files are bundled in the final .dist-info and wheel data.
Pinpoint locations:
- .github/workflows/build-wheels-linux.yaml (around lines 79–86)
- scripts/wheel/sherpa-onnx-core/sherpa_onnx/lib and …/licenses
Suggested diffs:
--- a/.github/workflows/build-wheels-linux.yaml
+++ b/.github/workflows/build-wheels-linux.yaml
@@ -83,6 +83,10 @@
export SHERPA_ONNX_ALSA_LIB_DIR=$PWD/alsa-lib/src/.libs
+ # Copy ALSA’s LGPL license for inclusion in wheels
+ mkdir -p third_party_licenses
+ cp -v alsa-lib/COPYING third_party_licenses/alsa-lib.LICENSE
+
mkdir build
pushd build--- a/scripts/wheel/assemble-sherpa-onnx-core.sh
+++ b/scripts/wheel/assemble-sherpa-onnx-core.sh
@@ -12,6 +12,9 @@
cp -v ./install/lib/lib*.so ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/lib
+mkdir -p ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/licenses
+cp -v ../../../../third_party_licenses/* ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/licenses/
+
mkdir -p ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/include/sherpa-onnx/c-api
cp -v ./install/include/sherpa-onnx/c-api/*.h ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/include/sherpa-onnx/c-apiFeel free to reach out if you need help adjusting your setup.cfg/MANIFEST.in to ensure these files end up in the wheel metadata.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| git clone --depth 1 --branch v1.2.12 https://github.com/alsa-project/alsa-lib | |
| pushd alsa-lib | |
| ./gitcompile | |
| popd | |
| export CPLUS_INCLUDE_PATH=$PWD/alsa-lib/include:$CPLUS_INCLUDE_PATH | |
| export SHERPA_ONNX_ALSA_LIB_DIR=$PWD/alsa-lib/src/.libs | |
| # .github/workflows/build-wheels-linux.yaml (around lines 79–86) | |
| git clone --depth 1 --branch v1.2.12 https://github.com/alsa-project/alsa-lib | |
| pushd alsa-lib | |
| ./gitcompile | |
| popd | |
| export CPLUS_INCLUDE_PATH=$PWD/alsa-lib/include:$CPLUS_INCLUDE_PATH | |
| export SHERPA_ONNX_ALSA_LIB_DIR=$PWD/alsa-lib/src/.libs | |
| # Copy ALSA’s LGPL license for inclusion in wheels | |
| mkdir -p third_party_licenses | |
| cp -v alsa-lib/COPYING third_party_licenses/alsa-lib.LICENSE | |
| mkdir build | |
| pushd build |
| git clone --depth 1 --branch v1.2.12 https://github.com/alsa-project/alsa-lib | |
| pushd alsa-lib | |
| ./gitcompile | |
| popd | |
| export CPLUS_INCLUDE_PATH=$PWD/alsa-lib/include:$CPLUS_INCLUDE_PATH | |
| export SHERPA_ONNX_ALSA_LIB_DIR=$PWD/alsa-lib/src/.libs | |
| # scripts/wheel/assemble-sherpa-onnx-core.sh (around line 12) | |
| cp -v ./install/lib/lib*.so ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/lib | |
| # Include the LGPL license text alongside the vendored libs | |
| mkdir -p ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/licenses | |
| cp -v ../../../../third_party_licenses/* ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/licenses/ | |
| mkdir -p ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/include/sherpa-onnx/c-api | |
| cp -v ./install/include/sherpa-onnx/c-api/*.h ../scripts/wheel/sherpa-onnx-core/sherpa_onnx/include/sherpa-onnx/c-api |
🤖 Prompt for AI Agents
.github/workflows/build-wheels-linux.yaml around lines 79–86: after cloning and
building alsa-lib, add a step to copy alsa-lib's COPYING file into the
repository's third-party license directory (e.g.
scripts/wheel/sherpa-onnx-core/sherpa_onnx/licenses/alsa-lib/COPYING) and ensure
the directory exists; then update the wheel assembly steps
(scripts/wheel/sherpa-onnx-core/*) to include that licenses directory into the
package data (e.g. copy or tar into the wheel build staging area under
sherpa_onnx/licenses/); finally update packaging config (MANIFEST.in and/or
setup.cfg) to include sherpa_onnx/licenses/** and ensure the COPYING file is
included in the final .dist-info/wheel data so the ALSA LGPL text is distributed
with the wheel.
| - name: Publish wheels to PyPI ${{ github.event.inputs.publish_sherpa_onnx_bin }} | ||
| if: ${{ (github.event.inputs.publish_sherpa_onnx_bin || 'true') == 'true' }} | ||
| env: | ||
| TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} | ||
| TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} | ||
| shell: bash | ||
| run: | | ||
| python3 -m pip install --upgrade pip | ||
| python3 -m pip install wheel twine==5.0.0 setuptools | ||
|
|
||
| twine upload /tmp/wheels/*.whl |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden the PyPI publish guard for sherpa-onnx-bin; prevent accidental publish on push.
The current condition coerces to 'true' for non-dispatch events due to the || 'true', which will publish on push. Parse the boolean and require workflow_dispatch explicitly.
- - name: Publish wheels to PyPI ${{ github.event.inputs.publish_sherpa_onnx_bin }}
- if: ${{ (github.event.inputs.publish_sherpa_onnx_bin || 'true') == 'true' }}
+ - name: Publish wheels to PyPI ${{ github.event.inputs.publish_sherpa_onnx_bin }}
+ if: ${{ github.event_name == 'workflow_dispatch' && fromJSON(github.event.inputs.publish_sherpa_onnx_bin || 'false') }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Publish wheels to PyPI ${{ github.event.inputs.publish_sherpa_onnx_bin }} | |
| if: ${{ (github.event.inputs.publish_sherpa_onnx_bin || 'true') == 'true' }} | |
| env: | |
| TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} | |
| TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} | |
| shell: bash | |
| run: | | |
| python3 -m pip install --upgrade pip | |
| python3 -m pip install wheel twine==5.0.0 setuptools | |
| twine upload /tmp/wheels/*.whl | |
| - name: Publish wheels to PyPI ${{ github.event.inputs.publish_sherpa_onnx_bin }} | |
| if: ${{ github.event_name == 'workflow_dispatch' && fromJSON(github.event.inputs.publish_sherpa_onnx_bin || 'false') }} | |
| env: | |
| TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} | |
| TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} | |
| shell: bash | |
| run: | | |
| python3 -m pip install --upgrade pip | |
| python3 -m pip install wheel twine==5.0.0 setuptools | |
| twine upload /tmp/wheels/*.whl |
🤖 Prompt for AI Agents
In .github/workflows/build-wheels-linux.yaml around lines 279-289, the step's if
condition currently falls back to 'true' for non-dispatch events and can publish
on pushes; change the guard to require a workflow_dispatch event and an explicit
true input. Remove the "|| 'true'" coercion and replace the condition with a
check that github.event_name == 'workflow_dispatch' &&
github.event.inputs.publish_sherpa_onnx_bin == 'true' (or equivalent expression)
so the step only runs when manually dispatched with the publish flag set.
| opts='--break-system-packages' | ||
|
|
||
| python3 -m pip install $opts --upgrade pip | ||
| python3 -m pip install $opts wheel twine==5.0.0 setuptools | ||
|
|
||
| twine upload /tmp/wheels/*.whl | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove Debian-specific pip flag on macOS and upgrade pip first
Prevents early failures and keeps steps consistent.
- opts='--break-system-packages'
-
- python3 -m pip install $opts --upgrade pip
- python3 -m pip install $opts wheel twine==5.0.0 setuptools
+ python3 -m pip install --upgrade pip
+ python3 -m pip install wheel twine==5.0.0 setuptools📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| opts='--break-system-packages' | |
| python3 -m pip install $opts --upgrade pip | |
| python3 -m pip install $opts wheel twine==5.0.0 setuptools | |
| twine upload /tmp/wheels/*.whl | |
| python3 -m pip install --upgrade pip | |
| python3 -m pip install wheel twine==5.0.0 setuptools | |
| twine upload /tmp/wheels/*.whl |
🤖 Prompt for AI Agents
In .github/workflows/build-wheels-macos-arm64.yaml around lines 245 to 251,
remove the Debian-specific pip flag (--break-system-packages) and the opts
variable usage; instead run python3 -m pip install --upgrade pip first, then run
python3 -m pip install wheel twine==5.0.0 setuptools (without the Debian flag),
and finally run twine upload /tmp/wheels/*.whl so pip is upgraded before package
installs and no Linux-only flags are used on macOS.
| opts='--break-system-packages' | ||
|
|
||
| python3 -m pip install $opts --upgrade pip | ||
| python3 -m pip install $opts wheel twine==5.0.0 setuptools | ||
|
|
||
| twine upload /tmp/wheels/*.whl | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary Debian-only pip flag on macOS
--break-system-packages isn’t needed on macOS and can cause failures before pip is upgraded. Drop it here.
- opts='--break-system-packages'
-
- python3 -m pip install $opts --upgrade pip
- python3 -m pip install $opts wheel twine==5.0.0 setuptools
+ python3 -m pip install --upgrade pip
+ python3 -m pip install wheel twine==5.0.0 setuptools📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| opts='--break-system-packages' | |
| python3 -m pip install $opts --upgrade pip | |
| python3 -m pip install $opts wheel twine==5.0.0 setuptools | |
| twine upload /tmp/wheels/*.whl | |
| python3 -m pip install --upgrade pip | |
| python3 -m pip install wheel twine==5.0.0 setuptools | |
| twine upload /tmp/wheels/*.whl |
🤖 Prompt for AI Agents
.github/workflows/build-wheels-macos-universal2.yaml around lines 246 to 252:
the workflow passes the Debian-specific pip flag --break-system-packages on
macOS which is unnecessary and can cause failures; remove the opts variable
usage for macOS steps and call pip install and pip install
wheel/twine/setuptools without the --break-system-packages flag so pip is
upgraded and packages are installed normally on macOS.
| set(pybind11_URL "https://github.com/pybind/pybind11/archive/refs/tags/v3.0.0.tar.gz") | ||
| set(pybind11_URL2 "https://hf-mirror.com/csukuangfj/sherpa-onnx-cmake-deps/resolve/main/pybind11-3.0.0.tar.gz") | ||
| set(pybind11_HASH "SHA256=453b1a3e2b266c3ae9da872411cadb6d693ac18063bd73226d96cfb7015a200c") | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
pybind11 upgraded to v3.0.0 — confirm C++ standard and transitive impacts
pybind11 v3 requires C++17 and has a few subtle changes vs 2.x. Please confirm the project enforces C++17 (or newer) and that all bindings still compile across your supported platforms.
You can quickly sanity-check C++ standard settings with:
🏁 Script executed:
#!/bin/bash
# Look for C++ standard settings and pybind11 usage
rg -nP -C2 'CMAKE_CXX_STANDARD\s*[:=]\s*(\d+)|set\s*\(\s*CMAKE_CXX_STANDARD\s+(\d+)' cmake
rg -nP 'add_compile_options|target_compile_features' cmake
rg -nP 'pybind11' -C2Length of output: 7694
🏁 Script executed:
#!/bin/bash
# Search entire repository for C++ standard enforcement and compile features/options
rg -nP -C2 'CMAKE_CXX_STANDARD\s*[:=]\s*(\d+)' .
rg -nP 'target_compile_features' .
rg -nP 'add_compile_options' .Length of output: 706
Enforce C++17 in the top‐level CMakeLists for pybind11 v3.0.0
I did not find any global CMAKE_CXX_STANDARD setting in your root CMakeLists.txt, nor a blanket target_compile_features(... PUBLIC cxx_std_17) outside of example subprojects. Since pybind11 v3.x requires at least C++17, you should enforce it project-wide to avoid build failures on platforms where the compiler defaults to an older standard.
• In CMakeLists.txt (project root), add near the top:
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)• Alternatively (or in addition), use:
add_compile_features(cxx_std_17)• Verify that all targets—including the pybind11 binding in python/csrc—inherit this requirement and still compile cleanly on your supported platforms.
• Update any CI configurations or build scripts that may override the C++ standard.
This ensures consistency and prevents subtle compilation errors after upgrading to pybind11 3.0.0.
🤖 Prompt for AI Agents
In cmake/pybind11.cmake around lines 4-7: pybind11 v3.0.0 requires C++17 but the
project has no global CMAKE_CXX_STANDARD set; update the top-level
CMakeLists.txt to set CMAKE_CXX_STANDARD 17, set CMAKE_CXX_STANDARD_REQUIRED ON
and set CMAKE_CXX_EXTENSIONS OFF (or call add_compile_features(cxx_std_17)) so
all targets default to C++17, and ensure any third-party or python/csrc targets
explicitly inherit this requirement (use target_compile_features(... PUBLIC
cxx_std_17) for targets that need it); also update CI/build scripts that might
override the standard.
| linux_x64_wheel_filename=sherpa_onnx_core-${SHERPA_ONNX_VERSION}-py3-none-manylinux2014_x86_64.whl | ||
| linux_x64_wheel=$src_dir/$linux_x64_wheel_filename | ||
|
|
||
| linux_arm64_wheel_filename=sherpa_onnx-${SHERPA_ONNX_VERSION}-cp38-cp38-manylinux2014_aarch64.manylinux_2_17_aarch64.whl | ||
| linux_arm64_wheel_filename=sherpa_onnx_core-${SHERPA_ONNX_VERSION}-py3-none-manylinux2014_aarch64.whl | ||
| linux_arm64_wheel=$src_dir/$linux_arm64_wheel_filename | ||
|
|
||
| macos_x64_wheel_filename=sherpa_onnx-${SHERPA_ONNX_VERSION}-cp39-cp39-macosx_11_0_x86_64.whl | ||
| macos_x64_wheel_filename=sherpa_onnx_core-${SHERPA_ONNX_VERSION}-py3-none-macosx_10_15_x86_64.whl | ||
| macos_x64_wheel=$src_dir/$macos_x64_wheel_filename | ||
|
|
||
| macos_arm64_wheel_filename=sherpa_onnx-${SHERPA_ONNX_VERSION}-cp39-cp39-macosx_11_0_arm64.whl | ||
| macos_arm64_wheel_filename=sherpa_onnx_core-${SHERPA_ONNX_VERSION}-py3-none-macosx_11_0_arm64.whl | ||
| macos_arm64_wheel=$src_dir/$macos_arm64_wheel_filename | ||
|
|
||
| windows_x64_wheel_filename=sherpa_onnx-${SHERPA_ONNX_VERSION}-cp38-cp38-win_amd64.whl | ||
| windows_x64_wheel_filename=sherpa_onnx_core-${SHERPA_ONNX_VERSION}-py3-none-win_amd64.whl |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Wheel filename switches look consistent; please verify tags exist for the published artifacts.
The move to sherpa_onnx_core with py3-none tags across platforms is good and matches the split-packaging intent. However, macOS tags (macosx_10_15_x86_64 and macosx_11_0_arm64) and manylinux2014 tags can easily diverge from what cibuildwheel actually emits.
Run this quick check to confirm the filenames exist for SHERPA_ONNX_VERSION on Hugging Face before we merge:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
ver=$(grep "SHERPA_ONNX_VERSION" "$(cd "$(dirname "$0")"/../.. && pwd)/CMakeLists.txt" | awk '{print $2}' | tr -d '"')
base="https://hf.co/csukuangfj/sherpa-onnx-wheels/resolve/main/cpu/$ver"
for f in \
"sherpa_onnx_core-${ver}-py3-none-manylinux2014_x86_64.whl" \
"sherpa_onnx_core-${ver}-py3-none-manylinux2014_aarch64.whl" \
"sherpa_onnx_core-${ver}-py3-none-macosx_10_15_x86_64.whl" \
"sherpa_onnx_core-${ver}-py3-none-macosx_11_0_arm64.whl" \
"sherpa_onnx_core-${ver}-py3-none-win_amd64.whl"
do
echo "HEAD $base/$f"
curl -sI "$base/$f" | sed -n '1p; /^content-length:/Ip' || true
doneLength of output: 254
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Locate repository root
root=$(git rev-parse --show-toplevel)
cmake="$root/CMakeLists.txt"
if [[ ! -f "$cmake" ]]; then
echo "Error: CMakeLists.txt not found at $cmake" >&2
exit 1
fi
# Extract SHERPA_ONNX_VERSION from CMakeLists.txt
ver=$(grep "SHERPA_ONNX_VERSION" "$cmake" | awk '{print $2}' | tr -d '"')
echo "Detected SHERPA_ONNX_VERSION: $ver" >&2
# Base URL for wheels on Hugging Face
base="https://hf.co/csukuangfj/sherpa-onnx-wheels/resolve/main/cpu/$ver"
# List of expected wheel filenames
files=(
"sherpa_onnx_core-${ver}-py3-none-manylinux2014_x86_64.whl"
"sherpa_onnx_core-${ver}-py3-none-manylinux2014_aarch64.whl"
"sherpa_onnx_core-${ver}-py3-none-macosx_10_15_x86_64.whl"
"sherpa_onnx_core-${ver}-py3-none-macosx_11_0_arm64.whl"
"sherpa_onnx_core-${ver}-py3-none-win_amd64.whl"
)
# Verify each wheel exists by fetching headers
for f in "${files[@]}"; do
echo "HEAD $base/$f"
curl -sI -X HEAD "$base/$f" \
| sed -n '1p; /^content-length:/Ip' \
|| echo "✗ $f not found"
doneLength of output: 3389
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Determine repository root and CMakeLists.txt path
root=$(git rev-parse --show-toplevel)
cmake="$root/CMakeLists.txt"
if [[ ! -f "$cmake" ]]; then
echo "Error: CMakeLists.txt not found at $cmake" >&2
exit 1
fi
# Extract only the numeric version (e.g., 1.12.9) from SHERPA_ONNX_VERSION
ver=$(sed -n -E 's/.*SHERPA_ONNX_VERSION.*"([0-9]+(\.[0-9]+)*)".*/\1/p' "$cmake")
echo "Detected SHERPA_ONNX_VERSION: $ver" >&2
# Base URL for the Hugging Face CPU wheels repository
base="https://hf.co/csukuangfj/sherpa-onnx-wheels/resolve/main/cpu/$ver"
# Expected wheel filenames
files=(
"sherpa_onnx_core-${ver}-py3-none-manylinux2014_x86_64.whl"
"sherpa_onnx_core-${ver}-py3-none-manylinux2014_aarch64.whl"
"sherpa_onnx_core-${ver}-py3-none-macosx_10_15_x86_64.whl"
"sherpa_onnx_core-${ver}-py3-none-macosx_11_0_arm64.whl"
"sherpa_onnx_core-${ver}-py3-none-win_amd64.whl"
)
# Verify each wheel by following redirects and checking for HTTP 200
for f in "${files[@]}"; do
url="$base/$f"
status=$(curl -s -o /dev/null -w '%{http_code}' -L "$url")
if [[ "$status" -eq 200 ]]; then
echo "✓ Found: $f (HTTP $status)"
else
echo "✗ Missing: $f (HTTP $status)"
fi
doneLength of output: 3331
Wheel artifacts missing on Hugging Face – blocking
The verification script reports HTTP 404 for all five expected wheels for SHERPA_ONNX_VERSION=1.12.9, meaning none of the following files are present under csukuangfj/sherpa-onnx-wheels/resolve/main/cpu/1.12.9:
• sherpa_onnx_core-1.12.9-py3-none-manylinux2014_x86_64.whl
• sherpa_onnx_core-1.12.9-py3-none-manylinux2014_aarch64.whl
• sherpa_onnx_core-1.12.9-py3-none-macosx_10_15_x86_64.whl
• sherpa_onnx_core-1.12.9-py3-none-macosx_11_0_arm64.whl
• sherpa_onnx_core-1.12.9-py3-none-win_amd64.whl
Location: scripts/dotnet/run.sh (lines 29–41)
Please ensure the wheels are published with exactly these filenames (or update SHERPA_ONNX_VERSION/tags to match the uploaded artifacts) before merging.
🤖 Prompt for AI Agents
In scripts/dotnet/run.sh around lines 29–41 the verification expects five Sherpa
ONNX wheel filenames for SHERPA_ONNX_VERSION=1.12.9 but they return 404; either
publish the missing wheel artifacts to
csukuangfj/sherpa-onnx-wheels/resolve/main/cpu/1.12.9 using the exact filenames
listed in the comment (manylinux2014_x86_64, manylinux2014_aarch64,
macosx_10_15_x86_64, macosx_11_0_arm64, win_amd64) or update
SHERPA_ONNX_VERSION/tags and any path logic in the script to match the actual
uploaded artifact names/versions, then re-run the verification to confirm HTTP
200 for all five files.
| elif "cp312" in str(whl): | ||
| py_version = "3.12" | ||
| elif "cp313" in str(whl): | ||
| py_version = "3.13" | ||
| elif "py3-none": | ||
| py_version = None | ||
| else: | ||
| assert False, f"Unknow python version in {whl}" | ||
|
|
There was a problem hiding this comment.
Bug: unconditional truthy branch for "py3-none" and typo in error path
elif "py3-none": is always true (non-empty string), so unknown tags will be misclassified as py3-none. Also, assert False is optimized out with -O and the message contains a typo.
Apply:
@@
- elif "cp312" in str(whl):
+ elif "cp312" in str(whl):
py_version = "3.12"
- elif "cp313" in str(whl):
+ elif "cp313" in str(whl):
py_version = "3.13"
- elif "py3-none":
- py_version = None
- else:
- assert False, f"Unknow python version in {whl}"
+ elif "cp314" in str(whl):
+ py_version = "3.14"
+ elif "py3-none" in str(whl):
+ py_version = None
+ else:
+ raise AssertionError(f"Unknown Python version tag in {whl.name}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif "cp312" in str(whl): | |
| py_version = "3.12" | |
| elif "cp313" in str(whl): | |
| py_version = "3.13" | |
| elif "py3-none": | |
| py_version = None | |
| else: | |
| assert False, f"Unknow python version in {whl}" | |
| elif "cp312" in str(whl): | |
| py_version = "3.12" | |
| elif "cp313" in str(whl): | |
| py_version = "3.13" | |
| elif "cp314" in str(whl): | |
| py_version = "3.14" | |
| elif "py3-none" in str(whl): | |
| py_version = None | |
| else: | |
| raise AssertionError(f"Unknown Python version tag in {whl.name}") |
🧰 Tools
🪛 Ruff (0.12.2)
50-50: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🤖 Prompt for AI Agents
In scripts/wheel/patch_wheel.py around lines 43 to 51, the conditional `elif
"py3-none":` is always truthy and should check whether "py3-none" is contained
in str(whl); change it to `elif "py3-none" in str(whl):` (or equivalent), and
replace the `assert False, f"Unknow python version in {whl}"` with a
non-optimizable exception (e.g., `raise RuntimeError(f"Unknown python version in
{whl}")`) so unknown tags are caught reliably and the error message typo is
fixed.
| setuptools.setup( | ||
| name=package_name, | ||
| python_requires=">=3.6", | ||
| python_requires=">=3.7", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Python version requirement increase.
The Python minimum version has been increased from 3.6 to 3.7. Ensure this change is intentional and documented, as it may affect users on older Python versions.
🏁 Script executed:
#!/bin/bash
# Check if Python 3.7 requirement is documented and consistent across the project
echo "Checking Python version requirements across the project:"
rg -n "python_requires.*3\.[67]" --type=py
rg -n "Programming Language :: Python :: 3\.[67]" --type=py
echo ""
echo "Checking if this is documented in README or changelog:"
rg -n -i "python.*3\.[67]|minimum.*python" README.md CHANGELOG.md || echo "No documentation found"Length of output: 530
Ensure Python ≥3.7 is fully reflected in packaging and docs
I’ve confirmed that you’ve bumped python_requires to >=3.7 in setup.py (line 72) and that CHANGELOG.md notes a “Fix building wheels for Python 3.7” —but:
- There is no Trove classifier for Python 3.7 (nor removal of the older 3.6 classifier) in
setup.py. - The README.md (and any user‐facing docs) haven’t been updated to call out Python 3.7 as the new minimum.
Please add the following mandatory updates:
• setup.py
– Remove the old Programming Language :: Python :: 3.6 classifier
– Add at least one Programming Language :: Python :: 3.7 (and, if desired, 3.8, 3.9, etc.) entry
classifiers=[
- "Programming Language :: Python :: 3.6",
+ "Programming Language :: Python :: 3.7",
"Programming Language :: Python :: 3 :: Only",
…
]• README.md
– In the Installation or Prerequisites section, update any “Python 3.6+” or generic Python references to explicitly require Python 3.7 or higher.
This will ensure that both package metadata and user documentation are consistent with the new minimum version requirement.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| python_requires=">=3.7", | |
| python_requires=">=3.7", | |
| classifiers=[ | |
| "Programming Language :: Python :: 3.7", | |
| "Programming Language :: Python :: 3 :: Only", | |
| … | |
| ], |
🤖 Prompt for AI Agents
In setup.py around line 72, the package metadata was updated to
python_requires=">=3.7" but the Trove classifiers still list Python 3.6 and lack
a Python 3.7 classifier; remove the "Programming Language :: Python :: 3.6"
classifier and add "Programming Language :: Python :: 3.7" (optionally add
3.8/3.9/etc. as needed). Also update README.md (Installation/Prerequisites) to
replace any "Python 3.6" or "Python 3.6+" references with "Python 3.7+" (or "3.7
or higher") so docs match the new minimum, ensuring both package metadata and
user-facing documentation consistently reflect Python ≥3.7.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
.github/workflows/test-pip-install.yaml (7)
39-45: Gate Python 3.13 to avoid red CI if wheels lag; make it “experimental”.3.13 coverage is great, but binary wheels across all OS/arch can lag. Mark 3.13 as experimental and continue-on-error to keep nightly green while still signaling breakages.
Apply at the job level (outside this hunk):
jobs: test_pip_install: + continue-on-error: ${{ contains(matrix.python-version, '3.13') || matrix.experimental == true }} runs-on: ${{ matrix.os }}And tag the 3.13 entries as experimental within the matrix:
- - os: ubuntu-latest - python-version: "3.13" + - os: ubuntu-latest + python-version: "3.13" + experimental: trueRepeat similarly for macOS and Windows 3.13 rows if desired. As a follow-up, please confirm 3.13 wheels exist for all three packages on all targeted platforms.
104-108: Install via “python -m pip” and force binary-only to catch missing wheels early.Using python -m pip ensures the right interpreter is used on all OS. For binary-heavy packages, disallow source builds so CI fails fast if a wheel is missing, which is what we want this job to detect.
- pip install --verbose -U sherpa-onnx sherpa-onnx-core sherpa-onnx-bin + python -m pip install -U pip wheel + export PIP_ONLY_BINARY=":all:" + # Optional: show environment for easier triage + python -c "import sys,platform; print(sys.version); print(platform.platform())" + python -m pip install --verbose -U sherpa-onnx sherpa-onnx-core sherpa-onnx-binOptionally print the resolved versions for debugging:
+ python -m pip show sherpa-onnx || true + python -m pip show sherpa-onnx-core || true + python -m pip show sherpa-onnx-bin || truePlease also confirm that “sherpa-onnx” depends on “sherpa-onnx-core” so we aren’t masking a dependency leak by installing all three at once.
109-126: Harden CLI tests for cross-OS PATH differences and ensure commands are discoverable.On Windows, PATH/script shims can be tricky under bash. Add a discovery step and fall back to python -m entry points if needed.
- name: Test sherpa-onnx-bin shell: bash run: | - sherpa-onnx-version + # Verify scripts are on PATH across OS + command -v sherpa-onnx-version || (echo "sherpa-onnx-version not on PATH" && exit 1) + sherpa-onnx-versionOptional: assert the binary returns a semantic version to catch stub issues:
+ sherpa-onnx-version | grep -E '^[0-9]+\.[0-9]+(\.[0-9]+)?' >/dev/nullNice-to-have: add a quick “--help | head -n 1” check to make logs compact while still exercising entrypoints.
127-138: Make core tests portable: use “python -m” and gate UNIX-specific flags on Windows.The “python3” alias isn’t guaranteed on Windows runners, and flags like “--*-only-L” are UNIX-centric.
- python3 -m sherpa_onnx --cflags - python3 -m sherpa_onnx --c-api-libs - python3 -m sherpa_onnx --c-api-libs-only-L - python3 -m sherpa_onnx --c-api-libs-only-l + python -m sherpa_onnx --cflags + python -m sherpa_onnx --c-api-libs + if [ "${{ runner.os }}" != "Windows" ]; then + python -m sherpa_onnx --c-api-libs-only-L + python -m sherpa_onnx --c-api-libs-only-l + fi- python3 -m sherpa_onnx --cxx-api-libs - python3 -m sherpa_onnx --cxx-api-libs-only-L - python3 -m sherpa_onnx --cxx-api-libs-only-l + python -m sherpa_onnx --cxx-api-libs + if [ "${{ runner.os }}" != "Windows" ]; then + python -m sherpa_onnx --cxx-api-libs-only-L + python -m sherpa_onnx --cxx-api-libs-only-l + fiIf there are Windows-specific equivalents (e.g., MSVC lib hints), consider adding them in a mirrored conditional block. I can add those once the CLI options are finalized.
139-145: Use “python -c” for portability and add a minimal import/use assertion.Small nits: prefer python over python3 for Windows, and ensure the attributes exist rather than printing objects.
- python3 -c "import sherpa_onnx; print(sherpa_onnx.__file__)" - python3 -c "import sherpa_onnx; print(sherpa_onnx.__version__)" - python3 -c "import sherpa_onnx; print(sherpa_onnx.OnlineRecognizer)" - python3 -c "import sherpa_onnx; print(sherpa_onnx.OfflineRecognizer)" + python -c "import sherpa_onnx, sys; print(sherpa_onnx.__file__); print(sherpa_onnx.__version__); sys.exit(0)" + python -c "import sherpa_onnx, sys; assert hasattr(sherpa_onnx, 'OnlineRecognizer'); assert hasattr(sherpa_onnx, 'OfflineRecognizer'); print('recognizers: ok')"
93-98: Consider skipping “./new-release.sh” in this test job.Running release tooling in a pip-install smoke test is unexpected and may create noisy diffs or side effects. If needed for version stamping, gate it behind a branch or input.
- - name: Update version + - name: Update version + if: startsWith(github.ref, 'refs/heads/release') || github.event_name == 'workflow_dispatch' shell: bash run: | ./new-release.sh git diff .
88-98: Speed up with pip caching (optional).Nightly matrix is large. A simple cache around the pip download dir significantly reduces runtime and PyPI load.
- uses: actions/checkout@v4 with: fetch-depth: 0 + + - name: Cache pip + uses: actions/cache@v4 + with: + path: ~/.cache/pip + key: ${{ runner.os }}-pip-${{ matrix.python-version }}-${{ hashFiles('**/requirements*.txt') }} + restore-keys: | + ${{ runner.os }}-pip-${{ matrix.python-version }}-If you don’t have pinned requirements files, you can key on the workflow file hash instead.
📜 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)
.github/workflows/test-pip-install.yaml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Release static tts-OFF
- GitHub Check: Release shared tts-ON
- GitHub Check: macos-latest Release static tts-OFF
- GitHub Check: ubuntu-latest Release static tts-ON
- GitHub Check: ubuntu-latest Release shared tts-ON
- GitHub Check: swift (macos-13)
- GitHub Check: Release shared-OFF tts-OFF
- GitHub Check: Debug shared-ON tts-ON
- GitHub Check: Debug shared-OFF tts-ON
- GitHub Check: Debug shared-OFF tts-OFF
- GitHub Check: Release shared-OFF tts-ON
- GitHub Check: Release shared-ON tts-ON
- GitHub Check: Debug shared-ON tts-OFF
- GitHub Check: Release shared-ON tts-OFF
- GitHub Check: rknn shared ON
- GitHub Check: ubuntu-24.04 3.13
- GitHub Check: ubuntu-24.04 3.12
- GitHub Check: ubuntu-24.04 3.10
- GitHub Check: ubuntu-24.04 3.8
- GitHub Check: ubuntu-24.04 3.11
🔇 Additional comments (2)
.github/workflows/test-pip-install.yaml (2)
1-88: Nice CI re-org and coverage expansion.Splitting tests to exercise bin, core, and Python entrypoints meaningfully validates the new packaging layout. Concurrency and nightly scheduling look good.
46-57: No changes needed to runner labelThe workflow correctly uses the
ubuntu-24.04-armlabel for ARM64 runners. According to the official GitHub-hosted runners documentation, the public preview ARM64 labels remainubuntu-24.04-arm(notubuntu-24.04-arm64). (docs.github.com)Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/wheel/patch_wheel.py (1)
76-82: Guard against empty existing RPATH to avoid introducing “:” (which can imply current directory).If
existing_rpathis empty,rpaths + ":" + existing_rpathleaves a trailing:, which some loaders interpret as the current directory. Join conditionally.- target_rpaths = rpaths + ":" + existing_rpath + # Join safely to avoid leading/trailing empty path elements. + target_rpaths = ":".join([p for p in (rpaths, existing_rpath) if p])
♻️ Duplicate comments (7)
scripts/wheel/patch_wheel.py (2)
47-48: Thanks for fixing the unconditional truthy branch for py3-none.
elif "py3-none" in str(whl):correctly addresses the earlier bug where"py3-none"(a non-empty string) was always truthy. Looks good.
45-50: Add cp314 handling and replaceassert Falsewith a proper exception (also fix typo).
- Wheels tagged
cp314will currently fall into theelse:and trigger anassert(or worse: be optimized away underpython -O).- Replace the assert with an explicit exception and fix the message typo (“Unknow” → “Unknown”).
Apply:
@@ - elif "cp313" in str(whl): - py_version = "3.13" - elif "py3-none" in str(whl): - py_version = None - else: - assert False, f"Unknow python version in {whl}" + elif "cp313" in str(whl): + py_version = "3.13" + elif "cp314" in str(whl): + py_version = "3.14" + elif "py3-none" in str(whl): + py_version = None + else: + raise AssertionError(f"Unknown Python version tag in {whl.name}")cmake/cmake_extension.py (1)
249-267: Extension ends up under sherpa_onnx/lib and os.system is used; import will fail without a loader and shell calls are non-portablePlacing _sherpa_onnx.* under sherpa_onnx/lib breaks import sherpa_onnx._sherpa_onnx unless you add loader logic. Also, os.system("mkdir …")/os.system("dir …") is brittle across platforms and unnecessary.
Action items:
- Copy the extension into the package root (sherpa_onnx/) so Python can import it without extra loader code.
- Replace os.system calls with os.makedirs and remove dir invocations.
- Fail fast if the extension wasn’t found, instead of silently returning a wheel without the module.
Apply this refactor within split mode:
- dst = os.path.join(f"{self.build_lib}", "sherpa_onnx", "lib") - os.system(f"mkdir {dst}") - os.system(f"dir {dst}") + # Place extension where Python import machinery expects it + dst = os.path.join(self.build_lib, "sherpa_onnx") + os.makedirs(dst, exist_ok=True) import glob ext = "pyd" if sys.platform.startswith("win") else "so" pattern = os.path.join(self.build_temp, "**", f"_sherpa_onnx.*.{ext}") matches = glob.glob(pattern, recursive=True) - print("matches", list(matches)) + print("matches", list(matches)) + if not matches: + raise FileNotFoundError( + f"Did not find built extension matching {pattern}" + ) for f in matches: - print(f, os.path.join(f"{self.build_lib}", "sherpa_onnx", "lib")) - shutil.copy(f"{f}", dst) - os.system(f"dir {dst}") + print("Copy:", f, "->", dst) + shutil.copy2(f, dst) returnIf you must keep the extension under sherpa_onnx/lib, add a minimal loader in sherpa_onnx/init.py to make imports work:
# In sherpa_onnx/__init__.py import os, sys _lib = os.path.join(os.path.dirname(__file__), "lib") if os.path.isdir(_lib) and _lib not in sys.path: sys.path.insert(0, _lib) del _libOptionally add a sanity test step in CI: python -c "import sherpa_onnx, sherpa_onnx._sherpa_onnx; print('ok')".
.github/workflows/build-wheels-macos-universal2.yaml (2)
241-253: Remove Debian-specific pip flag on macOS; upgrade pip first--break-system-packages is Linux/Debian-specific and unnecessary on macOS. It can cause failures.
- - name: Publish wheels to PyPI ${{ github.event.inputs.publish_sherpa_onnx_bin }} + - name: Publish wheels to PyPI if: ${{ matrix.os == 'macos-latest' && (github.event.inputs.publish_sherpa_onnx_bin || 'true') == 'true' }} env: TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} shell: bash run: | - opts='--break-system-packages' - - python3 -m pip install $opts --upgrade pip - python3 -m pip install $opts wheel twine==5.0.0 setuptools + python3 -m pip install --upgrade pip + python3 -m pip install wheel twine==5.0.0 setuptools twine upload /tmp/wheels/*.whlOptionally simplify the if condition to if: ${{ inputs.publish_sherpa_onnx_bin && matrix.os == 'macos-latest' }}.
338-346: Remove Debian-only pip flag in universal2 publish step as wellSame issue as above; drop --break-system-packages and upgrade pip first.
- - name: Publish wheels to PyPI + - name: Publish wheels to PyPI env: TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} run: | - opts='--break-system-packages' - - python3 -m pip install $opts --upgrade pip - python3 -m pip install $opts wheel twine==5.0.0 setuptools + python3 -m pip install --upgrade pip + python3 -m pip install wheel twine==5.0.0 setuptools twine upload ./wheelhouse/*.whl.github/workflows/build-wheels-macos-arm64.yaml (2)
240-252: Remove Debian-specific pip flag on macOS and upgrade pip firstMirrors the universal2 comment; drop --break-system-packages here to prevent macOS failures.
- - name: Publish wheels to PyPI ${{ github.event.inputs.publish_sherpa_onnx_bin }} + - name: Publish wheels to PyPI if: ${{ (github.event.inputs.publish_sherpa_onnx_bin || 'true') == 'true' }} env: TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} shell: bash run: | - opts='--break-system-packages' - - python3 -m pip install $opts --upgrade pip - python3 -m pip install $opts wheel twine==5.0.0 setuptools + python3 -m pip install --upgrade pip + python3 -m pip install wheel twine==5.0.0 setuptools twine upload /tmp/wheels/*.whlOptionally simplify the if condition to if: ${{ inputs.publish_sherpa_onnx_bin }}.
333-343: Remove Debian-only pip flag in the final arm64 publish jobSame fix for the final arm64 wheel publish step.
- - name: Publish wheels to PyPI + - name: Publish wheels to PyPI env: TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} run: | - opts='--break-system-packages' - - python3 -m pip install $opts --upgrade pip - python3 -m pip install $opts wheel twine==5.0.0 setuptools + python3 -m pip install --upgrade pip + python3 -m pip install wheel twine==5.0.0 setuptools twine upload ./wheelhouse/*.whl
🧹 Nitpick comments (20)
scripts/wheel/patch_wheel.py (3)
32-33: Avoid shell=True and quoting pitfalls in unzip call.Use argument lists with
shell=Falseto handle paths with spaces and avoid injection hazards.- subprocess.check_call(f"unzip {whl} -d {tmp_dir}", shell=True) + subprocess.check_call(["unzip", str(whl), "-d", str(tmp_dir)])
82-85: Avoid shell=True for patchelf invocation.Pass arguments directly; this is safer and avoids quoting issues for
$ORIGINand file paths.- subprocess.check_call( - f"patchelf --force-rpath --set-rpath '{target_rpaths}' {filename}", - shell=True, - ) + subprocess.check_call( + ["patchelf", "--force-rpath", "--set-rpath", target_rpaths, filename] + )
74-74:recursive=Truehas no effect with this pattern; decide single-level vs deep search.Since the pattern doesn’t contain
**,recursive=Trueis ignored. Either drop it or make the pattern recursive if you expect nested directories underbin.- for filename in glob.glob(f"{tmp_dir}/sherpa_onnx*data/data/bin/*", recursive=True): + for filename in glob.glob(f"{tmp_dir}/sherpa_onnx*data/data/bin/*"):If you do expect nested subdirectories:
- for filename in glob.glob(f"{tmp_dir}/sherpa_onnx*data/data/bin/*", recursive=True): + for filename in glob.glob(f"{tmp_dir}/sherpa_onnx*data/data/bin/**/*", recursive=True).github/workflows/run-python-test.yaml (1)
164-165: Guard CLI dependency checks to be split-package safe.With sherpa-onnx-bin potentially owning CLI binaries, sherpa_onnx-*.data/data/bin/sherpa-onnx may not exist in all builds. Make the check conditional to avoid brittle failures when only the core/Python wheel is present.
Apply this diff in both “Show dependencies” sections:
- readelf -d sherpa_onnx-*.data/data/bin/sherpa-onnx + f=$(echo sherpa_onnx-*.data/data/bin/sherpa-onnx 2>/dev/null || true) + if [ -f "$f" ]; then + readelf -d "$f" + else + echo "skip: no sherpa-onnx CLI binary in this wheel" + fiIf you expect the CLI to always be present here, confirm that this workflow builds the “bin” wheel in split mode or installs sherpa-onnx-bin before this step.
Also applies to: 177-178
cmake/onnxruntime-win-x64-gpu.cmake (1)
135-136: Standardizing DLL installs to lib and bin improves out-of-the-box usability on Windows.This makes both the Python extension (loading from lib with AddDllDirectory or PATH) and the CLI tools (loading from their bin dir) work without extra environment setup.
Optional: to avoid unconditional duplication in scenarios where only one consumer exists, gate the second install behind a toggle. Default ON preserves current behavior.
- install(FILES ${onnxruntime_lib_files} DESTINATION lib) - install(FILES ${onnxruntime_lib_files} DESTINATION bin) + install(FILES ${onnxruntime_lib_files} DESTINATION lib) + if(SHERPA_ONNX_INSTALL_DLLS_TO_BIN) + install(FILES ${onnxruntime_lib_files} DESTINATION bin) + endif()Add once near the top of the file (or a shared cmake include):
# Place near other options option(SHERPA_ONNX_INSTALL_DLLS_TO_BIN "Install runtime DLLs to bin in addition to lib" ON)cmake/onnxruntime-win-x64.cmake (1)
85-86: Unconditional install to both lib and bin brings x64 parity with other Windows targets.Matches the GPU/x86/arm64 behavior and simplifies assumptions for consumers.
Same optional toggle suggestion as above to reduce duplication where appropriate:
- install(FILES ${onnxruntime_lib_files} DESTINATION lib) - install(FILES ${onnxruntime_lib_files} DESTINATION bin) + install(FILES ${onnxruntime_lib_files} DESTINATION lib) + if(SHERPA_ONNX_INSTALL_DLLS_TO_BIN) + install(FILES ${onnxruntime_lib_files} DESTINATION bin) + endif()And ensure the option is declared once in a shared location or at the top of this file:
option(SHERPA_ONNX_INSTALL_DLLS_TO_BIN "Install runtime DLLs to bin in addition to lib" ON)cmake/onnxruntime-win-arm64.cmake (1)
85-86: ARM64 DLL install standardized to lib and bin — consistent with x64/x86/GPU flows.Good for predictability across all Windows architectures.
Consider the same optional gating to avoid duplicating DLLs when not needed:
- install(FILES ${onnxruntime_lib_files} DESTINATION lib) - install(FILES ${onnxruntime_lib_files} DESTINATION bin) + install(FILES ${onnxruntime_lib_files} DESTINATION lib) + if(SHERPA_ONNX_INSTALL_DLLS_TO_BIN) + install(FILES ${onnxruntime_lib_files} DESTINATION bin) + endif()Ensure the following option exists (in a shared CMake include or at the top):
option(SHERPA_ONNX_INSTALL_DLLS_TO_BIN "Install runtime DLLs to bin in addition to lib" ON)cmake/onnxruntime-win-x86.cmake (1)
85-86: Avoid duplicating DLL installs; keep a single destination to reduce artifact sizeInstalling the same DLLs to both lib and bin bloats the install tree and can confuse downstream packaging scripts. Pick one destination (prefer bin for runtime DLLs) and keep import libraries in lib. If other scripts expect DLLs under lib, standardize on that and remove the duplicate.
Apply one of the following diffs (Option A preferred):
Option A (install runtime DLLs only to bin):
-install(FILES ${onnxruntime_lib_files} DESTINATION lib) install(FILES ${onnxruntime_lib_files} DESTINATION bin)Option B (if your packaging flow strictly consumes from lib, drop bin):
install(FILES ${onnxruntime_lib_files} DESTINATION lib) - install(FILES ${onnxruntime_lib_files} DESTINATION bin)cmake/cmake_extension.py (1)
169-171: Typo in comment: “onverride” → “override”Minor spelling fix in developer-facing comment.
-# so they can onverride the "defaults" stored in `extra_cmake_args` +# so they can override the "defaults" stored in `extra_cmake_args`.github/workflows/build-wheels-aarch64.yaml (4)
169-178: Confirm the necessity of patch_wheel.py or remove it for simplicityYou reintroduced patch_wheel.py here while other flows moved to auditwheel-only inspection. If patch_wheel.py is still required for core/bin wheels, keep it; otherwise, drop this step to simplify and reduce risk.
Would you like me to inline auditwheel commands to fully replace patch_wheel.py here?
214-236: Ensure readelf is available on the runnerreadelf may be missing on ubuntu-24.04-arm. Install binutils before running readelf to avoid spurious failures.
- name: Show help shell: bash run: | sherpa-onnx --help + sudo apt-get update -q + sudo apt-get install -y binutils echo "---" ls -lh $(which sherpa-onnx) file $(which sherpa-onnx) readelf -d $(which sherpa-onnx) ldd $(which sherpa-onnx)
279-290: Simplify the PyPI publish conditionUse the boolean input directly; current expression mixes string fallback and can be brittle.
- - name: Publish wheels to PyPI ${{ github.event.inputs.publish_sherpa_onnx_bin }} - if: ${{ (github.event.inputs.publish_sherpa_onnx_bin || 'true') == 'true' }} + - name: Publish wheels to PyPI + if: ${{ inputs.publish_sherpa_onnx_bin }}
349-376: readelf dependency in the wheel-inspection stepThe Display/Show wheels step uses readelf but doesn’t install binutils in this job. Add the same binutils install here, or drop readelf if not essential.
- name: Show wheels shell: bash run: | ls -lh wheelhouse/*.whl - unzip -l wheelhouse/*.whl - echo "---" - mkdir t cp wheelhouse/*.whl ./t cd ./t unzip ./*.whl ls -lh echo "---" - - readelf -d sherpa_onnx/lib/*.so + sudo apt-get update -q && sudo apt-get install -y binutils + readelf -d sherpa_onnx/lib/*.so.github/workflows/build-wheels-macos-universal2.yaml (1)
274-285: Avoid redundant MACOSX_DEPLOYMENT_TARGET settingsYou set MACOSX_DEPLOYMENT_TARGET via both a dedicated step and again in CIBW_ENVIRONMENT. Keep one source of truth to avoid mismatches.
Example simplification: rely only on CIBW_ENVIRONMENT and remove the separate Set macOS deployment target step.
- - name: Set macOS deployment target - run: echo "MACOSX_DEPLOYMENT_TARGET=10.15" >> $GITHUB_ENV.github/workflows/build-wheels-macos-x64.yaml (6)
96-105: Avoid bundling static libraries; copy only dylibscp -v ./build/install/lib/lib* may inadvertently include static archives (*.a) or other files. Since you already remove libcargs.a, ensure only dynamic libs are shipped in sherpa-onnx-core to keep wheels lean and consistent.
- cp -v ./build/install/lib/lib* ./scripts/wheel/sherpa-onnx-core/sherpa_onnx/lib + cp -v ./build/install/lib/*.dylib ./scripts/wheel/sherpa-onnx-core/sherpa_onnx/lib || trueOptional: fail fast if headers are missing to catch packaging regressions early:
- cp -v ./build/install/include/sherpa-onnx/c-api/*.h ./scripts/wheel/sherpa-onnx-core/sherpa_onnx/include/sherpa-onnx/c-api + shopt -s nullglob + headers=(./build/install/include/sherpa-onnx/c-api/*.h) + [[ ${#headers[@]} -gt 0 ]] || { echo "No C API headers found"; exit 1; } + cp -v "${headers[@]}" ./scripts/wheel/sherpa-onnx-core/sherpa_onnx/include/sherpa-onnx/c-apiAlso applies to: 108-111
259-261: Confirm Python 3.14 (cp314) availability in cibuildwheel imagesSupport for cp314 may lag behind Python releases. If not yet supported, this matrix entry will fail. Consider temporarily excluding cp314 or gating it behind a manual flag until cibuildwheel publishes cp314 images.
Proposed conservative adjustment:
- python-version: ["cp38", "cp39", "cp310", "cp311", "cp312", "cp313", "cp314"] + python-version: ["cp38", "cp39", "cp310", "cp311", "cp312", "cp313"]
274-288: Minor: trailing space in CIBW_BUILD patternCIBW_BUILD has a trailing space that’s unnecessary and can be confusing in logs.
- CIBW_BUILD: "${{ matrix.python-version}}-* " + CIBW_BUILD: "${{ matrix.python-version}}-*"
300-335: Reduce duplication: factor HuggingFace publish into a reusable actionThe HF publish block appears twice (test and build jobs) and runs in multiple matrix permutations, which increases maintenance and risk of push races. Consider extracting it into a composite/reusable workflow or uploading a single merged artifact then publishing once.
204-239: Safer credential handling for HuggingFace pushesCloning with credentials embedded in the URL works but is noisier in logs and process lists. Prefer using a temporary credential helper or GH Actions’ built-in masking via an env-based askpass.
Example adjustment:
- git clone https://csukuangfj:$HF_TOKEN@huggingface.co/csukuangfj/sherpa-onnx-wheels huggingface + git -c credential.helper='store --file=.git-credentials' \ + config --global credential.useHttpPath true + printf "https://csukuangfj:%s@huggingface.co\n" "$HF_TOKEN" > .git-credentials + git clone https://huggingface.co/csukuangfj/sherpa-onnx-wheels huggingface
336-347: Switch to PyPI Trusted Publishing and gate by tag to prevent accidental releasesUsing username/password is legacy and less secure than PyPI’s OIDC Trusted Publisher. Also, consider publishing only on tagged releases to avoid unintentional uploads.
Example (replace the step):
- - name: Publish wheels to PyPI - env: - TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} - TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} - run: | - opts='--break-system-packages' - python3 -m pip install $opts --upgrade pip - python3 -m pip install $opts wheel twine==5.0.0 setuptools - twine upload ./wheelhouse/*.whl + - name: Publish wheels to PyPI (Trusted Publisher) + if: startsWith(github.ref, 'refs/tags/') + uses: pypa/gh-action-pypi-publish@v1.10.1 + with: + packages-dir: ./wheelhouse/Note: This requires configuring a PyPI Trusted Publisher for the repository once.
📜 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 (14)
.github/workflows/build-wheels-aarch64.yaml(5 hunks).github/workflows/build-wheels-linux.yaml(3 hunks).github/workflows/build-wheels-macos-arm64.yaml(4 hunks).github/workflows/build-wheels-macos-universal2.yaml(4 hunks).github/workflows/build-wheels-macos-x64.yaml(4 hunks).github/workflows/run-python-test.yaml(2 hunks)cmake/cmake_extension.py(5 hunks)cmake/onnxruntime-win-arm64.cmake(1 hunks)cmake/onnxruntime-win-x64-gpu.cmake(1 hunks)cmake/onnxruntime-win-x64.cmake(1 hunks)cmake/onnxruntime-win-x86.cmake(1 hunks)scripts/wheel/patch_wheel.py(1 hunks)scripts/wheel/sherpa-onnx-core/setup.py(1 hunks)sherpa-onnx/python/csrc/CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/wheel/sherpa-onnx-core/setup.py
- .github/workflows/build-wheels-linux.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 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:
.github/workflows/run-python-test.yaml
🪛 Ruff (0.12.2)
scripts/wheel/patch_wheel.py
50-50: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: ubuntu-latest Debug static tts-OFF
- GitHub Check: macos-latest Debug shared tts-OFF
- GitHub Check: macos-latest Release shared tts-OFF
- GitHub Check: ubuntu-latest Debug static tts-ON
- GitHub Check: ubuntu-latest Debug shared tts-OFF
- GitHub Check: ubuntu-latest Release static tts-ON
- GitHub Check: ubuntu-latest Release shared tts-OFF
- GitHub Check: ubuntu-latest Debug shared tts-ON
- GitHub Check: ubuntu-latest Release static tts-OFF
- GitHub Check: ubuntu-latest Release shared tts-ON
- GitHub Check: ubuntu-24.04 3.13
- GitHub Check: ubuntu-24.04 3.12
- GitHub Check: ubuntu-24.04 3.10
- GitHub Check: ubuntu-24.04 3.11
- GitHub Check: ubuntu-24.04 3.9
- GitHub Check: ubuntu-24.04 3.8
- GitHub Check: rknn shared OFF
- GitHub Check: rknn shared ON
- GitHub Check: swift (macos-13)
- GitHub Check: swift (macos-latest)
🔇 Additional comments (6)
.github/workflows/run-python-test.yaml (2)
160-160: Correct path for the extension after lib/ relocation — good catch.Switching readelf to inspect sherpa_onnx/lib/_sherpa_onnx*.so matches the new wheel layout and avoids false negatives when the extension is no longer at the package root.
173-173: Patched-wheel check updated consistently.Mirrors the unpatched flow; keeps visibility into DT_NEEDED/RPATH for the relocated extension.
cmake/cmake_extension.py (1)
146-164: Good: clean gating of install/C API/websocket flags for split modeSkipping CMAKE_INSTALL_PREFIX, C API, and websocket when splitting keeps the Python-wheel-only path lean and avoids shipping components that belong in the new -core/-bin wheels. Looks aligned with the PR goals.
.github/workflows/build-wheels-macos-x64.yaml (3)
111-121: Wheel builds for core and bin look goodExplicit plat-name macosx_10_15_x86_64 is aligned with the chosen deployment target and architecture.
Also applies to: 122-132
171-195: Sanity checks are effectiveInvoking version/help and inspecting linkage with otool provide quick validation that CLI shims and rpaths are correct.
Also applies to: 198-203
289-299: Artifacts naming/readout is clearThe unzip listing is handy when skimming logs, and the artifact naming includes OS/arch and Python tag.
Also applies to: 295-299
| inputs: | ||
| publish_sherpa_onnx_bin: | ||
| description: "Publish sherpa-onnx-bin" | ||
| required: false | ||
| default: "true" | ||
| type: boolean |
There was a problem hiding this comment.
Fix boolean input default and avoid string/boolean confusion
The input is declared as type boolean but the default is the string "true". This can lead to subtle condition bugs later.
Apply this diff to store a real boolean:
workflow_dispatch:
inputs:
publish_sherpa_onnx_bin:
description: "Publish sherpa-onnx-bin"
required: false
- default: "true"
+ default: true
type: boolean🤖 Prompt for AI Agents
.github/workflows/build-wheels-macos-x64.yaml lines 8-13: The workflow input
publish_sherpa_onnx_bin is declared type: boolean but its default is the string
"true", causing string/boolean confusion; change the default to an actual
boolean value (without quotes) so it is parsed as true, and update any
references that compare against string values to use boolean checks if
necessary.
| core: | ||
| runs-on: ${{ matrix.os }} | ||
| name: core | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: [macos-13] | ||
| python-version: ["cp38", "cp39", "cp310", "cp311", "cp312", "cp313"] | ||
| os: [macos-latest] | ||
|
|
||
| steps: |
There was a problem hiding this comment.
Run x64 core build on an Intel runner (macos-13) to avoid cross-arch issues
This job forces CMAKE_OSX_ARCHITECTURES='x86_64' but runs on macos-latest, which is Apple Silicon by default. Cross-compiling system Python extensions and native tools for x86_64 on ARM runners is fragile and often unsupported by toolchains and cibuildwheel. Use macos-13 (Intel) for deterministic x64 outputs.
- core:
- runs-on: ${{ matrix.os }}
+ core:
+ runs-on: ${{ matrix.os }}
@@
- matrix:
- os: [macos-latest]
+ matrix:
+ os: [macos-13]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| core: | |
| runs-on: ${{ matrix.os }} | |
| name: core | |
| strategy: | |
| fail-fast: false | |
| matrix: | |
| os: [macos-13] | |
| python-version: ["cp38", "cp39", "cp310", "cp311", "cp312", "cp313"] | |
| os: [macos-latest] | |
| steps: | |
| core: | |
| runs-on: ${{ matrix.os }} | |
| name: core | |
| strategy: | |
| fail-fast: false | |
| matrix: | |
| os: [macos-13] | |
| steps: |
🤖 Prompt for AI Agents
.github/workflows/build-wheels-macos-x64.yaml around lines 23 to 31: the job is
set to run on macos-latest but forces CMAKE_OSX_ARCHITECTURES='x86_64', causing
fragile cross-arch builds on Apple Silicon; update the matrix or runs-on to use
an Intel runner (macos-13) so the job executes on an x86_64 macOS image to
produce deterministic x64 outputs (e.g., change matrix.os or runs-on from
macos-latest to macos-13).
| - name: Configure CMake | ||
| shell: bash | ||
| run: | | ||
| export CMAKE_CXX_COMPILER_LAUNCHER=ccache | ||
| export PATH="/usr/lib/ccache:/usr/local/opt/ccache/libexec:$PATH" | ||
| cmake --version | ||
|
|
||
| mkdir build | ||
| cd build | ||
|
|
||
| cmake \ | ||
| -DSHERPA_ONNX_SPLIT_PYTHON_PACKAGE=ON \ | ||
| -DCMAKE_OSX_DEPLOYMENT_TARGET=10.15 \ | ||
| -D BUILD_SHARED_LIBS=ON \ | ||
| -D SHERPA_ONNX_BUILD_C_API_EXAMPLES=OFF \ | ||
| -D CMAKE_BUILD_TYPE=Release \ | ||
| -D CMAKE_OSX_ARCHITECTURES='x86_64' \ | ||
| -D CMAKE_INSTALL_PREFIX=./install \ | ||
| .. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make ccache path robust across Intel and Apple Silicon runners
The PATH adds Homebrew’s Intel prefix only. On Apple Silicon, ccache lives under /opt/homebrew/opt/ccache/libexec. Add both to avoid tool discovery failures if this ever runs on ARM or if the runner image changes.
- export PATH="/usr/lib/ccache:/usr/local/opt/ccache/libexec:$PATH"
+ export PATH="/usr/lib/ccache:/usr/local/opt/ccache/libexec:/opt/homebrew/opt/ccache/libexec:$PATH"Apply the same adjustment in the build step:
- export PATH="/usr/lib/ccache:/usr/local/opt/ccache/libexec:$PATH"
+ export PATH="/usr/lib/ccache:/usr/local/opt/ccache/libexec:/opt/homebrew/opt/ccache/libexec:$PATH"Also applies to: 76-84
🤖 Prompt for AI Agents
.github/workflows/build-wheels-macos-x64.yaml around lines 56 to 75 (also apply
to lines 76 to 84): the workflow only adds Homebrew’s Intel ccache path
(/usr/local/opt/ccache/libexec) to PATH, which breaks tool discovery on Apple
Silicon where ccache is at /opt/homebrew/opt/ccache/libexec; modify the PATH
exports to include both prefixes (Intel and Apple Silicon) and ensure the order
is safe (e.g. prepend /usr/lib/ccache then both Homebrew prefixes) so ccache is
found on either runner, and apply the same dual-path update to the subsequent
build step at lines 76-84.
| - name: Publish wheels to PyPI ${{ github.event.inputs.publish_sherpa_onnx_bin }} | ||
| if: ${{ (github.event.inputs.publish_sherpa_onnx_bin || 'true') == 'true' }} | ||
| env: | ||
| TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} | ||
| TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} | ||
| shell: bash | ||
| run: | | ||
| python3 -m pip install --upgrade pip | ||
| python3 -m pip install wheel twine==5.0.0 setuptools | ||
|
|
||
| twine upload /tmp/wheels/*.whl | ||
|
|
There was a problem hiding this comment.
Guard PyPI publishing so it never triggers on push and correctly handles booleans
Current condition may publish from push events due to the fallback string logic. Restrict publishing to manual runs and rely on the (boolean) input.
Apply this diff:
- - name: Publish wheels to PyPI ${{ github.event.inputs.publish_sherpa_onnx_bin }}
- if: ${{ (github.event.inputs.publish_sherpa_onnx_bin || 'true') == 'true' }}
+ - name: Publish wheels to PyPI (bin/core): ${{ inputs.publish_sherpa_onnx_bin }}
+ if: ${{ github.event_name == 'workflow_dispatch' && (inputs.publish_sherpa_onnx_bin == true || inputs.publish_sherpa_onnx_bin == 'true') }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Publish wheels to PyPI ${{ github.event.inputs.publish_sherpa_onnx_bin }} | |
| if: ${{ (github.event.inputs.publish_sherpa_onnx_bin || 'true') == 'true' }} | |
| env: | |
| TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} | |
| TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} | |
| shell: bash | |
| run: | | |
| python3 -m pip install --upgrade pip | |
| python3 -m pip install wheel twine==5.0.0 setuptools | |
| twine upload /tmp/wheels/*.whl | |
| - name: Publish wheels to PyPI (bin/core): ${{ inputs.publish_sherpa_onnx_bin }} | |
| if: ${{ github.event_name == 'workflow_dispatch' && (inputs.publish_sherpa_onnx_bin == true || inputs.publish_sherpa_onnx_bin == 'true') }} | |
| env: | |
| TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} | |
| TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} | |
| shell: bash | |
| run: | | |
| python3 -m pip install --upgrade pip | |
| python3 -m pip install wheel twine==5.0.0 setuptools | |
| twine upload /tmp/wheels/*.whl |
🤖 Prompt for AI Agents
In .github/workflows/build-wheels-macos-x64.yaml around lines 240 to 251, the
step's if condition uses a string fallback that can allow publishing on push;
change the condition to only run for manual workflow_dispatch runs and
explicitly check the input's boolean value, e.g., require github.event_name ==
'workflow_dispatch' and that github.event.inputs.publish_sherpa_onnx_bin ==
'true'; remove the fallback string logic so the input controls publishing
reliably.
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: [macos-latest] | ||
| python-version: ["cp38", "cp39", "cp310", "cp311", "cp312", "cp313", "cp314"] | ||
|
|
There was a problem hiding this comment.
Pin wheel build job to macos-13 (Intel) for x86_64 wheels
Same concern as the core job. cibuildwheel does not support cross-building macOS wheels across architectures; use an Intel runner for x86_64.
strategy:
fail-fast: false
matrix:
- os: [macos-latest]
+ os: [macos-13]
python-version: ["cp38", "cp39", "cp310", "cp311", "cp312", "cp313", "cp314"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| runs-on: ${{ matrix.os }} | |
| strategy: | |
| fail-fast: false | |
| matrix: | |
| os: [macos-latest] | |
| python-version: ["cp38", "cp39", "cp310", "cp311", "cp312", "cp313", "cp314"] | |
| runs-on: ${{ matrix.os }} | |
| strategy: | |
| fail-fast: false | |
| matrix: | |
| os: [macos-13] | |
| python-version: ["cp38", "cp39", "cp310", "cp311", "cp312", "cp313", "cp314"] |
🤖 Prompt for AI Agents
.github/workflows/build-wheels-macos-x64.yaml around lines 255-261: the job is
using macos-latest in the matrix which may select an ARM macOS runner; update
the workflow to pin the runner to an Intel macOS image by replacing macos-latest
with macos-13 (Intel) in the matrix.os (or set runs-on directly to macos-13) so
cibuildwheel runs on an x86_64 runner suitable for building Intel wheels.
| if py_version: | ||
| rpath_list = [ | ||
| f"$ORIGIN/../lib/python{py_version}/site-packages/sherpa_onnx/lib", | ||
| f"$ORIGIN/../lib/python{py_version}/dist-packages/sherpa_onnx/lib", | ||
| # | ||
| f"$ORIGIN/../lib/python{py_version}/site-packages/sherpa_onnx/lib64", | ||
| f"$ORIGIN/../lib/python{py_version}/dist-packages/sherpa_onnx/lib64", | ||
| # | ||
| f"$ORIGIN/../lib/python{py_version}/site-packages/sherpa_onnx.libs", | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parity gap in rpaths for py3-none: missing lib64 and .libs entries.
When py_version is None (py3-none wheels), the rpaths omit lib64 and .libs paths that you include when a specific Python version is detected. This can break runtime on systems where libraries land in lib64 or when .libs (auditwheel) is used.
Apply:
@@
- else:
- rpath_list = []
- for p in ["3.8", "3.9", "3.10", "3.11", "3.12", "3.13", "3.14"]:
- rpath_list.extend(
- [
- f"$ORIGIN/../lib/python{p}/site-packages/sherpa_onnx/lib",
- f"$ORIGIN/../lib/python{p}/dist-packages/sherpa_onnx/lib",
- ]
- )
+ else:
+ versions = ["3.8", "3.9", "3.10", "3.11", "3.12", "3.13", "3.14"]
+ rpath_list = []
+ for p in versions:
+ rpath_list.extend(
+ [
+ f"$ORIGIN/../lib/python{p}/site-packages/sherpa_onnx/lib",
+ f"$ORIGIN/../lib/python{p}/dist-packages/sherpa_onnx/lib",
+ f"$ORIGIN/../lib/python{p}/site-packages/sherpa_onnx/lib64",
+ f"$ORIGIN/../lib/python{p}/dist-packages/sherpa_onnx/lib64",
+ f"$ORIGIN/../lib/python{p}/site-packages/sherpa_onnx.libs",
+ ]
+ )Also applies to: 63-70
🤖 Prompt for AI Agents
In scripts/wheel/patch_wheel.py around lines 52-61 (and similarly lines 63-70),
the rpath list built when py_version is None is missing the lib64 and .libs
entries present in the py_version-specific branch; update the py_version is None
branch to append the same four additional paths (lib64 and .libs variants) used
above so rpath_list includes
"$ORIGIN/../lib/python{py_version}/site-packages/sherpa_onnx/lib64",
"$ORIGIN/../lib/python{py_version}/dist-packages/sherpa_onnx/lib64", and the
".libs" site-packages path (adapted for the py3-none case) to ensure runtime
resolution on systems using lib64 or auditwheel .libs.
| install(TARGETS _sherpa_onnx | ||
| DESTINATION ../ | ||
| ) | ||
| install(TARGETS _sherpa_onnx DESTINATION lib) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Install the extension into lib/ — aligns with the new package layout.
Installing _sherpa_onnx to lib/ is consistent with importing from sherpa_onnx.lib and with the CI changes that add lib/ to LD_LIBRARY_PATH.
That said, please verify rpath consistency now that the module itself resides under lib/:
- Line 103 currently injects -Wl,-rpath,${SHERPA_ONNX_RPATH_ORIGIN}/sherpa_onnx/lib. If $ORIGIN already resolves to .../site-packages/sherpa_onnx/lib (because the module is placed there), appending /sherpa_onnx/lib could overshoot (lib/sherpa_onnx/lib). It may be safer to use $ORIGIN alone in this layout.
You can quickly audit the intended value and usage across the repo:
🏁 Script executed:
#!/bin/bash
# Inspect where SHERPA_ONNX_RPATH_ORIGIN is defined and used
rg -nP -C3 'SHERPA_ONNX_RPATH_ORIGIN'
# If you have a built artifact locally, check RUNPATH/RPATH
fd '_sherpa_onnx.*\.so' -HI | xargs -I{} sh -c 'echo "== {} =="; readelf -d "{}" | rg -n "RPATH|RUNPATH" || true'Length of output: 8114
Adjust Python extension RPATH to use only the origin directory
The root CMakeLists sets SHERPA_ONNX_RPATH_ORIGIN to $ORIGIN (or @loader_path on Apple) and applies it via CMAKE_INSTALL_RPATH, so the extension’s runtime path already includes the directory containing _sherpa_onnx.so. Manually appending /sherpa_onnx/lib yields a lookup of $ORIGIN/sherpa_onnx/lib, which, once _sherpa_onnx.so is installed into …/site-packages/sherpa_onnx/lib, overextends to …/site-packages/sherpa_onnx/lib/sherpa_onnx/lib.
Please update the rpath injection in the Python CMakeLists:
• File sherpa-onnx/python/csrc/CMakeLists.txt, line 104
- target_link_libraries(_sherpa_onnx PRIVATE "-Wl,-rpath,${SHERPA_ONNX_RPATH_ORIGIN}/sherpa_onnx/lib")
+ target_link_libraries(_sherpa_onnx PRIVATE "-Wl,-rpath,${SHERPA_ONNX_RPATH_ORIGIN}")Alternatively, you can remove the manual rpath flag entirely and rely on the global CMAKE_INSTALL_RPATH setting. This ensures dependencies shipped alongside _sherpa_onnx.so in the same lib/ directory are correctly found at runtime.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| install(TARGETS _sherpa_onnx DESTINATION lib) | |
| # In sherpa-onnx/python/csrc/CMakeLists.txt | |
| # Link the Python extension with origin-only rpath | |
| target_link_libraries(_sherpa_onnx PRIVATE "-Wl,-rpath,${SHERPA_ONNX_RPATH_ORIGIN}") | |
| # Install the extension into the lib directory | |
| install(TARGETS _sherpa_onnx DESTINATION lib) |
🤖 Prompt for AI Agents
In sherpa-onnx/python/csrc/CMakeLists.txt around lines 104-117, the CMakeLists
manually appends "/sherpa_onnx/lib" to the Python extension RPATH which causes
duplicated paths like .../site-packages/sherpa_onnx/lib/sherpa_onnx/lib; remove
the manual rpath injection (or change it to use only $ORIGIN / @loader_path) so
the target relies on the global SHERPA_ONNX_RPATH_ORIGIN / CMAKE_INSTALL_RPATH
set in the root CMakeLists; update the install(TARGETS _sherpa_onnx ...) stanza
to not append extra RPATH flags (or replace them with a single
$ORIGIN-equivalent) so dependencies in the same lib/ directory are resolved
correctly at runtime.
| @@ -0,0 +1,35 @@ | |||
| import sys | |||
There was a problem hiding this comment.
Note that __main__.py and _info.py are generated by chatgpt. I did only some minor changes.
Issue #2517 reports that our Python package has been flagged as malware. I suspect this is because the wheel includes a large number of binary executables.
Another drawback of our current approach is that every wheel bundles the same binaries, even though they only depend on the operating system and not the Python ABI. This leads to significant duplication, wastes storage space, and makes it easy to hit PyPI’s free storage limit of 10 GB.
This PR splits the current package into three smaller ones. If you only use the Python API, you can continue installing sherpa-onnx in the same way as before—the change is completely transparent to you.
The following details are for those curious about the changes.
Now we have 3 packages:
sherpa-onnx-core
It contains only the shared library files and the header files. You can install it with
Note that it is a dependency of
sherpa-onnx-binandsherpa-onnx. It is automagically installed if you installsherpa-onnx-binorsherpa-onnx.Its whl filename looks like the following:
You can see it contains
py3-none, which means it does not depend on the Python ABI.After installing, you can run
sherpa-onnx-bin
It contains pre-compiled binaries. You can use
to install it.
After installing, you can run the following in your terminal
Note that the commands
sherpa-onnx-version,sherpa-onnx,sherpa-onnx-offline, etc, are automagically available without changing yourPATHenvironment variable.sherpa-onnx
It contains our Python API.
Install it with
Summary by CodeRabbit
New Features
Improvements
Other
Breaking Changes