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

patchwork: Init at 3.10.1 #46062

Closed
wants to merge 1 commit into from
Closed

patchwork: Init at 3.10.1 #46062

wants to merge 1 commit into from

Conversation

picnoir
Copy link
Member

@picnoir picnoir commented Sep 4, 2018

Motivation for this change

Add patchwork, a decentralized messaging and sharing app built on top of Secure Scuttlebutt (SSB).

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)
  • Fits CONTRIBUTING.md.

Hi,

As you can see, this software is distributed using a "new" (at least to me) archive type called AppImage. As its name states, it runs everywhere, it can install any software on any linux distribution, which sounds great.

Sadly, it does not run on nixos at all :(

I did not wanted to debug the "executable archive" by itself. Instead, I've looked for a way to extract the actual package from the archive in order to install it in a more "traditional" way.

I am extracting the actual package contained in various squashfs embedded in the AppImage using binwalk.

It's a bit hack-ish, but it has the certain advantage to work. I'm open to any improvement on the process.

I've setup a notification to watch any new release of this software: I plan to maintain this package.

Have a nice day!

@GrahamcOfBorg GrahamcOfBorg added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Sep 4, 2018
@cryptix
Copy link
Contributor

cryptix commented Sep 7, 2018

Hey! I'm very familiar with the scuttleverse and would take over the maintainance on this from ninja.

I'm also working on a way to patch ssb' electron apps that doesn't involve AppImages but I need some help with node2nix but that can happen on another discussion.

@picnoir
Copy link
Member Author

picnoir commented Sep 7, 2018

Added @cryptix as maintainer and squashed the commits.

Copy link
Member

@samueldr samueldr left a comment

Choose a reason for hiding this comment

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

I have left technical issues as comments in the files that will or may need resolving with the contribution.

name = "Patchwork";
exec = "patchwork";
icon = "$out/share/patchwork/";
comment = "Decentralized messanging and sharing app";
Copy link
Member

Choose a reason for hiding this comment

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

Typo:

Decentralized messaging and sharing app

categories = "Network;";
};

phases = [ "unpackPhase" "installPhase" "fixupPhase"];
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason to set phases? Most default phases should behave properly when not in presence of a "configure make make" project. AFAIUI, overriding this you could miss some important, but well hidden, free features (I don't have any source for this off the top of my head).

@@ -4521,6 +4521,8 @@ with pkgs;

patchutils = callPackage ../tools/text/patchutils { };

patchwork = callPackage ../applications/networking/scuttlebutt/patchwork { };
Copy link
Member

Choose a reason for hiding this comment

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

We have patchwork-classic = callPackage ../applications/networking/ssb/patchwork-classic { };

Any reason for using scuttlebutt over ssb in the pathname? (Other than missing it.)

Copy link
Member Author

Choose a reason for hiding this comment

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

None, I just totally missed patchwork-classic and the ssb folder ><


phases = [ "unpackPhase" "installPhase" "fixupPhase"];

buildIntputs = [ pythonPackages.binwalk p7zip ];
Copy link
Member

Choose a reason for hiding this comment

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

Since you're referring to ${binwalk}, it doesn't need to be in buildInputs. p7zip either, since it isn't used. Why do I say neither I used? Well, buildIntputs isn't the right name!

It seems binwalk can deal with the 7z dependency by itself, without it being in the PATH. (I additionally counter-verified by removing them locally.)

sha256 = "18dvzh2bfmq320s2qi6cqn0qc32y9yl2wdwj518aq8bds78yqqx2";
};

desktopItem = makeDesktopItem {
Copy link
Member

Choose a reason for hiding this comment

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

desktopItem isn't used, thus the generated desktop file isn't in the output.

$ find result/ -iname '*.desktop' | wc -l
0

You probably need something like the following in the install phase:

ln -s ${desktopItem}/share/applications/* $out/share/applications/

#
appName=$(basename -s .drv $src)
mkdir -p $out/{libexec,bin,share/patchwork}
cd _$appName.extracted
Copy link
Member

Choose a reason for hiding this comment

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

Since this is not a matryoshka extract, there ought to be few files extracted; in this particular case it ends up being one file, especially since you limit the type of extractions!

You can instead use globbing to cd into the right directory:

cd *-Patchwork-*.AppImage.extracted

It should be just as stable as grabbing the name using basename and src.

Alternatively. you can use this trick in unpackPhase:

ln -s $src source
${binwalk} source --quiet -D 'squashfs:.squashfs:7z x %e'

And binwalk would extract to _source.extracted. binwalk won't follow symlinks.

cp ssb-patchwork.png $out/share/patchwork/patchwork.png
cp -r app/* $out/libexec/
chmod a+x $out/libexec/ssb-patchwork
ln -s $out/libexec/ssb-patchwork $out/bin/patchwork
Copy link
Member

Choose a reason for hiding this comment

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

(three preceeding lines)

I'm not sure libexec is the right folder to extract wholesale the appimage. libexec includes internal binaries that are not intended to be executed directly by users or shell scripts. Applications may use a single subdirectory under .../libexec as par the FHS. It's probably better to use .../share/ssb-patchwork (or .../share/patchwork) instead as its own owned directory for misc. purposes.

libxcb, gnome2, nss, nspr, alsaLib, cups, fontconfig, expat, makeDesktopItem}:

let
rpath = lib.makeLibraryPath [
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to leave a reminder, for future contributors, as to how you figured out this list? Sometimes it's brute-force, sometimes it's a clever trick. Not strictly necessary, but a helpful note for future contributors (even future you!).

@samueldr
Copy link
Member

samueldr commented Sep 8, 2018

Additionally, while this is isn't the first AppImage that got contributed in nixpkgs, and definitely not the first binary build of a project, it is always preferred to get a source build going instead of going from a binary build.

Binary builds are generally used when

  • No sources are available (obviously).
  • The binaries have technical merits over a source build (partially open source software, of validated software where source builds differ.)
  • The build system is a mess hampering build from happening (e.g. some Java apps).

I haven't looked closely, but it looks like the README has build instructions.

One could also look at how other distributions (and unix-likes) package theirs:

(Uh, looks like only archlinux has a package. Let's make a second good example of a source-built one!)

I'm not 100% against using AppImages for nixpkgs, but this is probably a discussion better held elsewhere than in a contributions for a specific package. If another contributor feels it's good to merge this (once the misc. cleanups are applied) I won't hold it against them, but as this looks like it could be compiled from source, I strongly lobby for source compilation instead of using the binary build.

@samueldr
Copy link
Member

samueldr commented Sep 8, 2018

I will additionally add that it may be of interest to synchronize efforts with regards to AppImage apps with the other contributor(s) contributing AppImage-based builds. A common framework to extract and make use of AppImages will probably be of value for nixpkgs even if all the packages using AppImage builds end up using source builds; AppImages distribute more than open source packages.

See #44511.

@picnoir
Copy link
Member Author

picnoir commented Sep 8, 2018

Thank you very much for your review.

I privileged the appimage way because I personally had some pretty annoying experience with a npm-based derivation randomly breaking because of some loose upper/lower dependencies bounds in the past.

@cryptix is planning to implement a source-based derivation in a near future.

I don't think we need two duplicate patchwork derivations in the nixpkgs repo. I'm just gonna keep this one in a overlay for now and close this PR. Is this ok for you @samueldr ?

@picnoir picnoir closed this Sep 8, 2018
cryptix added a commit to cryptix/nixpkgs that referenced this pull request Sep 11, 2018
@picnoir picnoir deleted the patchwork branch December 11, 2019 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants