Fix CI - #3192
Fix CI#3192
Conversation
dosubot
Bot
added
the
size:L
Summary of ChangesHello @csukuangfj, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses continuous integration failures by enhancing debugging capabilities in the CMake build process, refining compilation conditions for the OpenFST library, and streamlining JSON serialization logic within the .NET scripts. These changes aim to improve the robustness and clarity of the build system and C# component. Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughReplaces many GitHub Action Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several fixes for the CI pipeline. The changes include updating an exception message for better clarity, adding a compiler version check in a CMake file to handle warnings, and removing conditional compilation in a C# file to improve compatibility. My main feedback is to remove the debugging print statements added to cmake/cmake_extension.py to ensure clean build logs.
| + shlex.split(cmake_args) | ||
| + ["-B", str(self.build_temp), "-S", str(sherpa_onnx_dir)] | ||
| ) | ||
| print("cmake_configure_cmd", cmake_configure_cmd) |
There was a problem hiding this comment.
This print statement appears to be for debugging and should be removed before merging to keep the build logs clean.
| "--", | ||
| "-m:2", | ||
| ] | ||
| print("cmake_build_cmd", cmake_build_cmd) |
There was a problem hiding this comment.
This print statement appears to be for debugging and should be removed before merging to keep the build logs clean.
| "--", | ||
| "-m:2", | ||
| ] | ||
| print("cmake_build_cmd", cmake_build_cmd) |
There was a problem hiding this comment.
This print statement appears to be for debugging and should be removed before merging to keep the build logs clean.
There was a problem hiding this comment.
Pull request overview
This PR addresses CI (Continuous Integration) issues by standardizing Docker execution patterns and removing conditional .NET 2.0 compilation code. The changes migrate from the deprecated addnab/docker-run-action to native docker run commands and simplify C# code by dropping .NET 2.0 support.
Changes:
- Replaced
addnab/docker-run-action@v3with directdocker runcommands across all workflow files - Removed .NET 2.0 conditional compilation directives and JSON serialization code paths
- Added compiler version checks for GCC 11+ compatibility flags
- Updated error messages and added debug print statements
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/dotnet/OfflineTtsGenerationConfig.cs | Removed .NET 2.0 conditional compilation, keeping only manual JSON serialization |
| cmake/openfst.cmake | Added GCC version check before applying compiler warning flags |
| cmake/cmake_extension.py | Added debug print statements and corrected error message text |
| .github/workflows/test-nodejs-addon-npm-aarch64.yaml | Replaced docker action with direct docker run command |
| .github/workflows/rknn-linux-aarch64.yaml | Replaced docker action with direct docker run command |
| .github/workflows/release-dart-package.yaml | Replaced docker action with direct docker run command for x86_64 and aarch64 builds |
| .github/workflows/npm-addon-linux-x64.yaml | Replaced docker action with direct docker run command |
| .github/workflows/npm-addon-linux-aarch64.yaml | Replaced docker action with direct docker run command |
| .github/workflows/nightly-wheel-arm.yaml | Replaced docker action with direct docker run command |
| .github/workflows/linux.yaml | Replaced docker action with direct docker run command, changed quote styles |
| .github/workflows/linux-jni.yaml | Replaced docker action with direct docker run command, updated version tag |
| .github/workflows/linux-jni-aarch64.yaml | Replaced docker action with direct docker run command, uncommented release parameters |
| .github/workflows/linux-gpu.yaml | Replaced docker action with direct docker run command |
| .github/workflows/build-wheels-linux.yaml | Replaced docker action with direct docker run command, updated volume paths and added debug output |
| .github/workflows/build-wheels-armv7l.yaml | Replaced docker action with direct docker run command, changed quote styles |
| .github/workflows/build-wheels-aarch64.yaml | Replaced docker action with direct docker run command, changed quote styles |
| .github/workflows/build-wheels-aarch64-rknn.yaml | Replaced docker action with direct docker run command |
| .github/workflows/axera-linux-aarch64.yaml | Replaced docker action with direct docker run command |
| .github/workflows/axcl-linux-aarch64.yaml | Replaced docker action with direct docker run command |
| .github/workflows/aarch64-linux-gnu-static.yaml | Replaced docker action with direct docker run command, removed comment |
| .github/workflows/aarch64-linux-gnu-shared.yaml | Replaced docker action with direct docker run command |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "multilib_policy=best" >> /etc/yum.conf | ||
| echo "skip_missing_names_on_install=False" >> /etc/yum.conf | ||
| sed -i '/^override_install_langs=/d' /etc/yum.conf | ||
| sed -i "/^override_install_langs=/d" /etc/yum.conf |
There was a problem hiding this comment.
Inconsistent quote style. The file uses single quotes elsewhere (lines 117, 120) but this change introduces double quotes. For consistency, consider using single quotes unless there's a specific reason for double quotes here.
| sed -i "/^override_install_langs=/d" /etc/yum.conf | |
| sed -i '\''/^override_install_langs=/d'\'' /etc/yum.conf |
| ls -lh install/bin | ||
|
|
||
| echo 'sherpa-onnx-core' | ||
| echo sherpa-onnx-core |
There was a problem hiding this comment.
Missing quotes around the echo argument. While functionally equivalent in this simple case, it's better practice to quote echo arguments consistently as done in line 137.
| echo sherpa-onnx-core | |
| echo "sherpa-onnx-core" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
cmake/cmake_extension.py (1)
235-236:⚠️ Potential issue | 🟡 MinorInconsistent error message: still says "sherpa" instead of "sherpa-onnx".
Line 204 was updated to say "Failed to configure sherpa-onnx", but line 236 still reads "Failed to build and install sherpa". Consider updating for consistency.
Proposed fix
- raise Exception("Failed to build and install sherpa") + raise Exception("Failed to build and install sherpa-onnx").github/workflows/aarch64-linux-gnu-shared.yaml (1)
96-144:⚠️ Potential issue | 🟡 MinorThe migration from
addnab/docker-run-action@v3to inlinedocker runlooks correct.Volume mount, image, and
bash -cwrapping are properly structured. The single-quoted script correctly preserves shell variable expansion for the container context while${{ }}expressions are expanded by the Actions runner beforehand.One pre-existing nit: Line 102 references
${{ matrix.config }}, but the matrix only definesos,gpu, andonnxruntime_version—configis undefined and will always be empty. The static analysis tool also flags this.Proposed fix
- echo "config: ${{ matrix.config }}" + echo "gpu: ${{ matrix.gpu }}".github/workflows/nightly-wheel-arm.yaml (1)
47-70:⚠️ Potential issue | 🟡 Minor
$GITHUB_ENVis not available inside the Docker container.Line 60 writes to
$GITHUB_ENV, but this environment variable is set by the GitHub Actions runner and won't exist inside the Docker container. This will cause a bash error ($GITHUB_ENV: ambiguous redirector similar).Since
PYTHON_VERSIONis only used within this same script block and no subsequent workflow steps reference it, this line can be safely removed.Proposed fix
v=${{ matrix.python-version }} PYTHON_VERSION=${v/./} - echo PYTHON_VERSION=$PYTHON_VERSION >> $GITHUB_ENV.github/workflows/npm-addon-linux-aarch64.yaml (1)
71-137:⚠️ Potential issue | 🟠 Major
sudo chown -R runner ./build(Line 127) will likely fail inside the Docker container.The
runneruser does not exist inside themanylinux2014_aarch64container, sochownwill fail withinvalid user: 'runner'. Withaddnab/docker-run-action, user mapping may have been handled by the action. With baredocker run, you need to use a numeric UID instead.Proposed fix — use numeric UID
- sudo chown -R runner ./build + chown -R $(id -u):$(id -g) ./buildAlternatively, if the intent is to match the host
runneruser (UID 1001 on GitHub-hosted runners), use:- sudo chown -R runner ./build + chown -R 1001:1001 ./buildOr, handle the
chownon the host side after docker exits, in a separate step:- name: Fix permissions run: sudo chown -R $USER ./build.github/workflows/aarch64-linux-gnu-static.yaml (1)
44-109:⚠️ Potential issue | 🟡 MinorThe docker run migration looks correct, but
matrix.configon Line 50 is undefined.Same issue as in
aarch64-linux-gnu-shared.yaml: the matrix only definesos, so${{ matrix.config }}always expands to empty. The static analysis tool flags this as well.Proposed fix
- echo "config: ${{ matrix.config }}" + echo "os: ${{ matrix.os }}".github/workflows/build-wheels-armv7l.yaml (2)
380-380:⚠️ Potential issue | 🟠 Major
$pis undefined — ALSA paths inSHERPA_ONNX_CMAKE_ARGSwill resolve incorrectly.The variable
$pis used in-DALSA_INCLUDE_DIR=$p/alsa-lib/includeand-DALSA_LIBRARY=$p/alsa-lib/src/.libs/libasound.so, but it is never assigned in this script. Compare with.github/workflows/linux.yamlLine 139 wherep=$PWDis set before use. Without this assignment,$pexpands to an empty string, producing paths like/alsa-lib/includeinstead of the intended absolute path.This appears to be a pre-existing bug, but since this script was re-wrapped in this PR, it's a good opportunity to fix it.
Proposed fix
Add
p=$PWDbefore the line that setsSHERPA_ONNX_CMAKE_ARGS, e.g. after line 376:export SHERPA_ONNX_ALSA_LIB_DIR=$PWD/alsa-lib/src/.libs export SHERPA_ONNX_MAKE_ARGS="VERBOSE=1" export SHERPA_ONNX_ENABLE_ALSA=1 + p=$PWD export SHERPA_ONNX_CMAKE_ARGS="-DCMAKE_C_FLAGS=\"-march=armv7-a -mfloat-abi=hard -mfpu=neon\" -DCMAKE_CXX_FLAGS=\"-march=armv7-a -mfloat-abi=hard -mfpu=neon\" -DSHERPA_ONNX_ENABLE_BINARY=OFF -DSHERPA_ONNX_BUILD_C_API_EXAMPLES=OFF -DSHERPA_ONNX_ENABLE_C_API=ON -DSHERPA_ONNX_ENABLE_WEBSOCKET=OFF -DALSA_INCLUDE_DIR=$p/alsa-lib/include -DALSA_LIBRARY=$p/alsa-lib/src/.libs/libasound.so"#!/bin/bash # Verify whether $p is defined anywhere in this workflow file rg -n '\bp=' .github/workflows/build-wheels-armv7l.yaml # Also check if the same pattern is in other armv7l-related files rg -n 'DALSA_INCLUDE_DIR=\$p' --glob '*.yaml' --glob '*.yml'
368-368:⚠️ Potential issue | 🟡 MinorFix incorrect architecture path in LD_LIBRARY_PATH for armv7l build.
The LD_LIBRARY_PATH at lines 368-369 references
build/bdist.linux-aarch64/wheel/..., which is incorrect for an armv7l-targeted build. This directory won't exist in the armv7l Docker container, making the path entry ineffective. The correct path should reference the armv7l architecture instead of aarch64. Since libraries are already copied directly to the wheel package, this may not cause immediate failures, but the incorrect path is misleading and should be corrected to match the actual build architecture.
🤖 Fix all issues with AI agents
In @.github/workflows/build-wheels-aarch64-rknn.yaml:
- Around line 44-51: The workflow contains dead echo statements referencing an
undefined matrix variable (echo "config: ${{ matrix.config }}") in the "Build
sherpa-onnx" step (and the similar echo later), which should be removed to avoid
confusion; edit the workflow to delete those echo lines (both occurrences) so
the step no longer prints the non-existent matrix.config, leaving the docker run
and bash -c logic intact.
In @.github/workflows/linux-jni.yaml:
- Line 73: Update the inconsistent release tag: the release step that uploads
the binary to k2-fsa/sherpa-onnx (the release step under the csukuangfj owner)
still uses tag: v1.12.11—change that tag value to v1.12.25 so both the jar
release and the binary release use the same tag; look for the release step
mentioning k2-fsa/sherpa-onnx (and the tag: key) and update its tag value
accordingly.
🧹 Nitpick comments (2)
.github/workflows/release-dart-package.yaml (1)
84-84: Leftover commented-out# if: falseon the aarch64 job.This appears to be a debugging toggle. Consider removing it to keep the workflow clean, or if it's intentional for toggling, add a comment explaining its purpose.
.github/workflows/linux-jni.yaml (1)
84-151: Docker run migration looks correct.Volume mount, image, and
bash -cwrapping are properly structured. The Java and ALSA setup inside the container is preserved.Note: The hardcoded
JAVA_HOMEpath on Line 103 (java-11-openjdk-11.0.23.0.9-2.el7_9.x86_64) is brittle — any package update will break it. Consider a dynamic lookup likeexport JAVA_HOME=$(dirname $(dirname $(readlink -f $(which java))))after installation, if feasible.
| repo_name: k2-fsa/sherpa-onnx | ||
| repo_token: ${{ secrets.UPLOAD_GH_SHERPA_ONNX_TOKEN }} | ||
| tag: v1.12.11 | ||
| tag: v1.12.25 |
There was a problem hiding this comment.
Tag version inconsistency: Line 73 uses v1.12.25 but Line 203 still uses v1.12.11.
Both release steps upload to k2-fsa/sherpa-onnx under the csukuangfj owner condition. The jar release tag was updated to v1.12.25, but the binary release tag on Line 203 was not updated. If both should target the same release, Line 203 needs updating as well.
Proposed fix
At Line 203:
- tag: v1.12.11
+ tag: v1.12.25🤖 Prompt for AI Agents
In @.github/workflows/linux-jni.yaml at line 73, Update the inconsistent release
tag: the release step that uploads the binary to k2-fsa/sherpa-onnx (the release
step under the csukuangfj owner) still uses tag: v1.12.11—change that tag value
to v1.12.25 so both the jar release and the binary release use the same tag;
look for the release step mentioning k2-fsa/sherpa-onnx (and the tag: key) and
update its tag value accordingly.
Summary by CodeRabbit
Chores
Build Improvements