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

"support" bundling AppImage when building in a container #822

Merged
merged 2 commits into from
Jul 13, 2020
Merged

"support" bundling AppImage when building in a container #822

merged 2 commits into from
Jul 13, 2020

Conversation

guss77
Copy link
Contributor

@guss77 guss77 commented Jul 13, 2020

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

  • Bugfix
  • Feature
  • 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:

  • It's submitted to the dev branch and not the latest branch
  • 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.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

As discussed in this AppImage bug report, Using appimagetool under Docker (and friends) as an executable won't work because AppImage executes its bundle by FUSE mounting the embedded squashfs file system - and this does not work in a container becase FUSE is a kernel feature that is hard (and likely not a good idea anyway) to access from a container.

As the above discussion recommends, it is actually simple to workaround the problem by manually extracting the AppImage file system to a local directory and running it from there.

This drive-by PR adds a test using lsmod to see if we have fuse supported available in the kernel, and if not - uses the --appimage-extract workaround to run the AppImage tool. I have tested this patch by building tauri-theia under docker.

The lsmod detection is probably not ideal, but I'm not sure what other detection method would work better - I thought using an environment variable to trigger the new behavior, but Docker itself does not provide any such identifying environment variable so discovery by builders might be a problem.

Let me know if this feature is interesting but you want a different implementation and I can see what I can do better.

guss77 added 2 commits July 13, 2020 16:38
When running with no FUSE support available - for example, in a container - use `--appimage-extract` to run the AppImage tool.
@guss77 guss77 requested a review from a team July 13, 2020 14:31
Copy link
Member

@nothingismagick nothingismagick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't test this - but I love the idea and totally appreciate your PR!!! ❤️

(also really awesome that you vetted it on theia!!!)

@lucasfernog & @nklayman - what do you think.

@lucasfernog
Copy link
Member

Do you have the dockerfile you used to try it?

@guss77
Copy link
Contributor Author

guss77 commented Jul 13, 2020

Do you have the dockerfile you used to try it?

Working on it here: https://github.com/guss77/tauri-theia/blob/dev/Dockerfile

I'm having some problems with Github Actions (first time for me), but it builds fine locally.

@lucasfernog
Copy link
Member

Hmm I didn't see any problems with GitHub Actions and AppImage, but maybe you could use this as a guide if you're testing it: https://github.com/tauri-apps/tauri-action

@guss77
Copy link
Contributor Author

guss77 commented Jul 13, 2020

Hmm I didn't see any problems with GitHub Actions and AppImage

Don't worry about that - my problems with Github Actions are definitely my own doing, and unrelated to AppImage, Tauri or Theia ;-)

@lucasfernog
Copy link
Member

Looks good, thank you for your contribution!

@lucasfernog lucasfernog merged commit 3337677 into tauri-apps:dev Jul 13, 2020
@guss77 guss77 deleted the patch-1 branch July 13, 2020 15:58
@nothingismagick
Copy link
Member

@jbolda - is there a way to add this to the changelog post merge?

@lucasfernog
Copy link
Member

Yeah we just need to commit a changefile.

@jbolda
Copy link
Member

jbolda commented Jul 13, 2020

The change file will technically reference the wrong PR / commit, but you can point back to the correct one in the commit message.

@lucasfernog
Copy link
Member

Totally forgot about the PR/commit on the changelog :(

@guss77
Copy link
Contributor Author

guss77 commented Jul 13, 2020

Maybe add that to the PR contribution guidelines?

@lucasfernog
Copy link
Member

Yeah maybe we just need a GH action to remind all of us.

@jbolda
Copy link
Member

jbolda commented Jul 14, 2020

@guss77 it is one of the check boxes that you checked 😄

Some kind of check would be nice though, but it would be a bit odd to fail if a change file doesn't exist as it is valid not to have one (e.g. updating a readme).

@lucasfernog
Copy link
Member

Maybe just a PR comment or something else

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.

4 participants