Desktop: Remove libfuse2 dependency from AppImage by using modern runtime#15034
Desktop: Remove libfuse2 dependency from AppImage by using modern runtime#15034JGCarroll wants to merge 4 commits intolaurent22:devfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe installation script's libfuse2 dependency check is now conditional, performed only for Joplin versions 3.6.8 and earlier, whilst the desktop app's build configuration is updated with an electron-builder version upgrade and restructured Windows signing and Linux desktop entry settings. Changes
🚥 Pre-merge checks | ✅ 4✅ 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the desktop build tooling to use a newer Electron Builder and configures AppImage packaging to use a modern runtime, aiming to remove the hard dependency on libfuse2 on newer Linux distributions. It also updates the Linux install/update script to only check for libfuse2 when downloading older AppImage releases that still require it.
Changes:
- Upgrade
electron-builderto26.8.1(and associated dependency graph updates). - Adjust Electron Builder config for Windows signing and Linux desktop entry generation; specify an AppImage toolset/runtime.
- Update the Linux install/update script to gate the
libfuse2dependency check based on the release version.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
yarn.lock |
Updates lockfile to reflect the electron-builder upgrade and new transitive dependencies. |
packages/app-desktop/package.json |
Bumps electron-builder and updates build config (Windows signing options, Linux desktop entry, AppImage toolset). |
package.json |
Adjusts Yarn resolutions, notably removing depd@1.x overrides. |
Joplin_install_and_update.sh |
Moves the libfuse2 check behind a version gate intended to apply only to older releases. |
| ## Check for libfuse2 for Joplin versions below 3.6.8, which transitioned to a new AppImage runtime without libfuse2 dependency | ||
| if [[ $(compareVersions "$RELEASE_VERSION" "3.6.8") -le 0 ]]; then |
There was a problem hiding this comment.
The version gate for the libfuse2 check is off by one: compareVersions returns 0 when versions are equal, so -le 0 will still enforce libfuse2 for release 3.6.8 even though the comment says versions below 3.6.8. This defeats the purpose of switching runtimes in 3.6.8+. Update the condition to only run the libfuse2 check when RELEASE_VERSION is strictly less than 3.6.8 (and consider adjusting the comment wording accordingly).
| ## Check for libfuse2 for Joplin versions below 3.6.8, which transitioned to a new AppImage runtime without libfuse2 dependency | |
| if [[ $(compareVersions "$RELEASE_VERSION" "3.6.8") -le 0 ]]; then | |
| ## Check for libfuse2 only for Joplin versions earlier than 3.6.8, which transitioned to a new AppImage runtime without libfuse2 dependency | |
| if [[ $(compareVersions "$RELEASE_VERSION" "3.6.8") -lt 0 ]]; then |
There was a problem hiding this comment.
Checked the compareVersions function manually in Bash:
james@James-Desktop:~/joplin$ compareVersions 3.6.9 3.6.8
1
james@James-Desktop:~/joplin$ compareVersions 3.6.8 3.6.8
0
james@James-Desktop:~/joplin$ compareVersions 3.6.7 3.6.8
-1
Ideal logic (now that 3.6.8 is is already released) is:
if [[ $(compareVersions "$RELEASE_VERSION" "3.6.8") -le 0 ]]; then
so would update comments to reflect 3.6.9 instead
| "node-gyp@npm:^9.0.0": "11.2.0", | ||
| "depd@npm:^2.0.0": "patch:depd@npm%3A2.0.0#~/.yarn/patches/depd-npm-2.0.0-b6c51a4b43.patch", | ||
| "depd@npm:~2.0.0": "patch:depd@npm%3A2.0.0#~/.yarn/patches/depd-npm-2.0.0-b6c51a4b43.patch", | ||
| "depd@npm:~1.1.2": "patch:depd@npm%3A2.0.0#~/.yarn/patches/depd-npm-2.0.0-b6c51a4b43.patch", | ||
| "depd@npm:2.0.0": "patch:depd@npm%3A2.0.0#~/.yarn/patches/depd-npm-2.0.0-b6c51a4b43.patch", | ||
| "depd@npm:^1.1.2": "patch:depd@npm%3A2.0.0#~/.yarn/patches/depd-npm-2.0.0-b6c51a4b43.patch", | ||
| "depd@npm:^1.1.0": "patch:depd@npm%3A2.0.0#~/.yarn/patches/depd-npm-2.0.0-b6c51a4b43.patch", | ||
| "formidable@npm:^2.0.1": "patch:formidable@npm%3A2.1.2#~/.yarn/patches/formidable-npm-2.1.2-40ba18d67f.patch" |
There was a problem hiding this comment.
Removing the depd@npm:^1.x resolution means the dependency graph can now pull in unpatched depd@1.1.2 (as seen in the updated yarn.lock). The depd 1.x line uses eval/new Function unless patched, which can conflict with the project's strict CSP in the desktop app (no unsafe-eval) and reintroduces the behavior the existing depd patches were meant to avoid. Consider restoring a resolution that applies the depd-npm-1.1.2-*.patch (or otherwise ensuring depd 1.x can’t be pulled in).
There was a problem hiding this comment.
@personalizedrefrigerator
I've rebuilt this in a fresh environment just now without any cached build artifacts,
- Reverting this section entirely works fine in a clean build, and would mean I haven't touched anything, so would be my preferred option
- I did try changing the patch to be instead be
depd-npm-1.1.2-b0c8414da7.patchas you suggested in the other review, this patch doesn't cleanly apply and so won't build
Would it be best to just revert the 3 lines above for now and ignore the other patch file?
There was a problem hiding this comment.
Would it be best to just revert the 3 lines above for now and ignore the other patch file?
Yes. That should work!
There was a problem hiding this comment.
Looks like the CI is showing the error I had when I tried this yesterday now those patches are restored.
I'm tempted to think there might be a problem with Electron Builder itself, possibly a race condition of some sort?
(But at least it shows I wasn't entirely mad!)
⨯ production dependency not found parent=agentkeepalive dependency=depd version=^1.1.2
⨯ Production dependency depd not found for package agentkeepalive failedTask=build stackTrace=Error: Production dependency depd not found for package agentkeepalive
There was a problem hiding this comment.
Thanks for checking! Something else to try might be to replace the entire depd resolutions section with
"depd": "patch:depd@npm%3A2.0.0#~/.yarn/patches/depd-npm-2.0.0-b6c51a4b43.patch",Update: Possibly related: electron-userland/electron-builder#9641, electron-userland/electron-builder#9625
There was a problem hiding this comment.
Tried that tweak locally in a clean build, 3 failures during yarn dist, same as the error/CI above. Out of curiosity I limited the build to 1 CPU core (by removing the rest from the VM) - same error.
There was a problem hiding this comment.
I'm looking into this locally (Fedora 43 VM).
There was a problem hiding this comment.
Here's what seems to be happening:
electron-builderdetects that Joplin is using the Yarn package manager.yarndoesn't provide a CLI command to list all production-only dependencies recursively (see [Feature] yarn info - add possibility to list only production packages yarnpkg/berry#5117), soelectron-builderfalls back to using NPM for detecting dependencies.- This doesn't work, so
electron-builderfalls back to scanningnode_modulesandpackage.json.- This approach doesn't include logic to check the
resolutionsfield in the toplevelpackage.json(presumably becauseresolutionsis yarn-specific).
- This approach doesn't include logic to check the
I was able to work around this by patching electron-builder's app-builder-lib. However, I'm now encountering build failures related to the deb step (specifically, fpm).
Update: Another problem: The built .AppImage is 2.5 GiB.
Update 2: The fpm and .AppImage size issues were caused by a development build of packages/onenote-converter. After rebuilding in release mode, the AppImage is only 204 MiB.
There was a problem hiding this comment.
I'm just here curious why the behaviour seems to be intermittent and sometimes builds fine and sometimes doesn't? I'd personally have expected it to be broken more consistently given the above, but the CI shows it failing on Windows/Mac yet working on Linux fine for example.
Thanks for the assist anyway! LMK if/when/how you want to merge those in here; meanwhile I'll test out the .AppImage itself on a few VM's tomorrow for the practical side of things.
Edit: Checked the CI below and it's because the ones that pass aren't running yarn dist to fail on, now I'm more confused why it randomly did work again for me earlier without patching app-builder-lib?
Edit 2: Your upgrade-electron-builder-try-2 branch works fine for me locally!
There was a problem hiding this comment.
I've opened a new pull request, with the patch: #15043
(Testing help would be appreciated!)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Joplin_install_and_update.sh`:
- Around line 182-197: The comment and the compareVersions check for
RELEASE_VERSION are inconsistent: decide whether version 3.6.8 requires
libfuse2; if 3.6.8 still requires libfuse2, update the comment to say "for
Joplin versions 3.6.8 and below" (or "up to and including 3.6.8"); if 3.6.8 does
NOT require libfuse2, change the comparison in the if that calls compareVersions
"$RELEASE_VERSION" "3.6.8" from -le 0 to -lt 0 so the check only runs for
versions strictly less than 3.6.8; ensure the comment and the condition
surrounding RELEASE_VERSION and compareVersions are aligned.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 068afcb1-1eaa-4e93-ac9b-52088e4dbf55
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
Joplin_install_and_update.shpackage.jsonpackages/app-desktop/package.json
💤 Files with no reviewable changes (1)
- package.json
See laurent22#15034 Co-authored-by: JGCarroll <JGCarroll@users.noreply.github.com>
|
Closed in favour of #15043 |
Re-creating #15032 to make it easier to review after the original suddenly ballooned in noise (sorry about that)
Original PR text:
This PR:
Removing the libfuse2 requirement doesn't make this "self-contained" as people would call it, the requirement instead shifts elsewhere.
e.g., running on Fedora 43 in WSL2:
which was resolved with sudo dnf install fuse fuse-libs - this doesn't mean that libfuse2 itself is being used however, the output above is distinctly different from the error messages before this commit (dlopen error on libfuse2 as opposed to a requirement for a fusermount binary). These tools are essential for a wide variety of other ecosystems (e.g., Flatpak) so can be assumed to be nearly universal.
Marking as draft to encourage some more rigorous testing and also because I need to figure out how to sign the SLA.
Unresolved issues from review in original PR:
depdresolution needs adjustmenthttps://github.com/laurent22/joplin/pull/15032/changes/BASE..6d8ca0fa21f9911664cd313dd0daea629257be71#r3040412718
(Assistance on that one appreciated!)
Second times the charm, hopefully
Test Plan
The AppImage should be checked on a modern distribution to confirm that it works as expected - default configuration with no additional packages beyond what's available on a fresh install. Fedora 44 beta and Ubuntu 26.04 beta seem ideal, neither will have
libfuse2out of the box.The AppImage should be checked on a relatively older but still supported distribution, like Ubuntu 22.04 - default configuration with no additional packages beyond what's available on a fresh install.
The AppImage should be checked on an EOL distribution (e.g., Ubuntu 16.04) - it's likely Electron itself will not work in an environment this old due to glibc & etc, however, we can potentially still gain some insight into
libfuse2vsfusermounteven if the application itself won't run. This can be compared to a v3.5 AppImage to ensure no regression (if v3.5 runs at all).Although this shouldn't have any impact on the application functionality itself, to be thorough, exporting to PDF, opening the print menu, importing a .JEX file, and trying secondary windows seem like some ideal functionality tests where the application does open to ensure basic functionality is unaffected.