[Nix-dev] Re: [Nix-commits] SVN commit: nix - 12645 - MarcWeber - in nixos/trunk: installer system

[Eelco Dolstra]

Hi,

Marc Weber wrote:

> Author: MarcWeber
> Date: 2008-08-17 01:11:57 +0000 (Sun, 17 Aug 2008)
> New Revision: 12645
> 
> You can view the changes in this commit at:
>    https://svn.nixos.org/viewvc/nix?rev=12645&view=rev
> 
> Removed:
>    nixos/trunk/installer/nixos-checkout.sh
> Modified:
>    nixos/trunk/installer/default.nix
>    nixos/trunk/installer/nixos-rebuild.sh
>    nixos/trunk/system/options.nix
> 
> Log:
> rewritten nixos-checkout code.
> You can now define multiple repositories. See options.nix

Some remarks:

Since the code for generating nixos-checkout is fairly large, it's better to put
it in a separate file (especially since it now kind of intersperses the
definitions of nixos-install, nixos-rebuild etc).

> +  inherit (pkgs.lib) id all whatis escapeShellArg concatMapStrings concatMap
> +  mapAttrs concatLists flattenAttrs filter;

Mind the indentation (there should be two more spaces in front of mapAttrs).

> +  f = repo : attrs :
> +    assert (__isAttrs attrs);

Please don't use the __primop primops, they were never really indented to be
used directly.  You can instead use builtins.primop, or if using it frequently
put "inherit (builtins) primop...;" somewhere.

> +    assert (repo + "" == repo);

This is a very strange assertion :-)

> +    if (! (attrs ? type)) then 
> +      throw "repo type is missing of : ${whatis attrs}"
> +    else if attrs.type == "svn" then
> +      let a = { # add svn defaults
> +                url = "https://svn.nixos.org/repos/nix/${repo}/trunk";
> +                target = "/etc/nixos/${repo}";
> +              } // attrs; in

Variable names like "a" aren't very descriptive.  Idem for that top-level
function "f".

Also indenting an attribute set all the way after the opening "{" is (IMHO) not
a good idea since it's very brittle: if you change the code in front of it (e.g.
change "let a = " to "let someOtherVarName = ") then you have to re-indent
everything.

> +    let t = escapeShellArg attrs.target; in

"escapeShellArg" really scares me, since the Nix expression language was never
intended as a general-purpose programming language.  It has neither the features
nor the performance to support complicated string operations very well.  Such
things are probably best done in builders.

> +    repos = {
> +      nixos = mkOption {
> +        default = [ { type  = "svn"; }  ];

Shouldn't there be a url here?

> +        description = "The nixos repository from which the system will be build. 
> +                       nixos-checkout will update all defined repositories,
> +                       nixos-rebuild will use the the first item which has
> +                       the attribute default = true falling back to the
> +                       first item. the type defines the repository tool added

Some typos there ("the the", "the type", "nixos").  Also the description isn't
quite accurate: we don't *build* from a repository but obtain the sources from a
repository, and nixos-checkout doesn't update repositories but working copies :-)

Also, is that "default = true" / fallback thing needed?

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