Change the way NixOS handles environment variables, NixOSize environment configuration, make it possible not to use Bash, add support for Zsh.

[oxij]

See commit messages.

[oxij]

Just to note. I somewhat like the "features" idea in #138 by @MarcWeber, but I think this patch handles environment variables better and doesn't change anything else except for one option name.

[peti]

Are the quotes in that line necessary? Wouldn't environment.variables.FONTCONFIG_FILE.value = ... work just the same way? Or am I missing something?

[oxij]

It would, I did not think much about the cosmetics.

[peti]

Is there any way to get rid of value-part in the assignments? The form

environment.variables.TZDIR = "/etc/zoneinfo";

seems simpler and more intuitive.

[oxij]

@peti, yep, in principle, it looks nicer without the value part, but then it simply becomes envVar option type and it's impossible guarantee that the option is single valued/multivalued.
This value-list-separation is the simplest way to declare the type of a variable I thought of.
The other way to get similar behavior is to declare all the variables explicitly beforehand, this would probably mean another top-level module attribute (options, config, …), I'm not fond of that idea.

[MarcWeber]

@oxij "but I think this patch handles environment variables better"
Would you mind explaining what's worse or too complicated with my patch?

[oxij]

@MarcWeber I don't like the whole idea of setting up the variables by typing sh code. Current nixos does this everywhere and your patch doesn't change that (AFAICS, it more or less duplicates this setup for zsh).
But I like some pieces in your version and I would steal them into my version sometime.

My point is that this version of my patch doesn't (at least shouldn't) change anything for current users, but makes environment setup more modular and reusable (and it could get even better when nixpkgs is fixed, see NixOS/nixpkgs#986).

[bjornfor]

@oxij: This looks interesting. But I didn't quite get your explanation of why you want/need .value and .list. I understand that you don't get one explicit type without them, but why is that important? Could you elaborate? IMHO, just environment.variables.SOMEVAR = "string" and environment.variables.SOMEVAR = ["a" "list"] looks much nicer.

[oxij]

@bjornfor Yes, I would like to have something prettier (algebraic data types!) too, but I did not find any other way to define the type of a variable to be uniqe string OR mergable list of strings with the help of the current tools. Suppose you set some variable SOMEVAR that must have exactly one value to "foo" in one module and to "bar" in the other, I don't want to get the list ["foo" "bar"] as the result of the merge, I want to get an error. All this is not too critical right now, because most of the setup is done in one file, but it's going to turn into a mess as soon as environment setup is spread into modules (it should).
Also, it's not me who invented this .something, e.g. we already have environment.etc."filename".text = "content" and environment.etc."filename".source = link.

[bjornfor]

@oxij: Thanks for explaining.

[MarcWeber]

@oxij: Yes, I duplicated the implementation because I felt ZSH is different, and sharing is hard (unless you start changing default behaviour of ZSH a lot).

How do you cope with system vs user profile and merging both exactly?

I dislike code duplication myself, but in that case it somehow looked easier to me to provide one implementation for each shell, because they have different features.

What I did change is that I added primitives which allow you to mix arbitrary environments the way you want.
Eg you can opt-out from system-env by just adding ~/.nix-profile. Or you could add ~/.nix-profile{1,2,3} to your bash/zsh environment. Its also you choosing suffix or prefix.

/etc/{bash,zsh}/setup-all is just the system default. People can either opt-out or put the code they want in their own rc files reusing the primitives defined in /etc/{bash,zsh}/SHELL_NAME_lib files.

So if you don't like shell, which is your proposal, how to merge system/user envs in a sane way without using the shell? Can you explicitly explain how your version differs in this aspect?

[bluescreen303]

NERDFIGHT! :)

Please, can we just merge this if nobody has any real objections?
Sure there is room for improvement/cleanup later, but as a whole I think this is better than what we currently have, right?

Discussions about whose solution is better do not really matter to me, because this is a solution that has been put forward as a pull request, seems to be backward compatible and doesn't have any clear drawbacks.

@MarcWeber if your solution is much better, you should have made a pull request!
Instead of overloading people who do make a request with questions about specifics compared to your solution.

As I remember correctly, I asked about nixos zsh support on nix-dev like 2 years ago, where you mentioned your patches. I asked if you were going to push them upstream, but you didn't feel they were ready yet or had seen enough testing. Now, consider this pull request as a start, where people will actually be able to use zsh and start testing its interaction with nix. Then if this solution proves lacking, we can probably port your stuff/ideas.

So unless @MarcWeber makes a pull request too (without regressions from the way things are now), I'd say we merge this, instead of arguing over differences to some hypothetical solution that was never put forward.

[peti]

