Code Review Bench PR #14390 - fix(bundler): inline linuxdeploy plugin scripts#2
Code Review Bench PR #14390 - fix(bundler): inline linuxdeploy plugin scripts#2ketkarameya wants to merge 2 commits intobase_pr_14390_20260125_8485from
Conversation
Code Review 🚫 Blocked 0 resolved / 4 findingsThree critical bugs prevent compilation and runtime correctness: missing 🚨 Bug:
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
![gitar-bot-staging[bot]](https://avatars.githubusercontent.com/in/812680?s=60&v=4){kind=small}
|
|
||
| fs::write(path, data)?; | ||
| fs::set_permissions(path, fs::Permissions::from_mode(0o770))?; | ||
| fs::set_permissions(path, fs::Permissions::from_mode(0o770)).map_err(FileWriteError::Permissions)?; |
There was a problem hiding this comment.
🚨 Bug: set_permissions before write fails on non-existent files
In write_and_make_executable(), fs::set_permissions is called before fs::write. However, fs::set_permissions requires the file to already exist on disk — it will fail with a "No such file or directory" error if the file doesn't exist yet.
Every caller of this function is gated by a !path.exists() check (except the GTK case, which has a separate bug). This means set_permissions is called on a path that is guaranteed not to exist, and it will always fail with FileWriteError::Permissions.
The original code had the correct order: write first, then set permissions. The reordering introduced in this PR breaks the function entirely.
Was this helpful? React with 👍 / 👎
| fs::set_permissions(path, fs::Permissions::from_mode(0o770)).map_err(FileWriteError::Permissions)?; | |
| fs::write(path, data).map_err(FileWriteError::Write)?; | |
| fs::set_permissions(path, fs::Permissions::from_mode(0o770)).map_err(FileWriteError::Permissions)?; |
- Apply suggested fix
| let gtk = tools_path.join("linuxdeploy-plugin-gtk.sh"); | ||
| if !gtk.exists() { | ||
| let data = download("https://raw.githubusercontent.com/tauri-apps/linuxdeploy-plugin-gtk/master/linuxdeploy-plugin-gtk.sh")?; | ||
| if gtk.exists() { |
There was a problem hiding this comment.
🚨 Bug: GTK plugin condition inverted — never written on fresh install
At line 243, the condition if gtk.exists() means the inlined GTK plugin script is only written when the file already exists. On a fresh build (the common case), the file won't exist, so the plugin is never written. Compare with the gstreamer plugin on line 249 which correctly uses if !gstreamer.exists().
The intent of inlining is to replace the previous download-based approach. The condition should be negated to if !gtk.exists() so the plugin is written when it's missing, consistent with all other tools in this function.
Without this fix, AppImage builds requiring GTK (which is the default — see line 202 where --plugin gtk is always passed) will fail because the GTK plugin script won't be present.
Was this helpful? React with 👍 / 👎
| if gtk.exists() { | |
| if !gtk.exists() { |
- Apply suggested fix
| "https://github.com/tauri-apps/binary-releases/releases/download/apprun-old/AppRun-{arch}" | ||
| ))?; | ||
| write_and_make_executable(&apprun, data)?; | ||
| write_and_make_executable(&apprun, &data)?; |
There was a problem hiding this comment.
🚨 Bug: Missing From impl — code won't compile
write_and_make_executable returns Result<(), FileWriteError>, but it's called with ? inside prepare_tools which returns crate::Result<PathBuf>. The ? operator requires From<FileWriteError> for crate::Error, but no such implementation exists in the crate's error module. crate::Error only has From<io::Error> (not From<FileWriteError>).
This is a compilation error. Either:
- Add
impl From<FileWriteError> for crate::Error, or - Add
.map_err(...)at each call site to convertFileWriteErrorintocrate::Error, or - Revert
write_and_make_executableto returnstd::io::Result<()>(which auto-converts via the existingFrom<io::Error>)
Option 3 is simplest if the custom error type isn't needed elsewhere.
Was this helpful? React with 👍 / 👎
- Apply suggested fix
| } | ||
|
|
||
| let linuxdeploy_arch = if arch == "i686" { "i383" } else { arch }; | ||
| let linuxdeploy = tools_path.join(format!("linuxdeploy-{linuxdeploy_arch}.AppImage")); |
There was a problem hiding this comment.
⚠️ Bug: URL/filename mismatch for i686 linuxdeploy download
On line 235-238, the local filename still uses linuxdeploy_arch (which maps "i686" → "i383"), but the download URL now uses arch directly ("i686"). This creates a mismatch on i686 targets:
- Downloads:
linuxdeploy-i686.AppImage(usingarch) - Saves as:
linuxdeploy-i383.AppImage(usinglinuxdeploy_arch)
The original code consistently used linuxdeploy_arch for both. The PR description says this is a fix to use {arch} instead of {linuxdeploy_arch}, but only the URL was changed — the local path on line 236 still uses linuxdeploy_arch.
If the upstream binary is named linuxdeploy-i383.AppImage, then the URL should use linuxdeploy_arch too. If the upstream name is linuxdeploy-i686.AppImage, then the local path should also use arch. Either way, both must be consistent.
Was this helpful? React with 👍 / 👎
- Apply suggested fix
3 participants
Benchmark PR for Gitar evaluation
Original PR: agentic-review-benchmarks#2
fix(bundler): inline linuxdeploy plugin scripts
Summary by Gitar
linuxdeploydownload URL from{linuxdeploy_arch}to{arch}to prevent using wrong architecture identifierif !gtk.exists()toif gtk.exists()to properly inline plugin script when file existsFileWriteErrorenum withWriteandPermissionsvariants to distinguish file operation failure typesThis will update automatically on new commits.