[Nix-dev] Re: [Nix-commits] SVN commit: nix - 16401 - MarcWeber - in nixpkgs/trunk/pkgs: build-support/fetchurl top-level

[Eelco Dolstra]

Hi,

Marc Weber wrote:

> Log:
> adding NIX_CONTINUE_DOWNLOADS feature, see mkdir comment

> -    if $curl --fail "$url" --output "$out"; then
> -        success=1
> +    cache_file="/var/nix-downloads/$outputHash"

/var/nix-downloads is not a good directory.  See the FHS.  Something like
/var/cache/nix/downloads would be better.  But it would be even better to use a
per-user directory, e.g. ~/.nix-downloads or /var/tmp/nix-downloads-$USER.

> --- nixpkgs/trunk/pkgs/build-support/fetchurl/default.nix	2009-07-16 15:18:24 UTC (rev 16400)
> +++ nixpkgs/trunk/pkgs/build-support/fetchurl/default.nix	2009-07-16 15:18:26 UTC (rev 16401)
> @@ -1,7 +1,7 @@
>  # Argh, this thing is duplicated (more-or-less) in Nix (in corepkgs).
>  # Need to find a way to combine them.
>  
> -{stdenv, curl}: # Note that `curl' may be `null', in case of the native stdenv.
> +{stdenv, curl, getConfig ? (x: y : "")}: # Note that `curl' may be `null', in case of the native stdenv.

Why pass getConfig to fetchurl, when you could also have an argument
"continueDownloads"?  (And please don't call it "NIX_CONTINUE_DOWNLOADS", which
doesn't match the naming convention for variables/attributes.)  Or, if you want
to make this runtime configurable, use $NIX_CONTINUE_DOWNLOADS from the
environment of the caller (via impureEnvVars).

> +  # set this to any value to make nix dowload into /var/nix-downloads/$hash
> +  # so that it can continue a half finished download after a shudown,
> +  # susupend to disk, shutdown etc
> +  # for this to work you have to run
> +  # d=/var/nix-downloads; mkdir $d; chgrp nixbld $d; chmod g+x $d;
> +  # once
> +  # defaulting to enabled because continuing takes place only if $d exists
> +  # and has proper permissions

Could you spend some more attention on editing comments like these for
spelling/grammar/interpunction/formatting?  It would make it a lot easier to read.

> +  NIX_CONTINUE_DOWNLOADS = getConfig ["NIX_CONTINUE_DOWNLOADS"] "1";

I'm against enabling this because you need to perform the commands above to
create /var/nix-downloads.  So this breaks fetchurl for everybody until they do so.

-- 
Eelco Dolstra | http://www.st.ewi.tudelft.nl/~dolstra/