[Nix-dev] Re: [Nix-commits] SVN commit: nix - 15430 - raskin - nixos/trunk/installer/cd-dvd

[Nicolas Pierron]

Hi Michael,

I had a look at your patch and I don't understand why you are not
using the mkOverride function introduced to tackle your problem.  I
think you may have some problems with your method.

On Sun, May 3, 2009 at 13:37, Michael Raskin <7c6f434c at mail.ru> wrote:
> Log:
> Port main functionality of rescue-cd-configurable into rescue-cd.nix as a set of small fixes. Rogue, manual are now ordinary jobs. It is possible to include build dependencies. It is possible to choose one of a few configurations in the boot menu. Of course, configuration overrides may be passed
>
> Changes:
>
> Modified: nixos/trunk/installer/cd-dvd/rescue-cd.nix
> ===================================================================
> --- nixos/trunk/installer/cd-dvd/rescue-cd.nix  2009-05-03 11:29:08 UTC (rev 15429)
> +++ nixos/trunk/installer/cd-dvd/rescue-cd.nix  2009-05-03 11:37:20 UTC (rev 15430)
>
> +  configuration = baseConfiguration // (configurationOverrides baseConfiguration);
>

1/ The " //" operator to override default settings: Duplicate, Remove?


The operator does not support configuration merges.  The counter
example can be found in such case:

----------------
{

  services = {
    manual = { enable = true; };
    rogue = { enable = true; };
    ..
  };

} // { # override everything under "services"

  services = {
    manual = { enable = false; };
    .. # You have to rewrite everything.
  };

}
----------------

As you have wrote your patch, I am guessing that
"configurationOverrides" will be used to inherit other options like:

----------------
config: {
  services = pkgs.lib.removeAttrs ["manual"] config.services;
}
----------------

This require a better understanding of Nix as you may have to explain
this to new people.  Currently configuration files are shown as
declarative and the action of removing does not feel sane.  Rewriting
everything does not scale.


2/ "configurationOverrides baseConfiguration": CD & DVD configurations
are no longer configuration files.


As started in the modular-nixos branch, all configuration files are
made to have the same pattern which is:

----------------
# Optional arguments.
{pkgs, config, ...}:

# An optional interface body
let
  options = {
    ..
  };
in

# The implementation body
{
  # List of required declarations
  require = [
    options
  ];

  ..
}
----------------

This pattern is useful because it avoid you to remember too much
things.  Your patch allow you to have a slightly different writing
where the optional arguments are replaced by:

----------------
config:

# the rest
..
----------------

First, you add doubt in writers minds and they will trigger us more
often to fix their bugs. Second, your CD configuration can not be
imported by another CD configuration which add a small feature.  This
second point implies that you may have to duplicate configurations or
to factor the configuration in some extra files.


3/ Suggestion

The file nixos/trunk/installer/cd-dvd/rescue-cd.nix contains two things:
- The derivation based on NixOS system.nix.
- The CD base configuration.

I recommend you to move the default CD configuration in its own file
and replace it by the "configuration" argument.  Then you could have a
all-images.nix file which would be similar to all-packages.nix where
all registered CD configurations will be available as a short cut with
the ' -A ' option.

---------------- all-images.nix
let
  mkCDImage = configuration: (import ./cd-build.nix) {
    inherit configuration;
  };
in

{
  rescue = mkCDImage (import ./rescue-cd.nix);
  rescueWithXServer = mkCDImage (import ./rescue-cd-with-xserver.nix);
}
----------------

Where rescue-cd-with-xserver.nix will be a configuration file which
includes the rescue-cd.nix configuration.  Thus you can extend a
configuration without extra Nix operator and you can remove the
override other configuration options.

---------------- rescue-cd-with-xserver.nix
{pkgs, ...}:

let
  inherit (pkgs.lib) mkOverride;
in

{
  # Extend the default rescue-cd configuration.
  require = import ./rescue-cd.nix;

  services = {
    # Do not show the manual on tty 7 because the XServer is enabled.
    showManual = mkOverride 90 {/* override everything */} {
      enable = false;
    };

    xserver = {
      enable = true;
    };
  };
}
----------------

Cheers,

-- 
Nicolas Pierron
http://www.linkedin.com/in/nicolasbpierron
- If you are doing something twice then you should try to do it once.