[Nix-dev] Re: [Nix-commits] SVN commit: nix - 14015 - raskin - in nixpkgs/trunk/pkgs: tools/graphics tools/graphics/cuneiform top-level

[Eelco Dolstra]

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/