Make zshrc more predictable #25648

yacinehmito · 2017-05-09T16:12:20Z

Make zshrc more predictable by modifying the initialization scripts.

Motivation for this change

Originally, programs.zsh sets default values for some initialization scripts.
Nix resolves the case of multiple values by concatenating them all.
It is however impossible to predict where the default script will be inserted; but we never want the default value to override the user-specified ones.

I encountered the problem when trying to augment nix-shell with zsh-specific hooks. As the hooks were inserted at unpredictable points in the scripts, it would sometimes fail.

Things done

Now, it doesn't set default values; almost everything is hardcoded at the beginning of the initialization file. Apart from predictability, it doesn't change anything.

Note that the point at which the promptInit script is evaluated has changed. For reasons I didn't investigate it seems to want to be evaluated last.

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

Ma27 · 2017-05-09T16:31:51Z

I like the idea of the change. I hope I'll find some time in the next days to test it :-)

Ma27 · 2017-05-12T09:25:30Z

I finally managed to test your change (sry for the delay :/)

I built a VM using the following configuration:

{config, pkgs, ...}:
{
  # You need to configure a root filesytem
  fileSystems."/".label = "vmdisk";

  # The test vm name is based on the hostname, so it's nice to set one
  networking.hostName = "vmhost"; 

  # Add a test user who can sudo to the root account for debugging
  users.extraUsers.vm = {
    password = "vm";
    shell = "${pkgs.bash}/bin/bash";
    group = "wheel";
  };
  security.sudo = {
    enable = true;
    wheelNeedsPassword = false;
  };

  environment.systemPackages = with pkgs; [ git lambda-mod-zsh-theme ];

  # this ZSH configuration is used on my laptop as well and works fine there
  programs.zsh = {
    enable = true;

    syntax-highlighting = {
      enable = true;
      highlighters = [ "main" "brackets" "root" "pattern" ];

      patterns = [
        ["rm -rf *" "fg=white,bold,bg=red"]
      ];
    };

    oh-my-zsh = {
      enable = true;
      theme = "lambda-mod";
      custom = with pkgs; "${lambda-mod-zsh-theme}/share";

      plugins = [
        "git"
      ];
    };
  };  
}

I created/provisioned the VM using the following command:

 NIXOS_CONFIG=`pwd`/vmtest.nix nixos-rebuild -I nixpkgs=$HOME/Projects/nixpkgs/ build-vm

However when running zsh in the VM the oh-my-zsh plugin uses the default theme and not the lambda theme.
Am I doing something wrong or is there an undetected issue in your change?

yacinehmito · 2017-05-14T20:06:39Z

I just reproduced your testing environment and if I execute echo $ZSH_THEME I get lambda-mod, although it's true that the prompt doesn't look anything like the lambda theme.

I will investigate.

(Btw, I'm experiencing for the first time the sheer awesomeness of the sharing capabilities of NixOS. I just copy-pasted a configuration and here I am with a bug reproduced. This is insane.)

Ma27 · 2017-05-14T20:24:35Z

thank you :)

I assume that some shell files are executed before setting the ZSH_THEME var so it might be a simple ordering problem in you change...

yacinehmito · 2017-05-14T20:46:31Z

It's an ordering problem indeed.

The default value of promptInit sets the prompt theme. In my PR it is executed last. This means that it will override what oh-my-zsh sets.
However, if I move promptInit up, any code that I will add to it to customize the prompt will be overridden by oh-my-zsh.

Like the other configurable snippets, I suggest removing promptInit's default value. The original content can be pasted higher up in the concatenated file while the customized lines will be left where they currently stand, i.e. at the end of the file.

I just force-pushed this change. What do you think @Ma27 ?

(I didn't test the changes yet because my system is currently updating and it is taking forever)

EDIT: I just tested the changes and it seems to be working.

Ma27 · 2017-05-15T04:54:28Z

@gpyh I can confirm that the appropriate theme is loaded now. Syntax highlighting works as well, so a big 👍 from my side :-)

LnL7 · 2017-05-18T17:11:12Z

nixos/modules/programs/zsh/zsh.nix

