Hi,
Michael Raskin wrote:
Some style comments:
> Added: nixpkgs/trunk/pkgs/tools/graphics/cuneiform/default.nix
> ===================================================================
> --- nixpkgs/trunk/pkgs/tools/graphics/cuneiform/default.nix
> (rev 0)
> +++ nixpkgs/trunk/pkgs/tools/graphics/cuneiform/default.nix 2009-02-09
> 20:37:11 UTC (rev 14015)
> @@ -0,0 +1,39 @@
> +a :
I'd really recommend stating the function arguments explicitly:
{fetchurl, cmake, imagemagick, patchelf}: ...
> +let
> + fetchurl = a.fetchurl;
Is this necessary? "fetchurl" is used only once, so aliasing it doesn't seem
very useful.
> + version = a.lib.getAttr ["version"] "0.6" a;
> + buildInputs = with a; [
> + cmake imagemagick patchelf
> + ];
Below there is a "inherit buildInputs", and buildInputs isn't used anywhere
else. So it can just be declared in the rec directly.
> +in
> +rec {
> + src = fetchurl {
> + url =
> "http://launchpad.net/cuneiform-linux/${version}/${version}/+download/cuneiform-${version}.tar.bz2";;
> + sha256 = "0jgiccimwv1aqp9gzl9937gdlh9zl5qpaygf0n1xcbfd5aqz14ig";
> + };
This fetchurl call is potentially inconsistent: if I supply a version other than
0.6, the sha256 hash won't be correct.
> + inherit buildInputs;
> + configureFlags = [];
If the configureFlags are empty they can just be omitted.
> + /* doConfigure should be removed if not needed */
> + phaseNames = ["doCmake" "doMakeInstall" "postInstall"];
Why the comment about doConfigure? If it's not needed then it's not needed :-)
> + cuneiform = builderDefsPackage (import ../tools/graphics/cuneiform) {
> + inherit cmake patchelf;
> + imagemagick=imagemagick;
This should be "inherit cmake patchelf imagemagick";
--
Eelco Dolstra | http://www.st.ewi.tudelft.nl/~dolstra/
_______________________________________________
nix-dev mailing list
[email protected]
https://mail.cs.uu.nl/mailman/listinfo/nix-dev