For what it's worth, I too think this patch should be merged. I don't see a reason not to ...

[MarcWeber]

@bluescreen303 NERDFIGHT is wrong word. You can assemble env vars in nix, and I even see some advantages.

My current (more or less) up to date patch #138. (Updated version which has been in use by me since 2 years) is on my github page.
The first announcment about 2 years ago is here:
http://article.gmane.org/gmane.linux.distributions.nixos/5666/match=zsh+bash+marc+weber+patch

@bluescreen303 What you say is that you expect regressions. Maybe that's unfair without having tested my patches.

My comments about the patch:

COMMENT 1:
I strongly dislike the bash.enable = true setting which sets userShell to "bash"
Why? There will be a conflict if you use bash.enable = true and zsh.enable = true which you might have to override with mkOverride (complicated) or lookup how the code does resolve this.

So at least this should be fixed before merging IMHO. How to fix?
read userShell, if its set to zsh, set zsh.enable to true. Same about bash.
Then all problems are gone. So please also take care about such details before wanting to merge.
It does make a difference in usability and readability.

COMMENT 2:

Exactly one of this or list must be set.
What about dropping this line showing an error if its done wrong?

COMMENT 3:

Enable Bash completion for all interactive bash shells.
2 years ago there have been users who did not like some bash completion implementations (eg of tar or such).
That's why my implementation introduces a dictionary you can set keys to opt-out from some completion scripts you don't like. If the admin only has the option "enable and annoy users" or disable "and be annoyed himself" its not optimal IMHO. How do you want to cope with such a situation? (I don't call this NERD fight)

COMMENT 4:

modules/programs/environment.nix
What is happening here? Previously the shell assembled INFOPATH:

export NIX_PROFILES="/run/current-system/sw /nix/var/nix/profiles/default $HOME/.nix-profile"

for i in $NIX_PROFILES; do # !!! reverse
    export INFOPATH=$i/info:$i/share/info${INFOPATH:+:}$INFOPATH

This code was dropped in favor of a nix only solution. The Nix only solution has pro and cons:
pro:
its fast, because no string concatenation has to be done by bash

cons:

1. I cannot omit directories which don't exist. Eg having ${sed}/qtdir added to QTDIRS env var doesn't make sense
2. Worse: The nix code will be run by nixos-rebuild, thus when updating the system. Because the system cannot know about user profiles, there is no chance to add $HOME/.nix-profile/infodir to INFOPATH.

Thus this means a major change in the behaviur ?

Please don't be that lazy calling this a "NERD FIGHT". I may be a NERD, but sometimes its worth thinking about what I say or suggest. I'm human, I fail. So forgive me if I'm mistaken. I didn't test it.

So I expect info failing to find info pages of user installed packages?
Probably the lazy comment "it needs some changes to nixpkgs
(which I'm too lazy to perform) to be perfect!" is meant to make the user understand
such a major change I'd even call regression unless its commented.

So please also adjust the comment accordingly so that everybody
clearly understands what this patch does without reading the patch!

A global list of profiles cannot replace user profiles, can it?

Thanks.

[oxij]

@MarcWeber,

1. On COMMENT1:
   Yes, both bash.enable = true and zsh.enable = true try to set the users.defaultUserShell.
   But, no, you don't need mkOverride, it tries to set the default value.
   So, yes, having both bash and zsh enabled needs something like users.defaultUserShell = "/run/current-system/sw/bin/zsh" in configuration.nix (but, well, what do you expect it to do in this case?), but that's all, and description states this explicitly.

2. On COMMENT2:
   Hey, it's a description string!
   Obviously it would throw an exception if both are set (value has uniq type, and config part makes this work as expected).

3. On COMMENT3:
   I did not change anything there, current state of art does exactly the same.
   This point is the answer to your previous message:
   I'm not trying to sell you a new modular way to configure shell. I'm trying to sell a modular way to configure environment. Easy zsh support (with all the nonmodular shortcomings, yes) comes as a bonus.
   NixOS can do better than that to shells' configuration, yes, but I'm not trying to propose any carefully deliberated solution here, I'm just changing environment handling.

4. On COMMENT4:
   Yes, everything was "NixOSized" here. No, nothing is changed at all. Generated environment variables are exactly the same as before. See the handling of environment.profiles and environment.profileVariables.
   You still can add $HOME/.nix-profile/infodir to INFOPATH by environment.profileVariables = (i : { INFOPATH = [ "${i}/infodir" ]; }); in your configure.nix (this will add more than just $HOME/.nix-profile/infodir, it will generate such a variable for each profile, including "$HOME/.nix-profile", but you get the idea).
   In principle, your wish to add $HOME/.nix-profile/infodir to INFOPATH (and nothing else, if I get your wish it right) should be done with environment.variables.INFOPATH.list = [ "$HOME/.nix-profile/infodir" ];, but it wouldn't work like this right now since environment.variables and environment.profileVariables are handled differently (because of nixpkgs issue mentioned above). So, yes, this is a valid point, I'll see if I can hack around this cheaply somehow tomorrow.
   But, again, this patch doesn't change generated environment variables, it changes the way they are generated, if something doesn't work in the "master" it still wouldn't work after this patch, if it works it still should.
   Even more, nothing related to user profiles is changed, per-profile environment variables still work as they were, except now they are generated by nix.
   Finally, yes, you can't filter out nonexistent paths, but current NixOS version doesn't do this either (and it would be painfully slow, by the way).

As for patch comment, well, I'll think about it, but I'm not sure it's possible to explain how the patch changes environment handling without simply retyping this change in English. (Also, NixOS saw much bigger changes with much shorter comments.)
Also, conventional configuration.nix users normally shouldn't notice this change at all.

[bluescreen303]

@MarcWeber I know it's the wrong word, as us nixers don't fight :)
And yes: the way I wrote it, it's like I expect regressions with your patch and that would be unfair.

What I mean is: these commits do 1 thing, and 1 thing only.
First commit: change the way nix builds up the environment, without changing any behaviour for users, exact same end-result. No need to configure/symlink/change a thing.
Second commit: Add zsh support on top of this.

With your patch, as you mention yourself, you change a lot at once (not just the environment), adding lots of new features. Also, I cannot simply verify (by looking at the changes and within 10 minutes) that things will end up in the same state for all cases.

Don't get me wrong, I like most ideas you propose/mention, but I don't like big all-or-nothing blobs.
If problems arise, all we can do is revert everything or start live-hacking on master to fix it.
So small, modular commits, like the 2 in this pull request, are highly preferred. They give a clear picture of how things fit together and they can be partially applied (revert last commit, keep the rest).

Furthermore, @oxij mentioned he had seen your work, so no unnecesarry duplication of work, which would have been my only concern if people propose similar stuff to what others have been working on.

So instead of turning this into a vote of which solution is more flexible (a nerdfight:)), can't you just accept this proposal (as it is a light change) and work together on sanding off the rough edges to get your additional stuff applied? Like I said: I would like those, just not all at once.

[MarcWeber]

Simply pointing me to environment.profiles would have been enough.

So only COMMENT 1 is left which I'd change.

Another question: Why is there no easy way to configure aspell behavior?
Why is a bash loop required?

Could this be handled by adding a reverse property to profileVariables items ?

I ask because I have a similar case for XML_CATALOG_FILES
and there could be more.

@bluescreen303 You could have asked for trying to create smaller patches.

I've written to the mailinglist in order to find out which additional changes could be
of interest to nix users.

[oxij]

@MarcWeber

How exactly? I don't like the idea of guessing bash/zsh.enable things from defaultUserShell.

Because you could have installed dictionaries with nix-env or by adding them into configuration.nix, preferably both should work, but aspell doesn't like two dict-dirs.
So the reverse property, if I'm thinking what you thinking, wouldn't help, since you still need to check all this dict-dir stuff is really there.

[oxij]

Okay, I think I fixed all the issues which are fixable with current nixpkgs.
defaultUserShell is still set as before, I'm not convinced that it should work differently (and don't understand exactly how differently).

(Would be nice if someone except me tested this.)

[bluescreen303]

I tested the previous "version" and found my environment to be exactly the same.

Let's start discussing possible extensions/improvements somewhere else. For now it's a nice improvement.

[bluescreen303]

And thanks :)

[edolstra]

Yes, thanks!

I've tweaked this a bit to unify "list" and "value" as @peti suggested. So now you can just say environment.variables.FOO = "string" or environment.variables.FOO = [ "a" " list" ].

[rickynils]

@edolstra Variables don't seem to merge well. environment.variables.EDITOR = "vim" gives me an environment where EDITOR == vimnano, since NixOS also sets that variable.

[MarcWeber]

@rickynils: use mkOverride, grep nixos for mkOverride usage examples

[oxij]

@edolstra I think the issue @rickynils mentions suggests that this list-value separation is needed after all.

[edolstra]

I've fixed this by adding a merge function that takes overrides into account. It gives an error message if there are multiple string values with the same priority.

[oxij]

Ah, ok then.

[shlevy]

This PR changed some options without adding them to <nixos/modules/rename.nix>, breaking old configurations. I am trying to fix this now, but in the future please check for this before merging.

[oxij]

@shlevy I thought I did check that. What did I miss?