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

wrap neovim but without its configuration file #101265

Merged
merged 7 commits into from Oct 29, 2020
Merged

Conversation

@teto
Copy link
Contributor

@teto teto commented Oct 21, 2020

Motivation for this change

Follow up of #101100.

Wrapping the init.vim has some sideeffects #55376 .
This PR allows to call wrapNeovim without wrapping its configuration file. Makes sense for home-manager where it can just drop the init.vim in the correct position (also for hackers who would prefer --cmd "source generatedinit.vim instead of the current -u generatedinit.vim.

I've tried to make commits valid on their own but if we agree on the architecture I can squash them:

  • first commit introduces neovimUtils.makeNeovimConfig which returns (mostly) { neovimRcContent (so that home-manager can choose where to write it), wrappedArgs (arguments to pass to the wrapper) }, these arguments can then be completed by the caller depending on the needs.
  • second commit introduces wrapNeovim2 (a lower level wrapper), and neovimUtils.legacyWrapper in order not to break older configurations.

I haven't thoroughly tested the part with the manifest generation.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@teto
Copy link
Contributor Author

@teto teto commented Oct 21, 2020

cc @timokau (just tell me if I should stop nagging you :p )

Copy link
Member

@timokau timokau left a comment

No worries, I'm glad somebody with the necessary know-how is improving the infrastructure :) Generally looks good to me, I haven't tried to understand every detail though.

pkgs/applications/editors/neovim/utils.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/neovim/utils.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/neovim/utils.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/neovim/utils.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/neovim/utils.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/neovim/wrapper.nix Outdated Show resolved Hide resolved
@teto teto force-pushed the teto:neovim_nowrap branch from d12d23c to 7909202 Oct 23, 2020
@teto
Copy link
Contributor Author

@teto teto commented Oct 23, 2020

Here I've provided an home-manager example on how to use it to generate ~/.config/nvim/init.vim https://github.com/teto/home-manager/blob/8509c5821acc2cb5c83d65af68dc7c74579700ee/modules/programs/neovim.nix#L260 .

Next step (in other PRs) would be to replace the opaque configure by some more descriptive fields. I would like to pass a list of plugins (with their respective config) as is done in home-manager: it's pretty convenient.
Once the makeNeovimConfig stabilizes, we could deprecate the old wrapper (or not ^^).

@timokau
Copy link
Member

@timokau timokau commented Oct 26, 2020

I would like to pass a list of plugins (with their respective config)

I'm not sure if that is what you mean, but it would be really neat if it was possible to specify both the plugins and the related vimrc snippets (to be executed pre/post load) in the same place. This is what I currently do (I think its partly broken and assumes everything is loaded at startup though):

https://github.com/timokau/dotfiles/blob/c3340392081c126b5d1cf8cd3e7c519d1cdeb202/home/neovim.nix#L49

@teto
Copy link
Contributor Author

@teto teto commented Oct 26, 2020

Yes the goal is to be able to specify the kind of config you have.
Home-manager has started going this way (to my great satisfaction): https://github.com/teto/home/blob/master/nixpkgs/hm/neovim.nix#L91. Endgoal is to have this kind of configuration in makeNeovimConfig without needing a nixos module.
But I plan to test things first with the home-manager module. My first neovim config revamp was a nightmare to maintain/rebase which is why I prefer to do it more iteratively now.

@teto
Copy link
Contributor Author

@teto teto commented Oct 26, 2020

Maybe @doronbehar you could test that it doesn't break your config ? (just a rebuild). I sense you may have an advanced configuration :)

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 27, 2020

Haven't done a full review or a test, but I am on the imperative side of these kind of configurations, so my nix Neovim config is not that complicated:

https://gitlab.com/doronbehar/nixos-configs/blob/90c39b8705f8f1eca8b02dec5b0694564e98a0dd/shell-programs.nix#L189-242

teto added 6 commits Oct 21, 2020
Endgoal is to be able to more easily generate per project neovim (with
python config for a python project etc).
Also one desirable feature is to be able to wrap neovim without the `-u`
command: for instance home-manager could thus write the config in the
expected $XDG_CONFIG_HOME/nvim/init.vim and neovim works as expected
with .exrc / MYVIMRC (features that `-u` interferes with).
to preserve backwards compatibility while introducing a lower level
wrapper called wrapperNeovim2.

This gives more possibilities to the caller.
@teto teto force-pushed the teto:neovim_nowrap branch from cbffc81 to 11781ab Oct 27, 2020
@teto
Copy link
Contributor Author

@teto teto commented Oct 27, 2020

thanks to the review, it's in much better shape. I fixed the manifest issue. From my point of view it's ready to be squashed and merged notwithstanding CI. It should be fully equivalent to the current state. Once that's merged, it will be easier to improve plugin support.

Copy link
Member

@timokau timokau left a comment

Looks good to me. Just some comments on the exact "unstable" semantics to address or not, based on your judgement and plans.

pkgs/top-level/all-packages.nix Show resolved Hide resolved
@timokau
Copy link
Member

@timokau timokau commented Oct 29, 2020

Alright, then 🚀?

@teto teto merged commit 2eb1610 into NixOS:master Oct 29, 2020
17 checks passed
17 checks passed
@github-actions
tests
Details
@github-actions
action
Details
@ofborg
Evaluation Performance Report Evaluator Performance Report
Details
@github-actions
Wait for ofborg
Details
@ofborg
grahamcofborg-eval ^.^!
Details
@ofborg
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
@ofborg
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
@ofborg
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2f98b69"; rev="2f98b69917002cdfb254828e32f013cc337cd3a9"; } ./pkgs/t
Details
@ofborg
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
@ofborg
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2f98b69"; rev="2f98b69917002cdfb254828e32f013cc337cd3a9"; } ./nixos/
Details
@ofborg
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2f98b69"; rev="2f98b69917002cdfb254828e32f013cc337cd3a9"; } ./nixos/
Details
@ofborg
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2f98b69"; rev="2f98b69917002cdfb254828e32f013cc337cd3a9"; } ./nixos/
Details
@ofborg
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2f98b69"; rev="2f98b69917002cdfb254828e32f013cc337cd3a9"; } ./pkgs/t
Details
@ofborg
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2f98b69"; rev="2f98b69917002cdfb254828e32f013cc337cd3a9"; } ./pkgs/t
Details
@ofborg
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="2f98b69"; rev="2f98b69917002cdfb254828e32f013cc337cd3a9"; } ./pkgs/t
Details
@ofborg
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
@ofborg
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@teto teto deleted the teto:neovim_nowrap branch Oct 29, 2020
@teto
Copy link
Contributor Author

