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
nudoku: init at 1.0.0 #65251
nudoku: init at 1.0.0 #65251
Conversation
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.
Well, had I bunch of thoughts... :)
mkdir -p {share/man,bin} | ||
''; | ||
meta = { | ||
inherit (s) version; |
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.
We usually don't have version in meta, so not sure if it does anything at all?
''; | ||
meta = { | ||
inherit (s) version; | ||
description = ''A ncurses based sudoku game''; |
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 generally used for multi line strings, so regular "
can be used instead :)
preInstall = '' | ||
mkdir -p {share/man,bin} | ||
''; | ||
meta = { |
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.
We usually add meta = with stdenv.lib; {
to make lines below more concise
meta = { | ||
inherit (s) version; | ||
description = ''A ncurses based sudoku game''; | ||
license = stdenv.lib.licenses.gpl3; |
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 be: license = licenses.gpl3;
inherit (s) version; | ||
description = ''A ncurses based sudoku game''; | ||
license = stdenv.lib.licenses.gpl3; | ||
maintainers = [ "Manuel Reinhardt <manuel.reinhardt@neon-cathedral.net>" ]; |
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.
Should refer to an entry in the maintainer list and not be a string :)
@@ -0,0 +1,32 @@ | |||
{stdenv, fetchurl, ncurses}: | |||
let |
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.
We could just drop the let-block and put the values where they belong
ncurses | ||
]; | ||
in | ||
stdenv.mkDerivation { |
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.
Let's add rec to this one so we can use the variables in other places.
]; | ||
in | ||
stdenv.mkDerivation { | ||
inherit (s) name version; |
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.
pname = "nudoku";
version = "1.0.0";
Name will be built from pname and version :)
in | ||
stdenv.mkDerivation { | ||
inherit (s) name version; | ||
inherit buildInputs; |
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.
This one can just be moved from the let block? :)
inherit (s) name version; | ||
inherit buildInputs; | ||
src = fetchurl { | ||
inherit (s) url sha256; |
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.
url = "https://github.com/jubalh/nudoku/releases/download/${version}/${pname}-${version}.tar.xz";
sha256 = "0nr2j2z07nxk70s8xnmmpzccxicf7kn5mbwby2kg6aq8paarjm8k";
That's fine, but why not fetchFromGitHub
? Sometimes there's good reasons to not use it (like they supply something extra in the actual releases).
Thanks for the feedback! I copied most of the stuff from some other package, but maybe I picked a bad one. Do you happen to have a good example that I could base a new package definition on? |
Thank you for your contributions.
|
This was an interesting exercise, but I'm not following up on it any more. |
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)