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

vimPlugins: turn filetype and syntax before sourcing the plugins #66536

Merged
merged 4 commits into from
Aug 13, 2019
Merged

vimPlugins: turn filetype and syntax before sourcing the plugins #66536

merged 4 commits into from
Aug 13, 2019
+5 −2

Conversation

kalbasit
Copy link
Member

@kalbasit kalbasit commented Aug 12, 2019 •
edited
Loading

Motivation for this change

Vim Terraform expects the filetypedetect group to exist. However, since we were enabling the filetype and the syntax after loading the plugins, it was exiting with an error preventing us from generating the remote plugins manifest with the plugin enabled. See #65894 for context.

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 nix-review --run "nix-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.
Notify maintainers

cc @timokau @gloaming @manveru @rvolosatovs

closes #65894

Sorry, something went wrong.

@kalbasit
vimPlugins: turn filetype and syntax before sourcing the plugins
a47e42c 
Vim Terraform expects the `filetypedetect` group to exist. However,
since we were enabling the filetype and the syntax *after* loading the
plugins, it was exiting with an error preventing us from generating the
remote plugins manifest with the plugin enabled. See NixOS#65894 for context.
@kalbasit
Copy link
Member Author

kalbasit commented Aug 12, 2019 •
edited
Loading

Tested with neovim on NixOS only. Reviewer: Please test with Vim as well as Darwin!

nix-build -I nixpkgs=. -E 'with import <nixpkgs> {}; neovim.override { configure = { vam.pluginDictionaries = [ "vim-terraform" ]; }; }' --show-trace

@ofborg ofborg bot added 6.topic: vim 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Aug 12, 2019
@timokau
Copy link
Member

timokau commented Aug 12, 2019

Not sure this is a good idea. The issue linked by @gloaming mentions that they

  • are not sure if filetype on can be completely undone
  • each filetype on adds ~100ms overhead

@kalbasit
Copy link
Member Author

kalbasit commented Aug 12, 2019 •
edited
Loading

Not sure this is a good idea. The issue linked by @gloaming mentions that they

* are not sure if `filetype on` can be completely undone

* each `filetype on` adds ~100ms overhead

We already had it in the vimRC. I merely moved it to the top.

EDIT: I see what you mean, sorry for missing your point. Another way to solve this is to enable this patch only for the manifest generation. It might still break the result vim. I'll give it a shot.

@kalbasit
Copy link
Member Author

EDIT: I see what you mean, sorry for missing your point. Another way to solve this is to enable this patch only for the manifest generation. It might still break the result vim. I'll give it a shot.

@timokau It seems to work for me if I enabled at the top only for generating the manifest. What do you think of this approach?

@timokau
Copy link
Member

timokau commented Aug 12, 2019

I don't think you missed my point; On the contrary, I missed that we already set filetype on. Not sure why we do that, not really our place to change the defaults. But since we already do...

But then there's still justinmk's point in neovim/neovim#6596 (comment) that this may interfere with plugin managers. So your new solution is probably better. Not sure why plugin managers do the filetype off/filetype on dance in the first place though.

@timokau
Copy link
Member

timokau commented Aug 12, 2019

Instead of earlyFiletype, maybe we want a more general beforePlugins escape hatch? Then we could just say beforePlugins = "filetype ident on". Paying the 100ms twice only for the manifest generation shouldn't matter.

@kalbasit
Copy link
Member Author

@timokau sure that sounds like a good idea. I've added beforePlugins and afterPlugins as it makes sense to have both. Obviously, the manifest generation only needs the beforePlugins.

PTAL

@timokau
Copy link
Member

timokau commented Aug 13, 2019

Isn't afterPlugins == customRC? I wonder what other plugin managers do. If they also don't set filetype on, how is it plugins like vim-terraform don't fail for them?

PTAL

I don't know that acronym.

@kalbasit
Copy link
Member Author

Isn't afterPlugins == customRC?

That's right. I removed it then.

I wonder what other plugin managers do. If they also don't set filetype on, how is it plugins like vim-terraform don't fail for them?

I'm not sure actually, that's a good question. We could take this discussion on a separate issue and look deeper into plugins. Maybe @junegunn could chime in here as well.

PTAL

I don't know that acronym.

Please Take Another Look :)

@timokau
Copy link
Member

timokau commented Aug 13, 2019

I wonder what other plugin managers do. If they also don't set filetype on, how is it plugins like vim-terraform don't fail for them?

I'm not sure actually, that's a good question. We could take this discussion on a separate issue and look deeper into plugins. Maybe junegunn could chime in here as well.

Right, I'm fine with merging this for now if it fixes the immediate build failure 👍. Anything else can be done as a followup.

Please Take Another Look :)

Good to go :)

@kalbasit kalbasit merged commit a3bf0c2 into NixOS:master Aug 13, 2019
@kalbasit kalbasit deleted the nixpkgs_65894 branch August 13, 2019 19:25
@kalbasit kalbasit mentioned this pull request Aug 26, 2019
Merged
This was referenced Jan 2, 2020
Closed
Merged
Merged
kalbasit added a commit to kalbasit/nixpkgs that referenced this pull request Jan 6, 2020
@kalbasit
Revert "vimPlugins: turn filetype and syntax before sourcing the plug…
23541b6 
…ins (NixOS#66536)"

This reverts commit a3bf0c2.
@kalbasit kalbasit mentioned this pull request Jan 7, 2020
Merged
10 tasks
jonringer pushed a commit that referenced this pull request Jan 7, 2020
@kalbasit
Revert "vimPlugins: turn filetype and syntax before sourcing the plug…
7909787 
…ins (#66536)"

This reverts commit a3bf0c2.
@kalbasit kalbasit mentioned this pull request May 6, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timokau timokau

Assignees
No one assigned
Labels
6.topic: vim 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vim-terraform plugin breaks neovim's manifest generation.
2 participants
@kalbasit @timokau