Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zsh: add plugins submodule #56

Closed
wants to merge 4 commits into from

Conversation

dermetfan
Copy link
Collaborator

@dermetfan dermetfan commented Sep 4, 2017

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
Copy link
Collaborator

uvNikita commented Sep 5, 2017

Thank you for PR!

I have a couple of comments.

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

  2. 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";
    };
  }
];
  1. 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?


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

@rycee rycee Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
  };
};

Copy link
Collaborator Author

@dermetfan dermetfan Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I amended the fix.

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

dermetfan commented Sep 5, 2017

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

  1. Add all PATH and fpath entries.
  2. Call compinit (-C should be faster than -u). This traverses fpath.
  3. 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.

Copy link
Collaborator

@uvNikita uvNikita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

plugins = mkOption {
type = types.listOf pluginModule;
default = [];
example = [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, amened.

@dermetfan
Copy link
Collaborator Author

dermetfan commented Sep 7, 2017

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
Copy link
Member

rycee commented Sep 8, 2017

@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
Copy link
Collaborator

uvNikita commented Sep 8, 2017

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?

dermetfan added a commit to dermetfan/home-manager that referenced this pull request Sep 8, 2017
In case `compinit` is called from within oh-my-zsh, this passes the security check.

see nix-community#56 (comment)
dermetfan added a commit to dermetfan/home-manager that referenced this pull request Sep 8, 2017
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 added a commit to dermetfan/home-manager that referenced this pull request Sep 8, 2017
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
Copy link
Collaborator Author

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.

(mkIf (builtins.length cfg.plugins > 0) {
programs.zsh.enableCompletion = mkDefault true;

home.file = builtins.listToAttrs (map (plugin: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dermetfan added a commit to dermetfan/home-manager that referenced this pull request Sep 8, 2017
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 added a commit to dermetfan/home-manager that referenced this pull request Sep 8, 2017
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)
Copy link
Collaborator

@uvNikita uvNikita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
}
{
file = "init.sh";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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.";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dermetfan added a commit to dermetfan/home-manager that referenced this pull request Sep 9, 2017
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 added a commit to dermetfan/home-manager that referenced this pull request Sep 9, 2017
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
Copy link
Collaborator

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)
dermetfan added a commit to dermetfan/home-manager that referenced this pull request Sep 11, 2017
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 dermetfan force-pushed the zsh-plugins branch 2 times, most recently from 11a10b1 to 38dd49b Compare September 11, 2017 11:33
@dermetfan
Copy link
Collaborator Author

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rycee
Copy link
Member

rycee commented Sep 11, 2017

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
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Collaborator Author

dermetfan commented Sep 12, 2017

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

uvNikita pushed a commit that referenced this pull request Sep 12, 2017
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
Copy link
Collaborator

Rebased changes into master. Thank you for the contribution!

@uvNikita uvNikita closed this Sep 12, 2017
@dermetfan dermetfan deleted the zsh-plugins branch September 12, 2017 13:30
@dermetfan dermetfan mentioned this pull request Sep 12, 2017
pasqui23 pushed a commit to pasqui23/home-manager that referenced this pull request Oct 14, 2017
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants