zsh: add plugins submodule #56

dermetfan · 2017-09-04T15:06:46Z

See #55.

The plugins module does not have its own compinit. Instead it defaults enableCompletion to true.

Users who cannot or do not want to disable programs.zsh in their system config can still disable enableCompletion to avoid the slowdown. For example:

programs.zsh.enableCompletion = !systemConfig.programs.zsh.enable;

uvNikita · 2017-09-05T12:43:12Z

Thank you for PR!

I have a couple of comments.

I'm getting this error when trying to run $ home-manager switch:
error: The option `programs.zsh.plugins.*.name' is used but not defined.
The error seems to appear independently of the configuration file. Removing default value for file parameter fixes it.
Would you mind to provide an example of the plugins configuration? Something like this:

plugins = [
  {
    name = "nix";
    src = pkgs.fetchFromGitHub {
      owner = "spwhitt";
      repo = "nix-zsh-completions";
      rev = "0.3.1";
      sha256 = "1sbc52f5818bcygljrji84dyvgw727x50m9v6qfrsdaji3zkqga1";
    };
  }
];

It seems when I add a plugin using this submodule my zsh startup time increases significantly (from around 0.06 to 0.34). Looks like it is connected with appending plugin path to fpath. Trying to call compinit in the end of .zshrc results in the following warning:zsh compinit: insecure directories, run compaudit for list.
Running compinit -u solves the problem, but startup time still remains high. Do you know what it can be connected with?

rycee · 2017-09-05T21:25:50Z

modules/programs/zsh.nix

+
+      file = mkOption {
+        type = types.str;
+        default = "${config.name}.plugin.zsh";


Like @uvNikita mentioned this will cause problems because Nix will attempt to expand this during evaluation even if the module is not used in the configuration, e.g., for generating the home-configuration.nix man page.

Instead I think you can add a config section:

pluginModule = types.submodule ({ config, ... }: { options = { … }; config = { file = mkDefault name; }; };

Right, I amended the fix.

dermetfan · 2017-09-05T23:23:26Z

I believe I figured out the right way to do this:

Add all PATH and fpath entries.
Call compinit (-C should be faster than -u). This traverses fpath.
Source the plugin scripts as now they have access to functions added by compinit.

EDIT: oh-my-zsh does it this way as well

With this setup and no compinit in my /etc/zshrc I get 0.03s now.

uvNikita

I've tried to test this with different configurations and here is results of a start-up time:

plain zsh (empty zsh.plugins, zsh.oh-my-zsh disabled): 0.05
with empty zsh.plugins, oh-my-zsh enabled, and one plugin in oh-my-zsh.plugins: 0.17
with one plugin in zsh.plugins and oh-my-zsh disabled: 0.05
with one plugin in zsh.plugins, oh-my-zsh enabled, and one plugin in oh-my-zsh.plugins: 0.41

Zsh was enabled only in a home environment (no /etc/zshrc file), the testing was performed with this command: time zsh -i -c exit.

So on my system in seems like two solutions work quite fast separately, but not together. I didn't find the reason of this for now, maybe you have any idea on why this is the case?

In any case, it seems like the problem is not in this solution particularly, but more in the combination of two, so I think we can add this changes as it is now.

I also think that we should add a news entry about a new parameter, that would be a good example on how to use this functionality. @rycee please correct me if I'm wrong.

uvNikita · 2017-09-07T18:41:07Z

modules/programs/zsh.nix

+      plugins = mkOption {
+        type = types.listOf pluginModule;
+        default = [];
+        example = [


I think in this case example = literalExample = ''...'' would be better since it produces a more readable message in man home-configuration.nix

Sure, amened.

dermetfan · 2017-09-07T21:01:22Z

Thanks for the benchmarks! The combination is probably slow because oh-my-zsh calls compinit as well, so all fpath entries will be traversed again. You could try disabling programs.zsh.enableCompletion but you would not be able to ensure that oh-my-zsh runs after programs.zsh.plugins. We could integrate the oh-my-zsh module into the zsh module to fix the order.

EDIT: I tried that and got 0.17s with loads of zsh.plugins and two oh-my-zsh.plugins. 🎉 You can check it out on my zsh-plugins-omz branch. If we don't want to merge the modules we could possibly make programs.zsh.initExtra a list and use mkBefore (or introduce programs.zsh.init as a list that contains initExtra, to avoid breaking changes).

rycee · 2017-09-08T07:58:28Z

@uvNikita Yeah, if this is interesting for the general Zsh user then I think it would make sense to have a news entry saying that this option now is available. I guess it should be conditioned on programs.zsh.enable… For now I'm thinking it is best to keep the news entries centralized in news.nix so if you decide to add one then please add it there.

uvNikita · 2017-09-08T09:44:05Z

I tested your branch and indeed it looks like it improved performance a bit, thank you for observation! I don't see a problem with combining these modules for optimization purposes since they both setup zsh.

However, I performed more tests and was still experiencing a significant (by order of magnitude) increase in startup time when using both oh-my-zsh.plugins and zsh.plugins. It took some time, but I found the reason for this.

When we use src = pkgs.fetchFromGitHub it stores plugins in /nix/store folder which is considered to be unsafe according to zsh compinit function (due to writable permissions for the group). For some reason, if $fpath contains such directory, the compinit function performs much longer which results in a slow startup. As it was mentioned, compinit -C disables security checks and removes this slowdown. But if oh-my-zsh is enabled, compinit is executed inside a $ZSH/oh-my-zsh.sh script like this: compinit -i -d "${ZSH_COMPDUMP}". -i option tells compinit to ignore unsafe directories (for some reason in my tests completions defined in such directories were still available, even after removing all cache and dump files), but since the security checks are still performed we are getting same slow startup.

I also figured out that it seems like not referencing /nix/store/.. directly in $fpath helps to prevent zsh to complain about insecure parent directory. To check that I created link in my home folder ~/plugin -> /nix/store/<hash>-plugin and add $HOME/plugin in $fpath. This setup fixes performance issues.

So, as for now, I see three ways to go:

Patch oh-my-zsh to optionally use -C parameter instead of -i.
Somehow reference plugins not from the /nix/store directly, but from the user home directory. For instance, to automatically create ~/zsh/plugins and put symlinks to the plugin sources there. The advantage of this approach would be that in this case we can remove potentially insecure -C option from our module as well.
Leave everything as is and write a warning for users that using both modules can reduce startup performance slightly.

I personally think the second option would be the best, but I'm not sure how hard it is to implement such behavior in nix. @dermetfan what do you think about this?

In case `compinit` is called from within oh-my-zsh, this passes the security check. see nix-community#56 (comment)

In case `compinit` is called from within oh-my-zsh, this passes the security check. Also allows us to call `compinit` without `-C` for vanilla plugins. see nix-community#56 (comment)

dermetfan · 2017-09-08T16:55:06Z

Spot on, @uvNikita! Turns out the second option was not so hard to implement, and it did fix the slowdown as you described. I'm now at 0.07s with the same plugins as in my previous comment.

I added the commits to this PR's branch so you can review.

rycee · 2017-09-08T17:27:52Z

modules/programs/zsh.nix

+    (mkIf (builtins.length cfg.plugins > 0) {
+      programs.zsh.enableCompletion = mkDefault true;
+
+      home.file = builtins.listToAttrs (map (plugin: {


You can avoid listToAttrs here since home.file accepts lists of sets (the path is in the target attribute). Something like

home.file = map (plugin: { target = ".zsh/plugins/${plugin.name}"; source = plugin.src; }) cfg.plugins;

should work.

rycee · 2017-09-08T17:29:14Z

modules/programs/zsh.nix

+    (mkIf cfg.oh-my-zsh.enable {
+      programs.zsh.enableCompletion = mkForce false;
+    })
+    (mkIf (builtins.length cfg.plugins > 0) {


I think it is a bit more straight-forward to say cfg.plugins != [], no biggie, though.

Oh sure, I'm still used to conventional languages I guess.

In case `compinit` is called from within oh-my-zsh, this passes the security check. Also allows us to call `compinit` without `-C` for vanilla plugins. see nix-community#56 (comment)

uvNikita

Tested new version -- the startup is fast and everything seems to work, great job! :)
I have just last few comments on the code. If you could fix them, merge the last and the first commit together (to have just zsh: add plugins submodule and zsh: fix double compinit slowdown with oh-my-zsh commits with expanded messages), and to add news entry, then I will be glad to merge the PR :)

uvNikita · 2017-09-09T11:16:26Z

modules/programs/zsh.nix

-  );
+    })
+    (mkIf cfg.oh-my-zsh.enable {
+      programs.zsh.enableCompletion = mkForce false;


Would be nice to leave a comment here on why we disable this option since it might not be obvious to someone reading it in the future.

uvNikita · 2017-09-09T11:18:14Z

modules/programs/zsh.nix

+              };
+            }
+            {
+              file = "init.sh";


Since plugin.name is now required a parameter, this example is not working anymore. Should probably add something like name = "enhancd".

uvNikita · 2017-09-09T11:19:52Z

modules/programs/zsh.nix

+
+      name = mkOption {
+        type = types.str;
+        description = "The name of the plugin. Don't forget to add <option>file</option> if the script name does not follow convention.";


Looks like these two descriptions are too long, would you mind to rewrite it using '' quotes?

In case `compinit` is called from within oh-my-zsh, this passes the security check. Also allows us to call `compinit` without `-C` for vanilla plugins. see nix-community#56 (comment)

zsh: fix double compinit slowdown with oh-my-zsh Integrate the oh-my-zsh module into the zsh module in order to be able to control invocation order. zsh: link plugins in home directory In case `compinit` is called from within oh-my-zsh, this passes the security check. Also allows us to call `compinit` without `-C` for vanilla plugins. see nix-community#56 (comment)

uvNikita · 2017-09-10T14:02:28Z

I was thinking that it would be nicer to have two commits: one about adding zsh.plugins and one about merging oh-my-zsh module into zsh for optimizations, since those are somewhat independent.

Integrate the oh-my-zsh module into the zsh module in order to be able to control invocation order.

In case `compinit` is called from within oh-my-zsh, this passes the security check. Also allows us to call `compinit` without `-C` for vanilla plugins. see nix-community#56 (comment)

Integrate the oh-my-zsh module into the zsh module in order to be able to control invocation order. zsh: link plugins in home directory In case `compinit` is called from within oh-my-zsh, this passes the security check. Also allows us to call `compinit` without `-C` for vanilla plugins. see nix-community#56 (comment)

dermetfan · 2017-09-11T18:53:07Z

Added a news entry in a third commit. Do we agree like this? 😃

rycee · 2017-09-11T19:56:17Z

modules/misc/news.nix

@@ -108,6 +108,14 @@ in
  config = {
    news.entries = [
      {
+        time = "2017-09-10T00:15:19+02:00";
+        message = ''


Looking good but should have a field condition = config.programs.zsh.enable; to limit the news to those that are using Zsh.

rycee · 2017-09-11T19:56:57Z

modules/misc/news.nix

@@ -108,6 +108,14 @@ in
  config = {
    news.entries = [
      {
+        time = "2017-09-10T00:15:19+02:00";


Should be UTC, i.e., like the output of date --iso-8601=second --universal.

rycee · 2017-09-11T20:01:51Z

Not that I understand much of Zsh but it looks like this PR has come a long way! Maybe should have a look at trying Zsh out ;-)

Thanks to both of you for such productive and fruitful work!

uvNikita · 2017-09-12T11:42:24Z

I probably didn't explain properly what I mean. I was thinking to have two commits in the end:

The first one, that contains both new parameter implementation (with plugins being linked into the home directory) and the news entry:

zsh: add plugins submodule 

Plugins are liked in the home directory.
In case `compinit` is called from within oh-my-zsh, this passes the security check,
and allows us to call `compinit` without `-C` for vanilla plugins.
Also, it solves issues with a slow start up,
see https://github.com/rycee/home-manager/pull/56#issuecomment-328057513.

And the second one, that merges oh-my-zsh and zsh modules for even further optimizations:

zsh: fix double compinit slowdown with oh-my-zsh

Integrate the oh-my-zsh module into the zsh module
to be able to control invocation order.

Alternatively, I can restructure your commits in this way during rebase if you would prefer that :)
Sorry for the confusion I created :)

uvNikita · 2017-09-12T11:49:14Z

@rycee I'm also very excited about these new changes and optimizations! With this PR merged and couple of small fixes that will be added soon our zsh module will be faster and more featureful even than the one from nixpkgs :)

dermetfan · 2017-09-12T12:19:40Z

@uvNikita Go ahead then, I pushed the original commits back up. My git-fu is ok but I don't see how I would rebase zsh: link plugins in home directory before its parent without manually resolving conflicts. I guess that's why communication failed here :)

To pass compinit security checks, plugins are liked into ~/zsh/plugins folder. This also solves issues with a slow start up, see #56 (comment).

uvNikita · 2017-09-12T13:00:24Z

Rebased changes into master. Thank you for the contribution!

To pass compinit security checks, plugins are liked into ~/zsh/plugins folder. This also solves issues with a slow start up, see nix-community#56 (comment).

dermetfan force-pushed the zsh-plugins branch from b97069e to e20ff95 Compare September 4, 2017 15:10

rycee reviewed Sep 5, 2017

View reviewed changes

dermetfan force-pushed the zsh-plugins branch 5 times, most recently from 2035f0a to 11948c4 Compare September 5, 2017 23:14

uvNikita reviewed Sep 7, 2017

View reviewed changes

dermetfan force-pushed the zsh-plugins branch from 11948c4 to c5b946a Compare September 7, 2017 20:46

dermetfan added a commit to dermetfan/home-manager that referenced this pull request Sep 8, 2017

zsh: link plugins in home directory

1f43598

In case `compinit` is called from within oh-my-zsh, this passes the security check. see nix-community#56 (comment)

dermetfan force-pushed the zsh-plugins branch from 1f43598 to 76a1880 Compare September 8, 2017 16:48

dermetfan force-pushed the zsh-plugins branch from 76a1880 to ce065fa Compare September 8, 2017 16:52

rycee reviewed Sep 8, 2017

View reviewed changes

dermetfan force-pushed the zsh-plugins branch from ce065fa to 37a7b92 Compare September 8, 2017 18:07

dermetfan force-pushed the zsh-plugins branch from 37a7b92 to 00d9c04 Compare September 8, 2017 18:11

uvNikita reviewed Sep 9, 2017

View reviewed changes

dermetfan force-pushed the zsh-plugins branch from 00d9c04 to 4dbddde Compare September 9, 2017 21:27

dermetfan force-pushed the zsh-plugins branch from 4dbddde to fdbc4ac Compare September 9, 2017 22:04

dermetfan added 3 commits September 11, 2017 12:32

zsh: add plugins submodule

f98f72a

zsh: fix double compinit slowdown with oh-my-zsh

76681a9

Integrate the oh-my-zsh module into the zsh module in order to be able to control invocation order.

zsh: link plugins in home directory

849cf3a

In case `compinit` is called from within oh-my-zsh, this passes the security check. Also allows us to call `compinit` without `-C` for vanilla plugins. see nix-community#56 (comment)

dermetfan force-pushed the zsh-plugins branch from fdbc4ac to 0d543cf Compare September 11, 2017 11:27

dermetfan force-pushed the zsh-plugins branch 2 times, most recently from 11a10b1 to 38dd49b Compare September 11, 2017 11:33

rycee reviewed Sep 11, 2017

View reviewed changes

dermetfan force-pushed the zsh-plugins branch from 38dd49b to b975dad Compare September 11, 2017 20:06

zsh: add news entry

ca6803a

dermetfan force-pushed the zsh-plugins branch from b975dad to ca6803a Compare September 12, 2017 12:13

uvNikita pushed a commit that referenced this pull request Sep 12, 2017

zsh: add plugins submodule

258bc85

To pass compinit security checks, plugins are liked into ~/zsh/plugins folder. This also solves issues with a slow start up, see #56 (comment).

uvNikita closed this Sep 12, 2017

dermetfan deleted the zsh-plugins branch September 12, 2017 13:30

dermetfan mentioned this pull request Sep 12, 2017

zsh: plugins #55

Closed

pasqui23 pushed a commit to pasqui23/home-manager that referenced this pull request Oct 14, 2017

zsh: add plugins submodule

6f26d72

To pass compinit security checks, plugins are liked into ~/zsh/plugins folder. This also solves issues with a slow start up, see nix-community#56 (comment).

uvNikita mentioned this pull request Feb 11, 2019

zsh does not work with home-manager build #578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zsh: add plugins submodule #56

zsh: add plugins submodule #56

dermetfan commented Sep 4, 2017 •

edited

Loading

uvNikita commented Sep 5, 2017

rycee Sep 5, 2017 •

edited

Loading

dermetfan Sep 5, 2017 •

edited

Loading

dermetfan commented Sep 5, 2017 •

edited

Loading

uvNikita left a comment •

edited

Loading

uvNikita Sep 7, 2017

dermetfan Sep 7, 2017

dermetfan commented Sep 7, 2017 •

edited

Loading

rycee commented Sep 8, 2017

uvNikita commented Sep 8, 2017

dermetfan commented Sep 8, 2017

rycee Sep 8, 2017

rycee Sep 8, 2017

dermetfan Sep 8, 2017

uvNikita left a comment

uvNikita Sep 9, 2017

uvNikita Sep 9, 2017

uvNikita Sep 9, 2017

uvNikita commented Sep 10, 2017

dermetfan commented Sep 11, 2017

rycee Sep 11, 2017

rycee Sep 11, 2017

rycee commented Sep 11, 2017

uvNikita commented Sep 12, 2017

uvNikita commented Sep 12, 2017

dermetfan commented Sep 12, 2017 •

edited

Loading

uvNikita commented Sep 12, 2017

zsh: add plugins submodule #56

zsh: add plugins submodule #56

Conversation

dermetfan commented Sep 4, 2017 • edited Loading

uvNikita commented Sep 5, 2017

rycee Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

dermetfan Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

dermetfan commented Sep 5, 2017 • edited Loading

uvNikita left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dermetfan commented Sep 7, 2017 • edited Loading

rycee commented Sep 8, 2017

uvNikita commented Sep 8, 2017

dermetfan commented Sep 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uvNikita left a 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

uvNikita commented Sep 10, 2017

dermetfan commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rycee commented Sep 11, 2017

uvNikita commented Sep 12, 2017

uvNikita commented Sep 12, 2017

dermetfan commented Sep 12, 2017 • edited Loading

uvNikita commented Sep 12, 2017

dermetfan commented Sep 4, 2017 •

edited

Loading

rycee Sep 5, 2017 •

edited

Loading

dermetfan Sep 5, 2017 •

edited

Loading

dermetfan commented Sep 5, 2017 •

edited

Loading

uvNikita left a comment •

edited

Loading

dermetfan commented Sep 7, 2017 •

edited

Loading

dermetfan commented Sep 12, 2017 •

edited

Loading