Conversation
There was a problem hiding this comment.
Pull request overview
This pull request includes improvements to error handling, code robustness, and installation processes across Python and shell scripts. The changes enhance subprocess error checking, improve shell script safety with better variable handling, and refactor package installation to provide better progress feedback.
Key changes:
- Enhanced error handling in voice extraction with SubprocessPipe return value checking
- Improved shell script robustness with
set -euo pipefailand defensive variable references - Refactored package installation to show per-package progress instead of bulk installation
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| lib/classes/voice_extractor.py | Added SubprocessPipe return value checks for better error detection; improved error handling with safer stderr decoding; reorganized voice_track path construction |
| lib/classes/device_installer.py | Fixed Jetson GPU detection logic; improved numpy version compatibility handling for PyTorch 2.2.2 and earlier; removed conda-specific scikit-learn installation |
| ebook2audiobook.sh | Added shell safety with set -euo pipefail; improved variable existence checks with ${VAR:-} pattern; added per-package installation progress bar; enhanced virtual environment detection |
| ebook2audiobook.cmd | Added per-package installation progress display; improved INSTALLED_LOG creation logic to skip in BUILD_DOCKER mode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| error = f'normalize_audio() ffmpeg.Error: {stderr_msg}' | ||
| return False, error | ||
| except FileNotFoundError as e: | ||
| error = 'normalize_audio() FileNotFoundError: {e} Input file or FFmpeg PATH not found!' |
There was a problem hiding this comment.
Missing a period after the f-string interpolation placeholder. The error message should have proper punctuation between the exception message and the description.
| error = 'normalize_audio() FileNotFoundError: {e} Input file or FFmpeg PATH not found!' | |
| error = f'normalize_audio() FileNotFoundError: {e}. Input file or FFmpeg PATH not found!' |
| if [[ -z "${CURRENT_PYVENV:-}" ]]; then | ||
| PYTHON_PATH="$(command -v python 2>/dev/null || true)" | ||
| if [[ ( -n "${CONDA_PREFIX:-}" && "$PYTHON_PATH" == "${CONDA_PREFIX:-}/bin/python" ) || \ | ||
| ( -n "${VIRTUAL_ENV:-}" && "$PYTHON_PATH" == "${VIRTUAL_ENV:-}/bin/python" ) ]]; then | ||
| CURRENT_PYVENV="${CONDA_PREFIX:-${VIRTUAL_ENV:-}}" | ||
| fi | ||
| fi | ||
| # Output result if a virtual environment is detected | ||
| if [[ -n "$current_pyvenv" ]]; then | ||
| echo -e "\e[31m=============== Error: Current python virtual environment detected: $current_pyvenv..\e[0m" | ||
| if [[ -n "$CURRENT_PYVENV" ]]; then | ||
| echo -e "\e[31m=============== Error: Current python virtual environment detected: $CURRENT_PYVENV..\e[0m" |
There was a problem hiding this comment.
The condition checks if CURRENT_PYVENV is empty, but then assigns to CURRENT_PYVENV (uppercase), while earlier in the code at lines 794-797 it assigns to 'current_pyvenv' (lowercase). This inconsistency will cause the variable check to always pass since CURRENT_PYVENV won't be set by the earlier assignments. Either all references should use CURRENT_PYVENV or all should use current_pyvenv.
| python -m pip install --upgrade --no-cache-dir -r "%SCRIPT_DIR%\requirements.txt" | ||
| set count_pkg=0 | ||
| for /f "usebackq delims=" %%P in ("%SCRIPT_DIR%\requirements.txt") do ( | ||
| if not "%%P"=="" if not "%%P:~0,1%"=="#" ( |
There was a problem hiding this comment.
The variable reference uses incorrect syntax. It should be '%%P:~0,1' not '%%P:~0,1%'. The closing percent sign should not be there when checking if the first character is a hash for comment detection.
| if not "%%P"=="" if not "%%P:~0,1%"=="#" ( | |
| if not "%%P"=="" if not "%%P:~0,1"=="#" ( |
| msg = 'Conversion to .wav format for processing successful' | ||
| return True, msg | ||
| else: | ||
| error = f'normalize_audio() SubprocessPipe Error:' |
There was a problem hiding this comment.
The error message references 'normalize_audio()' but this code is in the '_convert2wav' method. The error message should reference the correct function name '_convert2wav()'.
| error = f'normalize_audio() SubprocessPipe Error:' | |
| error = f'_convert2wav() SubprocessPipe Error:' |
| msg = 'Conversion to .wav format for processing successful' | ||
| return True, msg | ||
| else: | ||
| error = f'normalize_audio() SubprocessPipe Error:' |
There was a problem hiding this comment.
The error message ends with a colon but provides no additional information. Consider either adding specific error details or removing the trailing colon to make the message complete.
| error = f'normalize_audio() SubprocessPipe Error:' | |
| error = 'normalize_audio() SubprocessPipe Error' |
| error = f'_demucs_voice() SubprocessPipe Error:' | ||
| return False, error |
There was a problem hiding this comment.
The error message ends with a colon but provides no additional information. Consider either adding specific error details or removing the trailing colon to make the message complete.
| msg = 'Audio normalization successful!' | ||
| return True, msg | ||
| else: | ||
| error = f'normalize_audio() SubprocessPipe Error:' |
There was a problem hiding this comment.
The error message ends with a colon but provides no additional information. Consider either adding specific error details or removing the trailing colon to make the message complete.
| error = f'normalize_audio() SubprocessPipe Error:' | |
| error = 'normalize_audio() SubprocessPipe Error' |
| if not "%%P"=="" if not "%%P:~0,1%"=="#" ( | ||
| set /a count_pkg+=1 | ||
| echo [!count_pkg!] Installing %%P | ||
| python -m pip install --upgrade --no-cache-dir %%P || exit /b 1 |
There was a problem hiding this comment.
The per-line installation loop uses %%P from requirements.txt directly in the python -m pip install ... %%P command without any quoting or escaping, so special characters in a requirement (e.g. &, |, > or &&) will be interpreted by cmd.exe instead of being passed safely to pip. An attacker who can modify requirements.txt could add a line like somepkg & calc.exe, causing arbitrary commands to execute under the user’s account when this script runs. To prevent command injection, ensure each requirement line is treated as a single argument to pip (for example by properly quoting %%P or delegating parsing back to pip install -r so the shell never interprets requirement contents).
| python -m pip install --upgrade --no-cache-dir %%P || exit /b 1 | |
| python -m pip install --upgrade --no-cache-dir "%%P" || exit /b 1 |
No description provided.