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

[Michael Raskin]

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

Nicolas Pierron wrote:
> 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.

Well, I am ready to reply "why". I want to say beforehand that when I
express my emotions this is not directed towards anybody, just about
actual situation. Full answer includes some background.

(background

General situation:
Long ago I prepared LiveDVD expression which allowed to have
network-independent install-capable LiveDVD with X. Note that sane X
implementation and network independence were not featured in
rescue-cd.nix until my commit.

Next, for a long time Eelco Dolstra had no opportunity to review it.

During that time there were many improvements in the layout of general
NixOS and in the layout of official LiveCD. Anyway, official LiveCD
lacked many features. I ported some updates between official LiveCD and
my configurable expression. It became somewhat boring, of course.

When Eelco Dolstra found time to describe his opinion of more
configurable LiveCD expression, he said that he would want it to have
small number of reasonable flexible parameters.. Piecewise addition of
features I needed made some mess of rescue-cd-configurable.nix.

; end background
)

Now, rescue-cd-configurable seems to become broken and hard to fix after
latest nixos/nixpkgs updates. I want to do an update to latest NixPkgs
and to BtrFS for store (which means kernel 2.6.30-rc*). To do that I
need a working LiveDVD with a non-trivial set of packages. Fortunately,
I see that rescue-cd.nix can now be made configurable - with possibility
to rather easily port most of the useful features - with quite a small
non-intrusive patch. So I commit it.

> 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:
> 
> 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

I do not take that point. I'd even prefer creating new custom LiveDVDs
to take some understanding of the process..

> declarative and the action of removing does not feel sane.  Rewriting
> everything does not scale.

That's why I pass baseConfiguration. "x: x//{a=x.a//{b=c;};};" is
relatively scalable and does work. It is not the optimal solution, of
course. What you propose requires a big change. So it is better done in
a fork. So it will have a good chance to end up in the same state as
rescue-cd-configurable..

Also, I tried to start that way. That's how rogue/showManual jobs
appeared in NixOS. It turned out to be a noticeable amount of work
before anything testable.

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

I do follow your pattern with new upstart jobs. With the LiveCD I modify
existing code and I wanted to have a small patch for it to be less
controversial.

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

Well, if there are many similar configurations, factoring out common
settings is not that hard - and it can create either new files or new
general NixOS settings. And ot can be done incrementally.

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

And it had been so since its appearance. Non-configuration parts slowly
get washed away (i.e. to helpers/iso9660.nix etc)

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

That is a clear modular-branch style change. It gives no direct user
features. So it is not obvious whether it is correct to do in trunk. I
needed to add features without ruining anything worse than it was and
with ability to have it in trunk. It is nearly completely orthogonal.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iQEcBAEBAgAGBQJJ/ebUAAoJEE6tnN0aWvw393sH/jiYPMrm27i14+MIl7tfJpTo
0VMESm+jXVXKnn5D6xSYF4KOiffurwbcR0Di1Z4t90vDYSBeR+aa42CbNMm2jxeN
d1Gn55uepG2ashVeVCc4cvM8oErWhw3pUUqNpRgnJh+IAfdCHIlKyAW6pxk1z4VP
mkg+Ht5qRS84bsFaORtqUgUTIRaQ2k/Y1iQ57/WnmGqu/WjTiQP3B2p+YTQeIoWk
JQGGyDdXfMdIoANbqsI2ygWdguQvXt/6Zhnj20WYQbPi8PCpmR9ux0tL9xt7cxxO
EUGXf7HDB9wz1qCcvGHVvD4ZcOz2Y2ySCF+KFFVdQKL+DJ+cpcZ3Y/VszROJyBU=
=0GvE
-----END PGP SIGNATURE-----