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

vim-plugins: turn filetype and syntax before sourcing the plugins #77143

Merged

Conversation

@kalbasit
Copy link
Member

@kalbasit kalbasit commented Jan 6, 2020

Motivation for this change

Currently, all the plugins are loaded after all the plugins have already been loaded. This could break some plugins, for instance, vim-terraform assumes the filetype detection has already been enabled.

This commit follows VAM's recommendation and enables filetype and syntax before sourcing any of the plugins.

This was discovered after a follow up with vim-terraform's maintainer on an upstream pull request: hashivim/vim-terraform#133

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.
Notify maintainers

cc @timokau @gloaming

kalbasit added 2 commits Jan 6, 2020
…ins (#66536)"

This reverts commit a3bf0c2.
…utocmd (#76845)"

This reverts commit fa92f00.
@kalbasit
Copy link
Member Author

@kalbasit kalbasit commented Jan 6, 2020

Please backport this to 19.09.

@jonringer jonringer requested a review from Ma27 Jan 6, 2020
@dimbleby
Copy link

@dimbleby dimbleby commented Jan 6, 2020

It's possible that you're going to get into a tangle here: some plugin managers have the opposite requirement. Eg pathogen asks you to turn filetype on after doing its thing and vundle explicitly asks you to set filetype off first...

Good luck!

@kalbasit
Copy link
Member Author

@kalbasit kalbasit commented Jan 6, 2020

It's possible that you're going to get into a tangle here: some plugin managers have the opposite requirement. Eg pathogen asks you to turn filetype on after doing its thing and vundle explicitly asks you to set filetype off first...

Good luck!

Ah, thank you for pointing that out @dimbleby. I removed vundle and neobundle since they're not actually implemented and will probably include the settings directly with the plugin implementation for this to work correctly.

kalbasit added 2 commits Jan 6, 2020
@kalbasit kalbasit force-pushed the kalbasit:nixpkgs_enable-syn-ft-before-plugins branch from 3131b33 to dba76a1 Jan 6, 2020
Currently, all the filetype and syntax are enabled *after* all the plugins has
already been loaded. Whilst this is the case for Pathogen, it's not
recommended when using VAM.

This commit applies the recommendation for:
- VAM[0]: The filetype and syntax are enabled *before* the plugins are loaded.
- Pathogen[1]: The filetype and syntax are enabled *after* the plugins are loaded.
- Plug[2]: The filetype and syntax are automatically enabled.

[0]: https://github.com/MarcWeber/vim-addon-manager/tree/d9e865f3c2de5d9b7eabbc976f606cf1b89e29ea#recommended-setup
[1]: https://github.com/tpope/vim-pathogen/blob/a553410f1bdb9000fbc764069f3a5ec3454a02bc/README.markdown#runtime-path-manipulation
[2]: https://github.com/junegunn/vim-plug/blob/2f5f74e5e67f657e9fdac54891a76721bcd3ead3/README.md#usage
@kalbasit kalbasit force-pushed the kalbasit:nixpkgs_enable-syn-ft-before-plugins branch from dba76a1 to f18b391 Jan 6, 2020
@kalbasit
Copy link
Member Author

@kalbasit kalbasit commented Jan 6, 2020

I tested the following implementation with the vim-terraform plugin since it's the only one that falls on me if filetype was not enabled properly at the right time.

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

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

All the above-compiled vim were then tested by opening a Terraform file, I see no errors and the filetype is set correctly to terraform instead of tf.

I was not able to verify that vim-terraform is able to load correctly with the native plugins (I'm not sure how that even works), so I left that implementation unchanged by adding the filetype indent plugin on | syn on at the end of the implementation. I tried with the following command

nix-build -I nixpkgs=. -E 'with import <nixpkgs> {}; neovim.override { configure = { packages.myVimPackage = { start = [ "vim-terraform" ]; }; }; }' --show-trace
@timokau
Copy link
Member

@timokau timokau commented Jan 7, 2020

Didn't you open a similar PR already? This all seems very familiar to me.

Unfortunately I'm pretty short on time currently, so I can't test this extensively. I'm not entirely sure if turning both of these on is the right thing to do, but I'll let you and the other people interested in vim decide here.

@kalbasit
Copy link
Member Author

@kalbasit kalbasit commented Jan 7, 2020

Yes, indeed.

In #66536, we fixed the manifest generation but the runtime was still broken. This PR reverts #66536 and just fixes the root cause of us not respecting the recommend usage of the plugins we support, Pathogen, Vam and Plug.

Copy link
Contributor

@jonringer jonringer left a comment

[2 built, 1 copied (32.4 MiB), 6.8 MiB DL]
https://github.com/NixOS/nixpkgs/pull/77143
1 package built:
vimPlugins.vim-terraform
@jonringer
Copy link
Contributor

@jonringer jonringer commented Jan 7, 2020

@GrahamcOfBorg build vimPlugins.vim-terraform

@Ma27
Ma27 approved these changes Jan 7, 2020
@jonringer jonringer merged commit 8dccb59 into NixOS:master Jan 7, 2020
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
vimPlugins.vim-terraform on aarch64-linux Success
Details
vimPlugins.vim-terraform on x86_64-darwin Success
Details
vimPlugins.vim-terraform on x86_64-linux Success
Details
@kalbasit kalbasit deleted the kalbasit:nixpkgs_enable-syn-ft-before-plugins branch Jan 7, 2020
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

5 participants
You can’t perform that action at this time.