Fix vim-plugin dependencies #52767
Merged
Fix vim-plugin dependencies #52767
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@GrahamcOfBorg build vimUtils.test_vim_with_vim_nix vimUtils.test_vim_with_vim_nix_using_pathogen vimUtils.test_vim_with_vim_nix_using_plug vimPackages.vim-nix vimPackages.ncm2-jedi |
timokau
force-pushed
the
vim-plugin-updates
branch
from
420fe7b
to
177c600
Compare
timokau
commented
Dec 24, 2018
timokau
force-pushed
the
vim-plugin-updates
branch
5 times, most recently
from
0098853
to
b1a5016
Compare
|
Finally fixed the eval issues. |
Mic92
reviewed
Dec 24, 2018
|
Dependencies are strings for historically reason when the packages where generated by a hacky vim script. |
Mic92
reviewed
Dec 24, 2018
Vim plugins were previously represented as strings by default, necessitating a `knownPlugins` set. Backwards compatibility is kept (strings are still accepted), so vam plugins should continue to work.
nativeImpl previously simply ignored dependency information.
timokau
force-pushed
the
vim-plugin-updates
branch
from
b1a5016
to
8e8a09b
Compare
|
If nobody objects I'll merge this on friday (2018-12-28, Berlin time). |
|
Just noticed that the other two people I pinged haven't been very active the past year, so probably no point in waiting for a response from them. Are you happy with the current version @Mic92? |
Mic92
reviewed
Dec 27, 2018
pkgs/misc/vim-plugins/update.py
Outdated
| @@ -310,6 +310,7 @@ def generate_nix(plugins: List[Tuple[str, str, Plugin]]): | |||
| f""" | |||
| {plugin.normalized_name} = buildVimPluginFrom2Nix {{ | |||
| name = "{plugin.normalized_name}-{plugin.version}"; | |||
There was a problem hiding this comment.
Would that work?
Suggested change
| name = "{plugin.normalized_name}-{plugin.version}"; | |
| version = plugin.version; |
There was a problem hiding this comment.
Because stdenv is computing name anyway.
There was a problem hiding this comment.
Not quite as easily, because buildVimPlugin does its own name building. But I have pushed a new commit to add that functionality. I've also refactored the buildVimPlugin related functions into its own file, since vim-utils.nix is doing too much.
|
I also figured out how to use fixed points (by copying what python is doing) so its way less hacky now. |
timokau
force-pushed
the
vim-plugin-updates
branch
from
0656360
to
490c23a
Compare
timokau
force-pushed
the
vim-plugin-updates
branch
from
490c23a
to
af46529
Compare
|
Sorry for all the force-pushes, I don't know how to test eval locally so I'm dependent on ofBorg for that. Should be good to go now. |
|
@timokau I use the following #!/bin/sh
echo "Checking evaluation before committing";
exec nix-env -f . -qaP --meta --json --drv-path --show-trace >/dev/null |
|
Thanks, that's very useful. |
The override actually broke the build, no idea why it was added originally. The build works fine without it.
timokau
added a commit
to timokau/nixpkgs
that referenced
this pull request
Dec 30, 2018
I messed that up in my refactoring in NixOS#52767, since I moved the relevant bash code out of a function and failed to adjust the shell variable name. Now the plugin build will fail loudly when help tag generation fails to make sure this doesn't happen again.
10 tasks
timokau
added a commit
that referenced
this pull request
Dec 30, 2018
I messed that up in my refactoring in #52767, since I moved the relevant bash code out of a function and failed to adjust the shell variable name. Now the plugin build will fail loudly when help tag generation fails to make sure this doesn't happen again.
10 tasks
|
Can we get a release notes update? My neovim plugins are all broken now with the following error, which I'm fine with breaking changes, but lets make sure we have a note in the release notes: |
|
@disassembler breaking the API was not the intention, you should open an issue on that with the code you tried. |
10 tasks
|
@disassembler yes it was very much not my intention to break the API, sorry about that. I think I figured out the issue from your trace and it should be fixed in #53113. The issue was in |
timokau
added a commit
to timokau/nixpkgs
that referenced
this pull request
Dec 31, 2018
I broke this in NixOS#52767 and didn't notice because I only tested with vim and `requiredPlugins` is only used by neovim. This would break setups that use string-plugins (like pathogen) with neovim.
This was referenced Feb 19, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
I noticed that plugin dependencies simply did not work with the native vim plugin manager. It simply was not implemented, which was surprising.
To implement it, I first had to refactor vim-utils.nix. Now vim plugins are derivations by default, although the "name + knownPlugins" variant is still possible for backwards compatibility. I can't really see a reason why it was strings in the first place.
I also added a new ncm2 completion backend and updated the plugins (same PR since it would conflict otherwise).
Because of the autogenerated file this will bitrot fast, so I would like to get this merged relatively quickly.
Based on git-shortlog: @andsild, @jagajaga, @Mic92
I have build and tested the provided test instances with the different plugin managers. While doing that I also fixed the pathogen implementation that was already broken. I verified that the installed plugins shows up under
:scriptnames.Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)