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

neovim, neovimUtils, neovim-qt: drop python2 support #121339

Merged
merged 1 commit into from
May 2, 2021

Conversation

mweinelt
Copy link
Member

@mweinelt mweinelt commented May 1, 2021

Motivation for this change

In 2a00e53 pynvim support for python2 was disabled, this broke the
neovim build. I really think it is time to let go of python2 support in
neovim.

Fixes the build for me, I have no regrets.

I'm aware we might need a release notes entry for that. I'm open to suggestions, I've never relied on python2 in neovim.

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)
    /nix/store/synllhwl09qs1jzg9v452npvbgq6y0pf-neovim-0.4.4	  425376328
    /nix/store/zapccq0qrj4k5b94j49ifnqy6i5mzxkx-neovim-0.4.4	  383572776
    
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@mweinelt mweinelt requested a review from teto as a code owner May 1, 2021 00:33
@mweinelt mweinelt requested review from Profpatsch and prusnak May 1, 2021 00:35
@@ -1,5 +1,4 @@
{ lib, stdenv, mkDerivation, fetchFromGitHub, cmake, doxygen, makeWrapper
, msgpack, neovim, pythonPackages, qtbase, neovim-qt-unwrapped }:
{ stdenv, makeWrapper, neovim, neovim-qt-unwrapped }:
Copy link
Member Author

Choose a reason for hiding this comment

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

Drive-by cleanup of inputs, not used since 3a25004.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label May 1, 2021
@ofborg ofborg bot requested review from edanaher and peterhoeg May 1, 2021 00:55
@prusnak prusnak removed their request for review May 1, 2021 07:43
@teto
Copy link
Member

teto commented May 1, 2021

it's a bit annoying to break compatibility inside the same neovim release (python2 is dropped in 0.5) but I am not aware of any python2 remote plugins so it should probably be fine ?

@teto
Copy link
Member

teto commented May 1, 2021

maybe add some warning if python2 is used since it's a breaking change for the same version so it can be surprising.

@veprbl veprbl linked an issue May 1, 2021 that may be closed by this pull request
@mweinelt
Copy link
Member Author

mweinelt commented May 2, 2021

maybe add some warning if python2 is used since it's a breaking change for the same version so it can be surprising.

Sounds great, though not sure where would be a good spot.

@teto
Copy link
Member

teto commented May 2, 2021

when withPython2 is used, add a lib.warn or even a throw with the appropriate message.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation labels May 2, 2021
@mweinelt
Copy link
Member Author

mweinelt commented May 2, 2021

I've added three asserts/throws, can you check whether that is sufficient?

Also extended the release notes for vims python2 support drop with neovim.

Both as fixups, will squash them before merge.

Copy link
Member

@teto teto left a comment

Choose a reason for hiding this comment

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

Looks good to me

@austinbutler
Copy link
Member

Hopefully this is just me doing it wrong, but I can't seem to get this to work in my home-manager config.

{
  programs.neovim = {
    enable = true;
    package = (import (builtins.fetchTarball {
      url =
        "https://github.com/mweinelt/nixpkgs/archive/b91ddf1960e1c825a9bcddcbfa67d4eae0e1b971.tar.gz";
      sha256 = "0jdw4j0r6fkqhqaac5gr4i5nq35pm49wh98idzckrzhsg5h1b7yn";
    }) { config = { allowUnfree = true; }; }).neovim-unwrapped;
    extraConfig = ''
      if (has("nvim"))
        "For Neovim 0.1.3 and 0.1.4 < https://github.com/neovim/neovim/pull/2198 >
        let $NVIM_TUI_ENABLE_TRUE_COLOR=1
      endif

      "For Neovim > 0.1.5 and Vim > patch 7.4.1799 < https://github.com/vim/vim/commit/61be73bb0f965a895bfb064ea3e55476ac175162 >
      "Based on Vim patch 7.4.1770 (`guicolors` option) < https://github.com/vim/vim/commit/8a633e3427b47286869aa4b96f2bfc1fe65b25cd >
      " < https://github.com/neovim/neovim/wiki/Following-HEAD#20160511 >
      if (has("termguicolors"))
        set termguicolors
      endif

      filetype plugin indent on
      syntax on
      set tabstop=4 shiftwidth=4 expandtab
      set clipboard+=unnamedplus
    '';
    extraPython3Packages = (ps: with ps; [ black isort pylint ]);
    plugins = with pkgs.vimPlugins; [
      vim-nix
      coc-nvim
      coc-css
      coc-explorer
      coc-git
      coc-go
      coc-html
      coc-json
      coc-prettier
      coc-pyright
      coc-rust-analyzer
      coc-tsserver
      coc-yaml
      {
        plugin = vim-autoformat;
        # https://github.com/Chiel92/vim-autoformat/blob/master/plugin/defaults.vim
        config = ''
          let g:formatters_javascript = ['prettier']
          let g:formatters_json = ['prettier']
          let g:formatters_python = ['black']
          let g:formatters_typescript = ['prettier']
          let g:formatters_yaml = ['prettier']
        '';
      }
      {
        plugin = vim-one;
        config = ''
          set background=dark
          colorscheme one
        '';
      }
      vim-lastplace
    ];
    viAlias = true;
    vimAlias = true;
    vimdiffAlias = true;
    withNodeJs = true;
    withPython3 = true;
  };
}

I still end up with:

unpacking channels...
error: pynvim-0.4.3 not supported for interpreter python2.7
(use '--show-trace' to show detailed location information)

It seems it should be using the package from your branch since it checks the sha256.

@mweinelt
Copy link
Member Author

mweinelt commented May 2, 2021

The only pynvim occurrence that are left are scoped with python3.pkgs or python3Packages, so I guess you are not using the matching utils or wrapper from this branch.

Squashing now.

In 2a00e53 pynvim support for python2 was disabled, this broke the
neovim build. I really think it is time to let go of python2 support in
neovim.
@teto
Copy link
Member

teto commented May 2, 2021

@SCOTT-HAMILTON does this PR work for you ?

@austinbutler
Copy link
Member

Was able to get it working once I realized home-manager calls wrapNeovimUnstable, I then created an overlay for that package instead. Sorry for the noise, but I can confirm this works for me now.

@teto teto merged commit d942d44 into NixOS:master May 2, 2021
@mweinelt mweinelt deleted the neovim-🔥-python2 branch May 2, 2021 21:29
dotlambda added a commit to dotlambda/home-manager that referenced this pull request May 3, 2021
dotlambda added a commit to dotlambda/home-manager that referenced this pull request May 3, 2021
dotlambda added a commit to dotlambda/home-manager that referenced this pull request May 3, 2021
dotlambda added a commit to dotlambda/home-manager that referenced this pull request May 3, 2021
sumnerevans pushed a commit to nix-community/home-manager that referenced this pull request May 4, 2021
chisui pushed a commit to chisui/home-manager that referenced this pull request May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

neovim fails to build with pynvim-0.4.3
3 participants