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.

Sign up for GitHub

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

Jump to bottom

neovim: generate remote plugin manifest #53084

Merged
merged 3 commits into from Jan 21, 2019
Merged

neovim: generate remote plugin manifest #53084

merged 3 commits into from Jan 21, 2019

Conversation

timokau
Copy link
Member

@timokau timokau commented Dec 30, 2018 •
edited

Motivation for this change

This makes sure the user doesn't have to call UpdateRemotePlugins manually for plugins installed through nix. A minor patch to neovim is necessary, but it should be harmless. See neovim/neovim#9413 for a discussion about the patch.

cc @Mic92 @rvolosatovs

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@timokau
Copy link
Member Author

timokau commented Dec 30, 2018

I tested this with the deoplete-nvim plugin. The user can still additionally use UpdateRemotePlugins as before.

@timokau timokau changed the title Neovim remote plugins neovim: generate remote plugin manifest Dec 30, 2018
rvolosatovs
rvolosatovs approved these changes Dec 30, 2018
View reviewed changes
Copy link
Member

@rvolosatovs rvolosatovs left a comment

Choose a reason for hiding this comment

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

Pure awesomeness - great work!
I haven't tested it yet, but I can't come up with a reason why it would not work

pkgs/applications/editors/neovim/wrapper.nix Outdated Show resolved Hide resolved
@timokau timokau force-pushed the neovim-remote-plugins branch 4 times, most recently from 89d0825 to 8db8f71 Compare December 31, 2018 11:45
@timokau
Copy link
Member Author

timokau commented Dec 31, 2018

Given the recent fallout on my last refactor (#53112, #53113) I've added some better error handling and a testcase (we should have more testcases). The error handling is important since we're starting neovim with a vimrc. That vimrc should not contain any errors since it was automatically generated, but in case it does this will make it easier to debug.

I've also added the error handling (e.g. not silently ignoring errors) to the helptag generation, although that is less likely to fail.

I have

  • run nix-review to make sure all plugins and all testcases build
  • verified deoplete works with and without ~/.local/share/nvim/rplugin.vim present

@timokau
Copy link
Member Author

timokau commented Dec 31, 2018

@GrahamcOfBorg build vimUtils.test_nvim_with_remote_plugin

@timokau
Copy link
Member Author

timokau commented Dec 31, 2018

The high rebuild count is because buildVimPlugin was touched. However the vim plugins are all extremely fast to "build".

@teto
Copy link
Member

teto commented Jan 18, 2019

Might be nice to have until neovim adopts a better solution.

@Mic92
Copy link
Member

Mic92 commented Jan 18, 2019

Do you plan to open a pull request for that?

@timokau
Copy link
Member Author

timokau commented Jan 18, 2019

Oops, totally forgot this PR was still open. Thanks for the reminder @teto. I'll rebase this.

@Mic92 will who open a PR for what?

@timokau
neovim: fix indentation
51c4768
@timokau
neovim: generate remote plugin manifest
ab22e8c 
This makes sure the user doesn't have to call `UpdateRemotePlugins`
manually for plugins installed through nix. A minor patch to neovim is
necessary, but it should be harmless. See
neovim/neovim#9413 for a discussion about
the patch.
@timokau
vimUtils.buildVimPlugin: write vim errors to stderr
2bc0e00 
Previously vim would silently fail when help tags couldn't be generated.
We need to pass the "verbose" flag (with verbose level 1) to convince
vim to print errors to standard error.
@timokau
Copy link
Member Author

timokau commented Jan 18, 2019

@GrahamcOfBorg build vimUtils.test_nvim_with_remote_plugin

@timokau
Copy link
Member Author

timokau commented Jan 21, 2019

@Mic92 any objections to merge?

@rvolosatovs
Copy link
Member

FTR, I've been running this for a while now and it seems to work fine.

@Mic92 Mic92 merged commit fa8f134 into NixOS:master Jan 21, 2019
@timokau timokau deleted the neovim-remote-plugins branch January 21, 2019 21:32
@symphorien symphorien mentioned this pull request Jan 25, 2019
Closed
@ghost ghost mentioned this pull request Mar 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 approved these changes

@rvolosatovs rvolosatovs rvolosatovs approved these changes

@garbas garbas Awaiting requested review from garbas

Assignees
No one assigned
Labels
6.topic: vim 10.rebuild-darwin: 1-10 10.rebuild-linux: 101-500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@timokau @teto @Mic92 @rvolosatovs @GrahamcOfBorg