Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(bundler): bundle additional webkit files. patch absolute paths in libwebkit*.so files. fixes #2805,#2689 #2940

Merged
merged 6 commits into from
Dec 9, 2021

Conversation

FabianLars
Copy link
Member

@FabianLars FabianLars commented Nov 23, 2021

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Docs
  • New Binding Issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes. Issue #___
  • No

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

This needs to be tested on different distros before being merged!

Speaking of testing, fedora still won't work because of those permission denied errors (unrelated to this PR).
^Fixed this in the linuxdeploy plugin linked below.

I'm not sure if it actually fixes #2689, but it should because it looks like a version mismatch between the bundled webkit and the system's one.

This is a really ugly fix imo, but i don't really think much can be improved concept wise, but who knows 🤷

Anyway, here goes the explanation and stuff.
Basically we had 2 problems:

  1. we didn't include files that webkit needs at runtime
  2. the paths of those files are hardcoded into the libwebkit*.so file (hardcoded paths are a big no-no in AppImages).

The first issue's fix is pretty easy but also somewhat risky as we have to search for those files as each distro stores them somewhere else. That's the main reason why we need more testing, who knows what absurd paths are out there in the wild...

  • for example:
    • ubuntu: WebKitNetworkProcess, WebKitWebProcess and injected-bundle/libwebkit2gtkinjectedbundle.so in /usr/lib/x86_64-linux-gnu/webkit2gtk-4.0/
    • fedora: WebKitNetworkProcess and WebKitWebProcess in usr/libexec/webkit2gtk-4.0/ and libwebkit2gtkinjectedbundle.so in /usr/lib64/webkit2gtk-4.0/injected-bundle/
      The injectedbundle file doesn't seem to be needed as far as i can see, but it will log an error at launch (without crashing) so probably better to just include it.

The fork of linuxdeploy-plugin-gtk is needed because this plugin will overwrite the libwebkit*.so files and therefore we can't patch those files in template/appimage.sh. -> This one took me too long to notice...

Link to the change: https://github.com/FabianLars/linuxdeploy-plugin-gtk/blob/master/linuxdeploy-plugin-gtk.sh#L330
And yes we have to glob pattern match the file and folder names again because they seem to differ between distros too 😒

We can also move the fork to the tauri org, i don't really care, but i don't expect such a change to make it into upstream at all.
Furthermore i won't even try to get something like this into webkit itself, that's no bueno for my mental health lol.

Update 1

  1. We now use mkdir to create the folders before copying to make them writable. Otherwise only the first file will be copied.
  2. The find command can throw "permission denied" errors on protected folders. Normally, this would be fine as it just continues after throwing them. In this case we need to add || true to not trigger the set -euxo pipefail guard. Unfortunately this will skip the exit on other errors from these commands too, so we can still end up with broken AppImages after a "successful" script execution, but at least they will be logged when invoked with --verbose.

Update 2

Fixed the os error 13 in the gtk plugin which crashed the appimage script on fedora (and probably on other distros too).
This also fixes the appimage folder being undeletable on the following runs which also crashed the bundler but even earlier.

Update 3

Test results are coming in, check comment down below.

How to test

  1. (Optional) Clone the fork and run the examples instead of step 1&2
  2. Install the rust based cli from the fork: cargo install tauri-cli --force --git https://github.com/FabianLars/tauri --branch fix/appimage -> remember to revert/remove this afterwards :)
  3. Use cargo tauri build to build your project, the npm/yarn command won't work. (You may need to use tauri's next branch)
  4. Copy the AppImage to another system (needs to be another distro or at least another version). Alternatively you could temporarily rename the above mentioned files if you find them.

@FabianLars FabianLars requested a review from a team as a code owner November 23, 2021 00:01
@FabianLars FabianLars requested a review from a team November 23, 2021 00:01
@FabianLars FabianLars linked an issue Nov 23, 2021 that may be closed by this pull request
@FabianLars
Copy link
Member Author

FabianLars commented Nov 24, 2021

So i'm getting pretty confident so far, so here are test result i have so far.

Testresults

what i personally tested

  • built on ubuntu 20.04:
    • works on manjaro and fedora 35
  • built on manjaro:
    • works on fedora 35 (can't work on ubuntu 20.04 because of glibc)
  • built on fedora 35
    • works on another (fresh) fedora system lol (glibc is even newer than manjaro's, so i can't test that)

what others tested

@lucasfernog
Copy link
Member

Looking good!

@FabianLars FabianLars changed the title fix(bundler): bundle additional webkit files. patch absolute paths in libwebkit*.so files. fix #2805. fix #2689 fix(bundler): bundle additional webkit files. patch absolute paths in libwebkit*.so files. fixes #2805,#2689 Nov 24, 2021
@FabianLars
Copy link
Member Author

Sooo, i'd like to prepare this PR to be ready for merging, because it looks like it's working pretty reliable and doesn't introduce any regressions and therefore shouldn't need any substantial changes anymore. There are 2 things to talk about first tho.

1) Moving the plugin fork to the tauri org:
Should i transfer it (== 6 commits diff) or create a new fork directly in the org (== 1 commit diff)? I'm fine with both.

2) Possible code changes:
Basically 3 possibilities:

  • 1. We leave it all as it is.
  • 2. We move the template changes into the gtk plugin instead.
    • This can (but shouldn't) reintroduce folder permission issues again, because linuxdeploy and/or the plugin like to f up the folder creation. So maybe we need to add another chmod call 🤷
    • This can (but shouldn't) introduce other issues if the gtk plugin did some funny stuff with the binaries or something (because they were already there when the plugin ran). Sounds really unlikely after i read it out loud...
    • Probably better in a "separation of concerns" way or whatever it's called.
  • 3. We create another plugin just for that webkit stuff
    • Actually, scratch that, i tried it a little bit and it's not worth the hassle:
      • a) it's too risky as in "it's actually less reliable", at least in some simple testing i did after the discord talk.
      • b) we need to duplicate the permission code fixes.
      • c) we still need to fork the gtk plugin which is reason enough imo.
      • also webkitgtk is gtk related enough for 2. to make more sense than 3. i guess

So yeah either 1. or 2., I'm fine with both, but still leaning towards 1. cause it looks like the safest bet 🤔

@lucasfernog
Copy link
Member

I'm fine with a direct transfer of the plugin.

@xyzshantaram
Copy link
Contributor

@FabianLars absolute legend

@TexZeTech
Copy link

So I know this is not the same project but I would really like some help trying to figure it out. I'm using Orca slicer for my klipper printer and I cannot get the Device tab to display the mainsail interface anymore. I believe it's related to one of the issues in this thread, at this point, any and all help would be appreciated.

OrcaSlicer #920

Thank you for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Github Action AppImage build only works on Ubuntu symbol lookup error with the .AppImage on Ubuntu 19.04
4 participants