fix(release): bundle sharun AppImage loader#2307
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds helpers to detect sharun-based AppImages and bundle a host dynamic loader into AppDir/lib when missing, integrates that into strip_one_appimage so repack occurs if loader was added or graphics libs removed, switches tracking to MODIFIED_PATHS for selective re-signing, and adds a smoke-test step for Ubuntu 24.04. ChangesSharun dynamic linker bundling and modification tracking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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. 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}
aqilaziz
force-pushed
the
codex/2297-appimage-sharun-loader
branch
from
4260df2 to
b7a3e41
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/release/strip-appimage-graphics-libs.sh (1)
147-150: ⚡ Quick winConsider failing for unsupported architecture when sharun is detected.
Per PR objectives, packaging should fail if the build host cannot provide the required loader. Currently, unsupported architectures print a WARNING but continue, potentially shipping a broken AppImage that will still fail with "Interpreter not found!".
For consistency with the
exit 1on line 160 (missing host loader), consider also failing here:Proposed change
local loader_name if ! loader_name="$(appimage_loader_name)"; then - echo "[strip-libs] WARNING: unsupported AppImage architecture for sharun interpreter check" >&2 - return 1 + echo "[strip-libs] ERROR: AppImage uses sharun but architecture is unsupported; cannot determine required loader" >&2 + exit 1 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release/strip-appimage-graphics-libs.sh` around lines 147 - 150, The current check that calls appimage_loader_name and sets loader_name returns 1 on unsupported architecture, which only prints a warning and lets packaging continue; change this to fail the packaging consistently by exiting with a non-zero status (use exit 1) when loader_name cannot be determined and sharun will be used. Update the conditional block that currently echoes the WARNING and returns 1 so that it instead logs the same message and calls exit 1 (matching the behavior used later for the missing host loader), ensuring the script stops packaging a broken AppImage; refer to the appimage_loader_name call and the loader_name variable in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/release/strip-appimage-graphics-libs.sh`:
- Around line 147-150: The current check that calls appimage_loader_name and
sets loader_name returns 1 on unsupported architecture, which only prints a
warning and lets packaging continue; change this to fail the packaging
consistently by exiting with a non-zero status (use exit 1) when loader_name
cannot be determined and sharun will be used. Update the conditional block that
currently echoes the WARNING and returns 1 so that it instead logs the same
message and calls exit 1 (matching the behavior used later for the missing host
loader), ensuring the script stops packaging a broken AppImage; refer to the
appimage_loader_name call and the loader_name variable in your change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb454933-a334-442f-ba6d-80ffd61992e8
📒 Files selected for processing (2)
docs/RELEASE-MANUAL-SMOKE.mdscripts/release/strip-appimage-graphics-libs.sh
✅ Files skipped from review due to trivial changes (1)
- docs/RELEASE-MANUAL-SMOKE.md
aqilaziz
force-pushed
the
codex/2297-appimage-sharun-loader
branch
from
b7a3e41 to
1e5b736
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/release/strip-appimage-graphics-libs.sh`:
- Around line 80-92: The new aarch64 branch in appimage_loader_name introduces
arm64 detection but strip_one_appimage still assumes x86_64 paths/roots, which
can lead to partial mutations; either remove/undo the aarch64/arm64 case from
appimage_loader_name or update strip_one_appimage to handle arm64 fully: detect
architecture via appimage_loader_name or MATRIX/APPIMAGE_TARGET, set correct lib
roots and loader names (use ld-linux-aarch64.so.1), and ensure
rebuild/relocation logic and path substitutions use arm64-specific directories;
alternatively make appimage_loader_name return failure for non-x86 to fail fast
until strip_one_appimage gains full arm64 support.
- Around line 147-150: When appimage_loader_name fails to detect a loader (the
block using loader_name="$(appimage_loader_name)"), make this condition fatal
instead of returning 1 so a sharun AppImage doesn't proceed; replace the current
'return 1' with an immediate script failure (e.g. 'exit 1') and ensure the
existing warning message remains so that when uses_sharun_launcher has matched
the absence of a loader aborts the whole run rather than being treated as
"nothing to do".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5687a01-cfa1-42ca-8ea7-da1a6b65c44d
📒 Files selected for processing (2)
docs/RELEASE-MANUAL-SMOKE.mdscripts/release/strip-appimage-graphics-libs.sh
✅ Files skipped from review due to trivial changes (1)
- docs/RELEASE-MANUAL-SMOKE.md
aqilaziz
force-pushed
the
codex/2297-appimage-sharun-loader
branch
2 times, most recently
from
407d353 to
183e4e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/release/strip-appimage-graphics-libs.sh`:
- Around line 96-100: The candidates array used in the ld-linux-x86-64.so.2)
case doesn't include /usr/lib, so add "/usr/lib/$loader_name" to the candidates
list (the variable candidates and the case label ld-linux-x86-64.so.2) so the
script will find the loader installed at /usr/lib on some distros; update the
candidates array ordering as needed to include that path.
- Around line 125-128: The loop that hard-codes "$appdir/sharun",
"$appdir/bin/OpenHuman", and "$appdir/usr/bin/OpenHuman" will miss differently
named entrypoints (AppRun or other binaries); update the logic in
strip-appimage-graphics-libs.sh so it dynamically discovers candidate entry
binaries instead of using those literal names — e.g. check AppRun if present,
parse the .desktop Exec or iterate over all files under "$appdir"/{,usr/}bin and
the top-level AppDir root for executable ELF files, filter non-ELF/text files,
and run the loader-injection code against each discovered candidate (replace the
hard-coded list in the for candidate in ... loop with this discovery). Ensure
you still preserve existing checks that the file is executable and an ELF binary
before attempting injection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0db7898a-be8a-4d80-98ac-7caf2e12f6ab
📒 Files selected for processing (2)
docs/RELEASE-MANUAL-SMOKE.mdscripts/release/strip-appimage-graphics-libs.sh
aqilaziz
force-pushed
the
codex/2297-appimage-sharun-loader
branch
from
183e4e8 to
4e71ea9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@senamakel #2307 当前 checks 22 success / 2 skipped / 0 pending / 0 failure，CodeRabbit 最新 review approved，review threads 0 unresolved。这个修 v0.54.0 AppImage |
|
huge thanks @aqilaziz for getting the sharun loader bundled into the appimage 🙌 super clean fix making sure the repack+resign kicks in when the loader is the only thing missing. really appreciate you keeping the mesa/libdrm stripping intact too, merged and shipped! |
3 participants
Summary
sharunneeds inside the Linux AppImage when the post-build AppImage cleanup runs.Interpreter not found!regression.Problem
OpenHuman_0.54.0_amd64.AppImagecan exit immediately on Ubuntu 24.04 withInterpreter not found!because the AppImage contains asharunlauncher but nolib/ld-linux-x86-64.so.2.sharuninterpreter.Solution
sharun-style launchers by checking the extracted AppDir launcher binaries for theInterpreter not found!marker.squashfs-root/lib/when missing.sharunbut the build host cannot provide the required loader.Submission Checklist
## Related— N/A: no feature matrix row.docs/RELEASE-MANUAL-SMOKE.md)Closes #NNNin the## RelatedsectionImpact
ld-linux-x86-64.so.2.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
codex/2297-appimage-sharun-loader4260df24f5355a63df02e748e276586ebed0c53aValidation Run
pnpm --filter openhuman-app format:check— N/A: no frontend files changed.pnpm typecheck— N/A: no TypeScript files changed.ensure_sharun_interpretercopiesld-linux-x86-64.so.2into a synthetic sharun AppDir.C:\Program Files\Git\bin\bash.exe -n scripts/release/strip-appimage-graphics-libs.sh;git diff --check.Validation Blocked
command:Full AppImage rebuild and launch on Ubuntu 24.04.error:No produced Linux AppImage artifact is available in this local Windows workspace.impact:CI release packaging will exercise the changed script; manual release smoke checklist now covers the Ubuntu 24.04 launch regression.Behavior Changes
sharundynamic linker when needed.Interpreter not found!on affected AppImages.Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Chores