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

[Michael Raskin]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Eelco Dolstra wrote:
>> +a :  
> 
> I'd really recommend stating the function arguments explicitly:
> 
> {fetchurl, cmake, imagemagick, patchelf}: ...

This time reading leading part of the function is enough to see what
needs to be passed..

>> +let 
>> +  fetchurl = a.fetchurl;
> 
> Is this necessary?  "fetchurl" is used only once, so aliasing it doesn't seem
> very useful.

It is part of template, of course. And it allows me to use the same
fetcher script to update all kinds of packages.

>> +  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.

I do not want to write its contents twice. I also want it to be obvious
what dependencies are. So this style allows it to be declared in the header.

>> +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.

That is not the point. The point is that you do not need to edit version
everywhere in expression - and if you specify it wrong, fetchurl fails.

>> +  inherit buildInputs;
>> +  configureFlags = [];
> 
> If the configureFlags are empty they can just be omitted.
Template code. Also makes it easier for someone unfamiliar with
expression to add the flags.

>> +  /* 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 :-)
It is from template.. Using a template helps enormously not to forget
writing description, for example..

>> +  cuneiform = builderDefsPackage (import ../tools/graphics/cuneiform) {
>> +    inherit cmake patchelf;
>> +    imagemagick=imagemagick;
> 
> This should be "inherit cmake patchelf imagemagick";
That would be harder to change into imagemagickBig if anyone would want.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iQEcBAEBAgAGBQJJkZk/AAoJEE6tnN0aWvw3vBAIAI1E7phlJpN+Mq5CxoarjbMz
B65aEklYf4HJk/uyn7/LA9nm3t77UWCypF1vmHYKVzlM3urvZWkef7I0sLLNq1VV
qFNuyd1t8mpiL5mfrnO97UIS1eDZKQ10yd8q6cVLQYU5oeDEfmQBgZDijNfc2bNI
DaJVHyQVEf3SahHC1xlLBXbii3DfSWVYMHYyqBVPXumIusUU4qHSjZORAbbRXJrc
7C4Yf8Qk/q9XOzBDyzlm3/7aB7Fbix6Pry39aq3zhsYzKiD9qkMIQg0gQQgBGkdU
ULtfSRMF0A9h2EvHoDzIDguwwxT2FgMMFF/LsoZeprPJNNxRx8HM9TIwqfksEtw=
=2ll6
-----END PGP SIGNATURE-----