fix(install.sh): fix Python regex escaping and improve pattern matching#896
Conversation
- Fix Python regex escaping in resolve_from_release_api() - remove incorrect backslash escapes inside character classes that caused DMG asset matching to fail on macOS fallback path - Use =~ regex operator instead of == for more reliable pattern matching in bash conditionals - Add Icon= line to desktop entry for proper icon display on Linux Fixes installation failures on Linux/macOS when using the fallback release API resolver. Co-Authored-By: unn-Known1
📝 WalkthroughWalkthroughThe installer script's asset selection and validation logic is refactored to replace glob patterns with regex-based matching for stricter file validation. macOS filename matching is corrected, and a Linux desktop entry icon field pointing to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/install.sh (1)
478-501:⚠️ Potential issue | 🟠 MajorIcon fix is incomplete —
${bin_dir}/openhuman.pngis never created, so the icon still won't display.The new
Icon=${bin_dir}/openhuman.pngdirective points to a path that this installer never produces:
install_linux()only copies the.AppImageto${app_path}(lines 481-482). There is no step that extracts or downloadsopenhuman.pnginto${bin_dir}.- Per the repo layout, there is no
openhuman.pngasset published — the icons underapp/src-tauri/icons/areicon.png,32x32.png,128x128.png, etc. No release asset namedopenhuman.pngis generated byscripts/release/*either.Net effect: desktop environments will fail to resolve the icon path and fall back to a generic icon — the same behavior as before the change. The PR description claims this fixes icon display, but end users won't see a difference unless an icon is actually placed at that path.
Suggested approaches (pick one):
Extract the bundled icon from the AppImage at install time (AppImages ship a
.DirIcon):♻️ Extract .DirIcon from the AppImage
cp "${DOWNLOAD_PATH}" "${app_path}" chmod +x "${app_path}" + # Extract bundled icon so the .desktop Icon= entry resolves. + ( + cd "${TMP_DIR}" + "${app_path}" --appimage-extract '.DirIcon' >/dev/null 2>&1 || \ + "${app_path}" --appimage-extract 'usr/share/icons/hicolor/256x256/apps/*.png' >/dev/null 2>&1 || true + ) + if [ -f "${TMP_DIR}/squashfs-root/.DirIcon" ]; then + cp -L "${TMP_DIR}/squashfs-root/.DirIcon" "${bin_dir}/openhuman.png" || true + fiPublish and download a dedicated
openhuman.pngrelease asset alongside the AppImage, thencurlit into${bin_dir}before writing the desktop file.Install the PNG into the XDG icon theme directory and use a named icon instead of an absolute path:
Icon=openhumanwith the PNG copied to
${HOME}/.local/share/icons/hicolor/256x256/apps/openhuman.pngand (optionally)gtk-update-icon-cacheinvoked.Option 1 is self-contained and doesn't require release-pipeline changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 478 - 501, install_linux() currently copies the AppImage to ${app_path} but never extracts an icon, so update the install path after copying ${DOWNLOAD_PATH} (or when not in DRY_RUN) to extract the embedded .DirIcon from the AppImage into ${bin_dir}/openhuman.png (e.g. run the AppImage with --appimage-extract to produce squashfs-root and copy squashfs-root/.DirIcon to ${bin_dir}/openhuman.png, then clean up), then write the desktop file using Icon=${bin_dir}/openhuman.png; ensure this extraction step runs only when DRY_RUN is false and handles failures gracefully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/install.sh`:
- Around line 478-501: install_linux() currently copies the AppImage to
${app_path} but never extracts an icon, so update the install path after copying
${DOWNLOAD_PATH} (or when not in DRY_RUN) to extract the embedded .DirIcon from
the AppImage into ${bin_dir}/openhuman.png (e.g. run the AppImage with
--appimage-extract to produce squashfs-root and copy squashfs-root/.DirIcon to
${bin_dir}/openhuman.png, then clean up), then write the desktop file using
Icon=${bin_dir}/openhuman.png; ensure this extraction step runs only when
DRY_RUN is false and handles failures gracefully.
…ng (tinyhumansai#896) Co-authored-by: unn-Known1 <unn-known1@users.noreply.github.com>
2 participants
Summary
Fixes multiple bugs in
scripts/install.shthat cause installation failures on Linux/macOS.Bugs Fixed
1. Python Regex Escaping (Critical)
Location:
resolve_from_release_api()functionFixed incorrect backslash escapes inside Python regex character classes that caused DMG asset matching to fail on macOS fallback path:
2. Bash Pattern Matching
Changed from
==glob matching to=~regex operator for more reliable file type detection:3. Desktop Entry Icon
Added
Icon=directive to Linux desktop entry for proper application icon display.Impact
Fixes installation failures on Linux and macOS when using the fallback GitHub Releases API resolver.
Testing
Closes installation failures reported for Linux x86_64.
Summary by CodeRabbit
Bug Fixes
New Features