Conversation
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving error handling, code robustness, and shell script safety across multiple components. The changes enhance subprocess result checking, fix shell script quoting/variable handling issues, and improve the package installation process with progress feedback.
Key changes:
- Enhanced error handling for subprocess operations in
voice_extractor.pyby checking return values and providing specific error messages - Fixed critical shell script issues including unquoted variables, use of
command -vinstead ofwhich, and proper variable initialization - Improved package installation with individual package processing and progress tracking
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| lib/classes/voice_extractor.py | Added subprocess result checking and improved error handling for audio processing operations |
| lib/classes/device_installer.py | Fixed conditional logic for Jetson GPU detection and relocated numpy version checking |
| ebook2audiobook.sh | Improved shell script robustness with proper quoting, variable initialization, and safer command execution |
| ebook2audiobook.cmd | Enhanced Windows batch file to install packages individually with progress tracking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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.
Error message is incomplete and provides no actionable information. Consider including details about what went wrong, such as process exit code or stderr output if available from the SubprocessPipe object.
| error = f'normalize_audio() SubprocessPipe Error:' | |
| # SubprocessPipe indicates a failure; include any available diagnostics. | |
| details = [] | |
| if hasattr(proc, "returncode"): | |
| details.append(f"exit code={getattr(proc, 'returncode')}") | |
| stderr = getattr(proc, "stderr", None) | |
| if stderr: | |
| try: | |
| if isinstance(stderr, (bytes, bytearray)): | |
| stderr_text = stderr.decode("utf-8", errors="replace") | |
| else: | |
| stderr_text = str(stderr) | |
| except Exception: | |
| stderr_text = str(stderr) | |
| details.append(f"stderr={stderr_text}") | |
| detail_str = f" ({'; '.join(details)})" if details else "" | |
| error = f'normalize_audio() SubprocessPipe Error during ffmpeg conversion{detail_str}' |
| msg = 'Voice track isolation successful' | ||
| return True, msg | ||
| else: | ||
| error = f'_demucs_voice() SubprocessPipe Error:' |
There was a problem hiding this comment.
Error message is incomplete and provides no actionable information. Consider including details about what went wrong, such as process exit code or stderr output if available from the SubprocessPipe object.
| error = f'_demucs_voice() SubprocessPipe Error:' | |
| details_parts = [] | |
| returncode = getattr(proc, "returncode", None) | |
| if returncode is not None: | |
| details_parts.append(f"returncode={returncode}") | |
| stderr = getattr(proc, "stderr", None) | |
| if stderr: | |
| details_parts.append(f"stderr={stderr}") | |
| stdout = getattr(proc, "stdout", None) | |
| if stdout: | |
| details_parts.append(f"stdout={stdout}") | |
| details = "; ".join(details_parts) if details_parts else "No additional SubprocessPipe details available." | |
| error = f'_demucs_voice() SubprocessPipe Error: {details}' |
| msg = 'Audio normalization successful!' | ||
| return True, msg | ||
| else: | ||
| error = f'normalize_audio() SubprocessPipe Error:' |
There was a problem hiding this comment.
Error message is incomplete and provides no actionable information. Consider including details about what went wrong, such as process exit code or stderr output if available from the SubprocessPipe object.
|
|
||
| function check_desktop_app { | ||
| if [[ " ${ARGS[*]} " == *" --headless "* || has_no_display -eq 1 ]]; then | ||
| if [[ " ${ARGS[*]} " == *" --headless "* ]] || ! has_no_display; then |
There was a problem hiding this comment.
The logic appears inverted. The function has_no_display returns 0 (success/true) when a display IS present, so ! has_no_display will be true when there's NO display, which contradicts the intended behavior of checking for desktop app viability. Consider changing to || has_no_display without the negation.
| 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.
Variable CURRENT_PYVENV should be quoted consistently with the defensive quoting pattern used elsewhere. Change to \"$CURRENT_PYVENV\".
| echo -e "\e[31m=============== Error: Current python virtual environment detected: $CURRENT_PYVENV..\e[0m" | |
| echo -e "\e[31m=============== Error: Current python virtual environment detected: \"$CURRENT_PYVENV\"..\e[0m" |
| 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 |
There was a problem hiding this comment.
The condition checks if CURRENT_PYVENV is empty, but CURRENT_PYVENV is never assigned before this check (it's initialized as empty at line 5). This should check current_pyvenv (lowercase) which is set in lines 795 and 797, then assign to CURRENT_PYVENV.
| 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 | |
| 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 | |
| else | |
| CURRENT_PYVENV="$current_pyvenv" |
No description provided.