@@ -67,9 +67,7 @@ in
      };

      promptInit = mkOption {
-        default = ''
-          autoload -U promptinit && promptinit && prompt walters


why did you remove the default here? even tho a custom prompt will be set afterwards the walters prompt will always get loaded.

You're right. My problem is with how oh-my-zsh works. It changes the content of interactiveShellInit, which is executed before promptInit. As such, the default value will always have precedence. If I change the order and executes it after, another problem arises: any change I make to promptInit will be overriden by oh-my-zsh.
I compromised by putting the current default value before interactiveShellInit and the actual promptInit after.

You are right that this will always cause walters to be loaded, which may be undesirable.
Another more sensible solution would be to change the default value in case oh-my-zsh is used. Would you rather have that?

right, I think adding this will have the expected behaviour while keeping walters the default for the promptInit option.

{ config = { # or maybe? (cfg.oh-my-zsh.theme != "") programs.zsh.promptInit = mkIf cfg.oh-my-zsh.enable (mkDefault ""); }; }

LnL7 · 2017-05-18T17:15:47Z

nixos/modules/programs/zsh/zsh.nix

+
+        ${cfge.interactiveShellInit}
+
+        HELPDIR="${pkgs.zsh}/share/zsh/$ZSH_VERSION/help"


maybe this should also be moved to the top?

Same comment as the previous one; I only copy-pasted. It may be at the end of the file to prevent the user from setting HELPDIR. I can move it at the top if you wish so.

yes, there's probably not a reason to override it but it would be consistent the with other environment variables.

LnL7 · 2017-05-18T17:22:42Z

nixos/modules/programs/zsh/zsh.nix

-          fpath+=($p/share/zsh/site-functions $p/share/zsh/$ZSH_VERSION/functions)
-        done
-
-        ${if cfg.enableCompletion then "autoload -U compinit && compinit" else ""}


I think this should be moved to the end, that way if a user extends fpath in there the completions will be loaded as expected.
Also ${optionalString cfg.enableCompletion "autoload -U compinit && compinit"} is a little bit nicer.

I only copy-pasted the content from interactiveShellInit to the actual template, but I can change it if you want me to.

On the first remark about fpath, I don't understand how the order is relevant. The variable will be extended either way.

On the second remark, sure, I'll do that.

I'm pretty sure compinit loads everything during initialisation, so it won't pick up the extra entries added afterwards.

Actually it's the other way around. Auto-loaded may use compadd which require completion to be enable. There is no direct relationship between compinit and fpath. Therefore I will move compinitup.

@yacinehmito Are you 100% sure about that?

http://zsh.sourceforge.net/Doc/Release/Completion-System.html#Autoloaded-files

When compinit is run, it searches all such files accessible via fpath/FPATH and reads the first line of each of them. This line should contain one of the tags described below. Files whose first line does not start with one of these tags are not considered to be part of the completion system and will not be treated specially.

yacinehmito · 2017-05-28T13:17:32Z

@LnL7 I've made the changes at last! (sorry for the big delay)

Originially, `programs.zsh` sets default values for some initialisation scripts. Nix resolves the case of multiple values by concatenating them all. It is however impossible to predict where the default script will be inserted; but we never want the default value to override the user-specified ones. Now, it doesn't set default values; almost everything is hardcoded at the begining of the file.

yacinehmito · 2017-07-08T17:31:40Z

Bumping !

LnL7 · 2017-07-08T17:45:12Z

Oh sorry, kind of forgot about this.

yacinehmito · 2017-07-11T00:56:54Z

Thanks!

yacinehmito force-pushed the custom branch from 17d839e to 523881d Compare May 14, 2017 20:43

LnL7 reviewed May 18, 2017

View reviewed changes

yacinehmito force-pushed the custom branch from 523881d to e60563a Compare May 28, 2017 13:14

yacinehmito force-pushed the custom branch from e60563a to c306942 Compare July 1, 2017 18:32

yacinehmito force-pushed the custom branch from c306942 to 1f70f38 Compare July 6, 2017 20:44

LnL7 approved these changes Jul 9, 2017

View reviewed changes

LnL7 merged commit 8189811 into NixOS:master Jul 9, 2017

LnL7 mentioned this pull request Jul 27, 2017

zsh auto-completion for packages in environment not working #27587

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make zshrc more predictable #25648

Make zshrc more predictable #25648

yacinehmito commented May 9, 2017 •

edited

Loading

Ma27 commented May 9, 2017

Ma27 commented May 12, 2017

yacinehmito commented May 14, 2017 •

edited

Loading

Ma27 commented May 14, 2017

yacinehmito commented May 14, 2017 •

edited

Loading

Ma27 commented May 15, 2017

LnL7 May 18, 2017

yacinehmito May 18, 2017

LnL7 May 18, 2017

LnL7 May 18, 2017

yacinehmito May 18, 2017

LnL7 May 18, 2017

LnL7 May 18, 2017 •

edited

Loading

yacinehmito May 18, 2017

LnL7 May 18, 2017

yacinehmito Jun 15, 2017

olejorgenb Aug 24, 2017

yacinehmito commented May 28, 2017

yacinehmito commented Jul 8, 2017

LnL7 commented Jul 8, 2017

yacinehmito commented Jul 11, 2017


		${cfge.interactiveShellInit}

		HELPDIR="${pkgs.zsh}/share/zsh/$ZSH_VERSION/help"

Make zshrc more predictable #25648

Make zshrc more predictable #25648

Conversation

yacinehmito commented May 9, 2017 • edited Loading

Motivation for this change

Things done

Ma27 commented May 9, 2017

Ma27 commented May 12, 2017

yacinehmito commented May 14, 2017 • edited Loading

Ma27 commented May 14, 2017

yacinehmito commented May 14, 2017 • edited Loading

Ma27 commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LnL7 May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yacinehmito commented May 28, 2017

yacinehmito commented Jul 8, 2017

LnL7 commented Jul 8, 2017

yacinehmito commented Jul 11, 2017

yacinehmito commented May 9, 2017 •

edited

Loading

yacinehmito commented May 14, 2017 •

edited

Loading

yacinehmito commented May 14, 2017 •

edited

Loading

LnL7 May 18, 2017 •

edited

Loading