@teto teto commented Oct 29, 2020

next step will be to map some config with a plugin. I would like to make it easy to merge configs.

@LnL7
Copy link
Member

@LnL7 LnL7 commented Oct 30, 2020

@teto This broke neovim.configure, eg. a simple example doesn't load the specified plugins or rc file anymore. The config file is completely empty apart from set nocompatible.

with import ./. {};

neovim.override {
  configure = {
    packages.nix.start = with vimPlugins; [ ale deoplete-nvim ];
    customRC = ''
      echoe "Hello World!"
    '';
  };
}
@teto
Copy link
Contributor Author

@teto teto commented Oct 30, 2020

my bad will try to fix it by tomorrow.

@teto
Copy link
Contributor Author

@teto teto commented Oct 31, 2020

@LnL7 to be honest, I had not thought of the override usecase, especially to modify the customRC part. I think it's fair to break the override scenario which is more a hacker-like scenario IMO (hoping it doesn't break for too many people). Would you mind using wrapNeovim instead ? Otherwise I could throw a warning when configure is passed.

Also this broke home-manager in some cases (doubly so since I messed up the lib.warn call to explain wrapper arguments should now be a list instead of a string an array :s Since it's not a super useful change I will revert wrap arguments to be a string again). nix-community/home-manager#1577

@teto teto mentioned this pull request Oct 31, 2020
3 of 10 tasks
@LnL7
Copy link
Member

@LnL7 LnL7 commented Oct 31, 2020

@teto I'm not particularly opposed to changing it but it's not something that should be changed without warning.

The snipped I showed is what's documented in various places like the the nixpkgs manual, wiki, etc... and thus will be the way almost everybody configures it currently. Given that most of the references and usages are outside of nixpkgs I consider this part of the api that should go through a deprecation cycle if it's changed.

@teto
Copy link
Contributor Author

@teto teto commented Oct 31, 2020

oh shit you are right ! didn't realize this was the standard way. Gonna fix this too then.

teto added a commit to teto/nixpkgs that referenced this pull request Oct 31, 2020
the compatibility layer for `wrapNeovim` introduced in NixOS#101265
broke the recommended way of configuration:
https://nixos.org/manual/nixpkgs/stable/#custom-configuration
https://nixos.wiki/wiki/Vim#Customizations

restore the old ways.
@teto
Copy link
Contributor Author

@teto teto commented Oct 31, 2020

@LnL7 I've added a commit to fix it in #102231

@kalekseev
Copy link
Contributor

@kalekseev kalekseev commented Nov 2, 2020

@teto seems like nodejs plugin is broken now, my overlay enables nodejs and disables ruby:

self: super: {
  neovim = super.neovim.override {
    extraPython3Packages = (ps: [ps.pythonPackages.jedi]);
    withNodeJs = true;
    withRuby = false;
  };
}

I get opposite effect, bin contains ruby host prog but no nodejs

❯ ls -la /nix/store/y05mzgy7lzkjxvvh73n1pym03dc4qvk5-neovim-0.4.4/bin/
total 24
dr-xr-xr-x  6 wtf  admin  192 Jan  1  1970 .
dr-xr-xr-x  5 wtf  admin  160 Jan  1  1970 ..
-r-xr-xr-x  1 wtf  admin  749 Jan  1  1970 nvim
-r-xr-xr-x  1 wtf  admin  175 Jan  1  1970 nvim-python
-r-xr-xr-x  1 wtf  admin  176 Jan  1  1970 nvim-python3
lrwxr-xr-x  1 wtf  admin   80 Jan  1  1970 nvim-ruby -> /nix/store/yhy98cq45c2fijax5kd5y1xiagsfg467-neovim-ruby-env/bin/neovim-ruby-host
@teto
Copy link
Contributor Author

@teto teto commented Nov 2, 2020

ok I will look into it in a few hours.

@teto teto mentioned this pull request Nov 2, 2020
2 of 10 tasks
@teto
Copy link
Contributor Author

@teto teto commented Nov 2, 2020

@kalekseev I've submitted a fix. I am glad unstable is blocked for now :| maybe I would need to write some tests before further refactoring but maybe after the 0.5 release. So much to do till then.

@jonringer
Copy link
Contributor

@jonringer jonringer commented Nov 3, 2020

Users of the home-manager neovim module are also broken by this.

@teto
Copy link
Contributor Author

@teto teto commented Nov 3, 2020

@jonringer shouldnt' be the cae on master anmore (I have yet to check vimAlias but should be fixed too).

@jonringer
Copy link
Contributor

@jonringer jonringer commented Nov 3, 2020

When I originally rebased my other PR, it was before your build fixes.

aliases do need to be fixed

$ vim
The program ‘vim’ is currently not installed. It is provided by
several packages. You can install it by typing one of the following:
  nix-env -iA nixos.vim
  nix-env -iA nixos.vimHugeX

I just developed a muscle memory around tryping vim, and a lot of my "aliases" were written in mind with this.

jonringer added a commit to jonringer/nixpkgs-config that referenced this pull request Nov 3, 2020
module configuration was broken in NixOS/nixpkgs#101265
@jonringer
Copy link
Contributor

@jonringer jonringer commented Nov 3, 2020

honestly, i think this should all be reverted, and continued in another branch.

EDIT: This has evolved over several years, and many users consume this in non-obvious ways. There's a nixos module, home-manager module, and wiki's which have code in which this does not play well with these changes.

At the very least, move this over to a different top-level attr, and allow some people some time to migrate over to the "new API" with some deprecation process similar to @LnL7 was saying.

@teto
Copy link
Contributor Author

@teto teto commented Nov 3, 2020

I've tried before to do it out of tree but 1/ it was hard to maintain. 2/ It means I would drop a huge patch.

I've written both the home-manager and nixos modules. My endgoal is to make it possible to generate vim configs and write them anywhere (init.vim / .nvimrc) without the drawbacks of using -u. This allows to have per-project tailor-made nvim configurations (that could be then sharable or generated, akin to haskellPackages.shellFor ).

The goal with this PR is to make this work easier to maintain while not impacting users via a thin legacy wrapper. I kinda messed up the compatibility layer but now all should be fine. Lucky us I could fix those before it reached nixos-unstable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants