ledger-live-desktop: init at 1.12.0 #62753
Conversation
| ''; | ||
|
|
||
| meta = with stdenv.lib; { | ||
| description = "Ledger Live (Desktop)"; |
There was a problem hiding this comment.
The description shouldn't really be the name of the package. I found "The companion to your Ledger hardware wallet" on their website but that's more of a slogan so there is almost certainly something better.
There was a problem hiding this comment.
@alexarice this is the description pasted from their github repo
There was a problem hiding this comment.
meta.description should:
Be capitalized.
Not start with the package name.
Not have a period at the end.
This is what contributing.md says and it doesn't seem like a helpful description, even if it is the description on their github
| buildInputs = [ appimage-run ]; | ||
|
|
||
| unpackPhase = ":"; | ||
| preInstall = ":"; |
There was a problem hiding this comment.
@alexarice like this?
There was a problem hiding this comment.
like
|
You will need to squash the commits into one adding yourself as a maintainer and one adding the package at some point |
thedavidmeister
force-pushed
the
2019-06-06-ledger-live-desktop
branch
from
908bce7
to
4cf3187
Compare
|
@alexarice squashed, we good? |
|
I have already approved and cannot do anything else |
Update pkgs/applications/altcoins/ledger-live-desktop/default.nix Co-Authored-By: Benjamin Hipple <bhipple@protonmail.com> add preInstall and postInstall for ledger-live-desktop updated longDescription for ledger-live-desktop resolving feedback
lukateras
force-pushed
the
2019-06-06-ledger-live-desktop
branch
from
4cf3187
to
659e3f7
Compare
|
@GrahamcOfBorg build ledger-live-desktop |
| description = '' | ||
| The companion to your Ledger hardware wallet | ||
| Easily set up and manage your Ledger device | ||
| ''; |
There was a problem hiding this comment.
description should probably fit a single line. This is what https://nixos.org/nixos/packages.html will end up rendering.
| The companion to your Ledger hardware wallet | ||
| Easily set up and manage your Ledger device | ||
| ''; | ||
| homepage = https://www.ledger.com/live; |
| cp $src $out/share/${pname} | ||
| echo "#!${runtimeShell}" > $out/bin/${pname} | ||
| echo "${appimage-run}/bin/appimage-run $out/share/${pname}" >> $out/bin/${pname} | ||
| chmod +x $out/bin/${pname} $out/share/${pname} |
There was a problem hiding this comment.
This is something makeWrapper is a good fit for, as an example:
nixpkgs/pkgs/applications/science/logic/tlaplus/default.nix
Lines 23 to 24 in 92a047a
| }: | ||
|
|
||
| stdenv.mkDerivation rec { | ||
| github-user = "LedgerHQ"; |
There was a problem hiding this comment.
Variables should use camelCase (unlike package names, which use hyphens), see Nixpkgs manual: https://nixos.org/nixpkgs/manual/#sec-syntax
Also, it would make more sense to have it inlined into url, given this is not something that other Nixpkgs packages do.
FTR, if you are interested into something more convenient than fetchurl for fetching GitHub releases, it might make sense to have something like fetchGitHubRelease with options similar to those found in fetchFromGitHub.
|
@GrahamcOfBorg build ledger-live-desktop |
lukateras
changed the title
|
@yegortimoshenko i'm happy with your changes, let's merge :) |
Motivation for this change
Adds ledger live desktop, the app that manages ledger S/X/blue
https://www.ledger.com/
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)