-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
[wip] appimageTools: add a new buildAppImage wrapper #82158
Conversation
What's the use case exactly? Nixpkgs might not be the best vehicle for distributing appimages. |
45ee8eb
to
1e5ac3e
Compare
@edolstra : the use case is people trying to manage their appimage with nixpkgs, of generally hard to package or closed source software, and we spend time reviewing and writing such appimages that could be acceptable in such way i guess. @GrahamcOfBorg build appimagePackages.retroshare |
1e5ac3e
to
e2fb3e1
Compare
@GrahamcOfBorg build appimagePackages.colobot |
Btw there are 1800 PR in nixpkgs, we don't lack of reviewing, so if people can wait for a difficult app to be package and merge this way, i think it doesn't hurt. I think the opposite, there is a choice to start using an appimage that is identify as it (prefixed) then do a better integration at time. |
pkgs/top-level/appimage-packages.nix
Outdated
self = appimagePackages; | ||
appimagePackages = with self; { | ||
|
||
buildAppImage = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to just put packages in to top-level/all-packages.nix
rather than one big expression file. Hiding packages in attribute set also might lead to people packaging an application twice. After all packages should be built without using app images if possible. Having an abstraction is ok, but currently it does not seem to provide enough to justify the abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"all packages should be built without using app images if possible"
we are very agree on that, but you know it's not always possible.
currently it does not seem to provide enough to justify the abstraction.
I made all my possible to put the interesting stuff in appimageTools instead, but i think i get something useful here too, we still can someapp = appimagePackages.someapp to top-level/all-packages.nix , people would identify clearly it's not a native one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes most sense to merge it as is for now and introduce other features as needed, i've some on plan but not yet ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Make it obvious that by doing someapp = appimagePackages.someapp
looks like a good idea on the second thought - we should do this for all packages with no exception for visibility reasons though. However I would use callPackage in appimage-packages.nix
for all in this for two reasons: It allows to override dependencies using override
and one file per package is easier to maintain. We had at some point a huge pkgs/top-level/python-packages.nix
which was not fun at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes the main interest was to simplify the maintainer and package work . Perhaps i better move this buildappimage to appimagetools, and let people creating an one file per package, but the drawback is it's harder to maintain for me as author of buildAppImage. since i've some other nixpkgs job on the fire, i put this on wip again, let's this maturate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we have buildAppImage
provided as a library function implementing all the somewhat repetitive bits of packaging a appimage as a nix derivation, and then still callPackage
it from all-packages.nix
?
I still like to be able to have packages in all-packages.nix
, and be able to build them by building the top-level attribute.
Also, I like all the metadata we have, and that each package is in its own directory.
I still don't really see a point in a separate appimage-packages.nix
, and having them inside a appimagePackages.*
namespace - I'd say if there's some software only (reasonably) obtainable by extracting it from an appimage, then package it by extracting the appimage, but let's not make the way these are added into nixpkgs more alien than another regular package.
We already do that for some software, be it extracting a .deb
, snap
or whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have ~150 recurseIntoAttrs in all-packages.nix, think in a first time that was possible to use that possibility for simplest appimages (not all). But a drawback is to not be able to use other callPackages like qt5.callPackage ( deprecated ? https://nixos.org/nix-dev/2017-February/022900.html ), libsForQt5 ... but i think that's not a real issue since this should make it unnecessary by the buildFHSUserEnv. ( what about python3Packages ? pkgsi686Linux ? ) . So i wonder if i have interest to provide a appimages.callPackage . So for the moment i will just move buildAppImage in appimageTools.
pkgs/top-level/appimage-packages.nix
Outdated
} // attrs.meta; | ||
}); | ||
|
||
beakerbrowser = buildAppImage rec { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you using any of these packages? I would prefer if we have a maintainer and actual user who keeps the packages up to date/in a working state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i need examples to make the thing, and i took some that will find maintainer soon, @kamilchm ... or people needs it or colobot i just think was fun to provide as it to children.
Please, take in count that i'm working a lot on the appimage integration on nixpkgs, and i can give some hints why it's cleaner when we have such things in our repo :
i know there still improvement to be done, specially automatise desktop integration in installPhase or extraInstallCommands, but the best is ennemy of the good. |
@bignaux please keep this PR focused on the actual thing it's proposing - otherwise it becomes harder to review. As explained in #82158 (comment), I don't really see the point of adding a I'd propose closing this PR, and refactoring some of the existing derivations that already extract appimages to use the tooling you propose. That'd make it easier to understand what it'd be improving. |
i'd propose you to not close it since it is done by main author of appimagetools and 3rd of https://github.com/AppImage/AppImageKit/graphs/contributors ... |
Well, it was a suggestion. You're the author of the PR, so you can close it for yourself, if you agree. If you disagree, you can keep it open ;-) I just wanted to clarify this PR (as it is currently) is about adding a Creating nix packages from appimages is one of the many ways we obtain software, next to fetching from source and building, extracting binary tarballs, extracting debian packages, extracting snaps, … - while we of course prefer building from source where possible. I don't think nixpkgs should be an appimage registry - hence my reservation against a |
This sounds like something that should be in a separate repository (or eventually, flake). The monolithic nixpkgs repo is already too big and too much of a bottleneck to contributors. |
I would agree that we should not mirror every possible appimage applications in existence especially if we have them as a normal package already. However as I understand the author, it is more about improving our existing appimage infrastructure in order to support packages that we would not have otherwise at all in NixOS, since they would be too hard to package. |
Well, in that case this should be a new library function, plus some changes to the existing packages to make use of that function - instead of the addition of |
Ok. Let's follow this approach. |
I'd prefer change the title and continue on that branch since this issue/PR have been linked on others threads ... I don't see any good reason to close it. Where i did the stuff to simplify the appimage packaging is not so important compare to the real discussion i'd have (that never happens anyway, each time it's only review trailing whitespace) is about what user need in it, and you seems not to be the users. |
@bignaux can you then update the PR title and description with what this is about? |
Yes i'll do it quickly as i split and rewrite the PR. |
e2fb3e1
to
572948e
Compare
This contributor has since been banned from the org, closing. |
Motivation for this change
Provide a new bundle of appimage without pollute the nixpkgs with small appimageTools.wrapType derivation.
in a first time, discussion was about adding a recurseIntoAttrs method and a ./pkgs/top-level/appimage-packages.nix on the example of haxe-packages.nix . Now we add a new method on appimageTools.
will close #38037 , #76849 ...
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)cc @worldofpeace
cc @jtojnar
cc @tilpner
cc @Mic92
cc @dtzWill