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/ _______________________________________________ nix-dev mailing list [email protected] https://mail.cs.uu.nl/mailman/listinfo/nix-dev
