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

appimage-run improvements #57169

Merged
merged 4 commits into from Apr 20, 2019
Merged

appimage-run improvements #57169

merged 4 commits into from Apr 20, 2019

Conversation

timokau
Copy link
Member

@timokau timokau commented Mar 9, 2019

Motivation for this change

Fixes #57127. CC @tilpner.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@timokau
Copy link
Member Author

timokau commented Mar 9, 2019

What do you think about adding gdb to the package list? It can be useful for digikam, but I don't know how common it is with appimages. I wish PATH was kept, but I don't see a straightforward way to do that.

Copy link
Member

@tilpner tilpner left a comment

Choose a reason for hiding this comment

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

Looks good. I'm slightly against including gdb (but the environment is already huge, maybe it's the wrong place to save space).

Consider addressing the style nitpick, and then let's merge it.

@@ -10,7 +10,15 @@ in buildFHSUserEnv (fhsArgs // {

runScript = writeScript "appimage-exec" ''
#!${runtimeShell}
if [ $# -le 0 ]; then
Copy link
Member

Choose a reason for hiding this comment

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

The number of arguments passed couldn't be less than 0, so I would prefer -eq here.

@@ -10,7 +10,15 @@ in buildFHSUserEnv (fhsArgs // {

runScript = writeScript "appimage-exec" ''
#!${runtimeShell}
if [ $# -le 0 ]; then
echo "Uage: $0 FILE [OPTION...]"
Copy link
Member

Choose a reason for hiding this comment

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

Should be "Usage"

@tm-nihonsuki
Copy link

Forgive me if this is the wrong place to mention this. Just run appimage-run on the latest Synfigstudio and it failed with:
error while loading shared libraries: libltdl.so.7: cannot open shared object file: No such file or directory
From what I've read around the problem, appimages assume a core of available library modules that don't need to be included with the app. Could it be that appimage-run is out of sync with that list?
Thanks for your patience.

@timokau
Copy link
Member Author

timokau commented Mar 28, 2019

From what I've read around the problem, appimages assume a core of available library modules that don't need to be included with the app. Could it be that appimage-run is out of sync with that list?

Yes, that is exactly the problem this PR is supposed to solve. However libltdl.so (which is part of libtool.lib in nixpkgs) isn't on the list.

I'd be in favor of including it anyways, since the main purpose of appimage-run is convenience and if there exists one package that expects libltdl.so, there are probably more. What do you think @tilpner?

@tm-nihonsuki
Copy link

If that list is canonical, is it not the responsibility of the packager to make sure that all other libraries are part of the package? If so, perhaps I should nudge the chaps at Synfig...

@tilpner
Copy link
Member

tilpner commented Mar 29, 2019

@timokau It is fairly easy for users to add their own packages to appimage-run (appimage-run.override { extraPkgs = p: with p; [ ... ]; }, so I don't want to increase closure size to get a single AppImage working. Packages to be added should fulfill at least one of:

  • Mentioned in official exclude list
  • Used in more than one publicly available AppImage
  • Already used in practically every desktop NixOS system, so addition would be free

The problem with adding every library on-demand is that it hurts people who want to just run well-behaved AppImages (and they can't do anything about it, since there currently is no way to remove packages), and packages could never be removed in the name of backwards-compatibility.

(That said, I didn't necessarily follow these rules for the initial list, I just copied it from the Steam chroot)

@tm-nihonsuki
Copy link

@tilpner for the newbie's sake, would that override go in configuration.nix?

@tilpner
Copy link
Member

tilpner commented Mar 29, 2019

@tm-nihonsuki Yes, it would go wherever you used appimage-run previously.

E.g. instead of

{ pkgs, ... }: {
  users.users.nihon.packages = with pkgs; [
    appimage-run
  ];
}

you would do

{ pkgs, ... }: {
  users.users.nihon.packages = with pkgs; [
    (appimage-run.override { extraPkgs = p: with p; [ ... ]; })
  ];
}

@timokau
Copy link
Member Author

timokau commented Mar 30, 2019

If that list is canonical, is it not the responsibility of the packager to make sure that all other libraries are part of the package? If so, perhaps I should nudge the chaps at Synfig...

Yes, they should have included libtool. They probably didn't notice because it is a dependency of pulseaudio and therefore practically any system will have it anyway.

@timokau
Copy link
Member Author

timokau commented Mar 30, 2019

@timokau It is fairly easy for users to add their own packages to appimage-run (appimage-run.override { extraPkgs = p: with p; [ ... ]; }, so I don't want to increase closure size to get a single AppImage working. Packages to be added should fulfill at least one of:

* Mentioned in official exclude list

* Used in more than one publicly available AppImage

* Already used in practically every desktop NixOS system, so addition would be free

Most libraries expected by an appiamge probably fulfill this point, since it was presumabliy tested on multiple systems and nobody complained about the library missing. The libraries present on a typical nixos system shouldn't be too different to those common to on a typibal ubuntu/gentoo/arch system.

At least in this case:

$ nix-store -q --referrers /nix/store/k46mlii63fmq3hkwyfrx3sqhfg1lsfl1-libtool-2.4.6-lib
/nix/store/k46mlii63fmq3hkwyfrx3sqhfg1lsfl1-libtool-2.4.6-lib
/nix/store/3kh4nmq46j3yq80dgbajbh9xypga6107-graphviz-2.40.1
/nix/store/ikmidhjfqibdni9vc2jan4l1qd4fbd4c-libpulseaudio-12.2
/nix/store/cxqmvc76n7j22rp7id12rm58im4a5sgc-libcanberra-0.30
/nix/store/d31ibx1348p2ypqy4vr461v1xgjg6zfx-libcanberra-0.30
/nix/store/db4ajimxvqnsnvgs8rh4mysc67hk3591-openldap-2.4.46
/nix/store/f173pxwhqfvq13x32kp03wlx7pk85h27-libtool-2.4.6
/nix/store/f26kp3s64nczmkjr790kdv0iw2kwb9ls-libcanberra-0.30
/nix/store/wfyqd3rgkj8w1sjdg8zbidb34g6jh55g-libgphoto2-2.5.17
/nix/store/xyr3lgfczb1bdbxvydyd16my6vrmd2ca-libcanberra-0.30
/nix/store/ymdrqjn6n80iigyz037h1036623sbp0c-pulseaudio-12.2

The problem with adding every library on-demand is that it hurts people who want to just run well-behaved AppImages (and they can't do anything about it, since there currently is no way to remove packages), and packages could never be removed in the name of backwards-compatibility.

Seems like we have slightly different usecases. For me appimage-run is more of a one-off thing, no permanent solution. In that case convenience trumps all.

@tm-nihonsuki
Copy link

That's very helpful. I've been able to get Synfigstudio to run with appimage-run after adding libtools and a few more libX libraries. However, I cannot run the app from the start menu entry created when I first ran appimage-run on this image. What is the best way to proceed. Is it safe to simply delete the squashfs created and re-run the appimage-run on the image with the new configuration?

@tilpner
Copy link
Member

tilpner commented Mar 30, 2019

I agree that appimage-run is a convenience for (hopefully™) temporary usecases where a Nix package is not available. Small closure size factors into that convenience though, so it is worth attempting to keep it low (I realize it's huge already).

And yes, the requirements (which I quite arbitrarily typed up, and don't need to be applied too strictly) are fairly loose. I hadn't checked for libtool, and it seems fine to add to this PR.

@tilpner
Copy link
Member

tilpner commented Mar 30, 2019

@tm-nihonsuki Yes, it's safe to delete ~/.cache/appimage-run. You can also look at using appimageTools for a less temporary setup.

@tm-nihonsuki
Copy link

Okay, I can only run from the downloaded image with appimage-run. If I try to run the linked AppRun in the squashfs, I get a 'No such...' error. What have I overlooked?

@tilpner
Copy link
Member

tilpner commented Mar 30, 2019

I haven't linked to anything about AppRun, I'm not sure what you mean.

The whole point of appimage-run is getting around the "No such file" error, you can't just execute the extracted files from a normal shell.

@tm-nihonsuki
Copy link

When I first ran appimage-run and the synfig image, I was asked if I wanted to integrate appimages into the desktop environment, IIRC. It is probably this that is responsible for the links being placed in the menu. As I'm unsure what appimage-run does, I may have conflated the two coincident events. appimage-run clearly inflates the archive. How, then, does it run the app: is there a script it builds and executes?

@timokau
Copy link
Member Author

timokau commented Apr 20, 2019

I added libtool and addressed the review comments. Good to merge @tilpner?

@tilpner
Copy link
Member

tilpner commented Apr 20, 2019

LGTM!

@timokau timokau merged commit 0e8177a into NixOS:master Apr 20, 2019
@timokau timokau deleted the appimage-improvements branch April 20, 2019 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

appimage-run can't run digikam
4 participants