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

vimUtils: customize accepts alternate {g}vimExecutable value #87639

Open
wants to merge 1 commit into
base: master
from

Conversation

@mitchellh
Copy link

mitchellh commented May 12, 2020

Motivation for this change

I use Nix on darwin and use MacVim. MacVim uses "mvim" as the shell executable to launch the GUI version. I want to wrap pkgs.macvim with vimUtil.makeCustomizable so I can customize the vimrc but this doesn't work because of the hardcoded "vim" and "gvim" executables.

I have to use makeCustomizable rather than call vimWithRC directly because programs.vim in home-manager uses pkgs.vim_configurable to set all the Vim settings. So I have to make an overlay that sets pkgs.vim_configurable to the customizable version of macvim. But this is a blocker! See the vim module in home-manager here. Also, calling vimWithRC directly in general feels kinda weird, like I'm digging too far into the abstraction.

This introduces an argument to set the base name for these commands. I'm unsure what to name this so open to other requests.

This is my first PR so please let me know what else needs to be done. I followed the CONTRIBUTING.md guidelines as close as possible!

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.
This enables vim implementations that don't strictly use the "vim" and
"gvim" pattern for CLI and graphical respectively to still be wrapped
using customize so that a vimrc can be generated.

The primary example is MacVim on Darwin, which uses "vim" and "mvim".
@mitchellh mitchellh requested a review from jonringer as a code owner May 12, 2020
@mitchellh
Copy link
Author

mitchellh commented May 12, 2020

An alternate idea I had but more invasive was to change customize or vimWithRC to accept a list of binary names to write a script for, i.e. vimExecutables = [ "vim" "gvim" ] or something so we could accommodate additional binaries. But, it felt too invasive and difficult to maintain backwards compatibility.

Copy link
Contributor

jonringer left a comment

Workflow looks to be transparent to users that are already using this.

@jonringer jonringer requested review from timokau and evanjs May 12, 2020
@jonringer
Copy link
Contributor

jonringer commented May 12, 2020

Also, thanks for opening your first PR with nixpkgs @mitchellh :)

@jonringer
Copy link
Contributor

jonringer commented May 12, 2020

I use Nix on darwin and use MacVim. MacVim uses "mvim" as the shell executable to launch the GUI version. I want to wrap pkgs.macvim with vimUtil.makeCustomizable so I can customize the vimrc but this doesn't work because of the hardcoded "vim" and "gvim" executables.

If an installation of gvim and macvim are mutually exclusive, then it may also make sense for the macvim package to also export a gvim alias/symbolic link to mvim. I don't really use mac, so I'm not 100% sure.

This could be similar to viAlias that exists for neovim which create symlinks in the underlying neovim package https://github.com/rycee/home-manager/blob/e9945ee6eefee0e261fe4d5b3a7c5de4f0f8d91b/modules/programs/neovim.nix#L42

@mitchellh
Copy link
Author

mitchellh commented May 12, 2020

I use Nix on darwin and use MacVim. MacVim uses "mvim" as the shell executable to launch the GUI version. I want to wrap pkgs.macvim with vimUtil.makeCustomizable so I can customize the vimrc but this doesn't work because of the hardcoded "vim" and "gvim" executables.

If an installation of gvim and macvim are mutually exclusive, then it may also make sense for the macvim package to also export a gvim alias/symbolic link to mvim. I don't really use mac, so I'm not 100% sure.

This could be similar to viAlias that exists for neovim which create symlinks in the underlying neovim package https://github.com/rycee/home-manager/blob/e9945ee6eefee0e261fe4d5b3a7c5de4f0f8d91b/modules/programs/neovim.nix#L42

Ah! So MacVim does BOTH so it supports gvim already. But at least in circles I’m familiar with it seems more idiomatic on Mac to use “mvim”. So this is just trying to get nix to play nice with the idiom.

Hence that other comment of proposing maybe a new config to support multiple but that’d be a deeper change.

I’m open to any of this

@timokau
Copy link
Member

timokau commented May 12, 2020

I see two alternative options:

The most expressiveway to model this would be a mapping from input name to output name (deprecating vimExecutableName and gvimExecutableName). This would default to

wrapperMapping = {
    "vim" = "my-custom-vim-executable-name";
    "gvim" = "gvim";
}

The simplest way to do it would be to just always wrap mvim if it exists. We could also add an additional mvimExecutableName or just let the user use gvimExecutableName for that purpose. I'd argue the renaming functionality should not have been added in the first place anyway.

Those are the two ends of the complexity spectrum, with your solution living somewhere in the middle. I think I'd favor the simplest option, configuring vim is already complicated enough.

@timokau timokau requested a review from teto May 12, 2020
@mitchellh
Copy link
Author

mitchellh commented May 12, 2020

Those are the two ends of the complexity spectrum, with your solution living somewhere in the middle. I think I'd favor the simplest option, configuring vim is already complicated enough.

What do you view as the “simplest”? I’m fine with whatever you think should be done and I’m happy to make those changes.

Copy link
Contributor

jonringer left a comment

assuming that name is usually vim and gvimExecutableName is usually gvim, I think this was the original intended behavior.

vimExecutable ? "vim",
gvimExecutable ? "gvim",
Comment on lines +402 to +403

This comment has been minimized.

Copy link
@jonringer

jonringer May 12, 2020

Contributor
Suggested change
vimExecutable ? "vim",
gvimExecutable ? "gvim",
vimExecutable = "${vim}/bin/${vimExecutable}";
gvimExecutable = "${vim}/bin/${gvimExecutable}";
Comment on lines +405 to +406

This comment has been minimized.

Copy link
@jonringer

jonringer May 12, 2020

Contributor
Suggested change
vimExecutable = "${vim}/bin/${vimExecutable}";
gvimExecutable = "${vim}/bin/${gvimExecutable}";
vimExecutable = "${vim}/bin/${vimExecutableName}";
gvimExecutable = "${vim}/bin/${gvimExecutableName}";
@mitchellh
Copy link
Author

mitchellh commented May 12, 2020

I'm good with those suggestions of reusing the existing arguments in a smarter way. I think that has a bigger risk of breaking userspace if anyone out there is doing something wacky like vimExecutableName = "something-else" (which would've worked prior).

@timokau
Copy link
Member

timokau commented May 13, 2020

Those are the two ends of the complexity spectrum, with your solution living somewhere in the middle. I think I'd favor the simplest option, configuring vim is already complicated enough.

What do you view as the “simplest”? I’m fine with whatever you think should be done and I’m happy to make those changes.

The second option, having it "just work". That means withVimRC just checks if there's a mvim executable and if there is one just wraps that one as well. No additional configuration necessary.

I'd wait until at least one other person agrees with this before implementing it though. The current implementation is also fine, but I'm very hesitant to add even more complexity to the vim configuration from a user perspective.

I'm good with those suggestions of reusing the existing arguments in a smarter way. I think that has a bigger risk of breaking userspace if anyone out there is doing something wacky like vimExecutableName = "something-else" (which would've worked prior).

Yes, support renaming the executable is exactly the thing which I think should not have been added in the first place. Now it would need proper deprecation to remove though, which is out of scope for this PR.

@mitchellh
Copy link
Author

mitchellh commented May 14, 2020

Agreed @timokau.

@jonringer not sure what your thumbs up on my last comment is supposed to imply 😄

@jonringer
Copy link
Contributor

jonringer commented May 14, 2020

I'm largely of the opinion that the macvim package should just also export a gvim link, similar to llvm exporting a "false" gcc installation.

  postInstall = ''
    ln -s $out/bin/mvim $out/bin/gvim
  '';
@jonringer
Copy link
Contributor

jonringer commented May 14, 2020

@jonringer not sure what your thumbs up on my last comment is supposed to imply smile

Just agreeing that it has the potential to break users who have set it to something else.

@mitchellh
Copy link
Author

mitchellh commented May 14, 2020

I'm largely of the opinion that the macvim package should just also export a gvim link, similar to llvm exporting a "false" gcc installation.

  postInstall = ''
    ln -s $out/bin/mvim $out/bin/gvim
  '';

It already does this, and this works no problem. Its just more idiomatic to use mvim with it. I think I mentioned this a few comments up. So, if the stance is we don't want to support mvim, that's fine its just a somewhat weird experience and have to rewire a decade or so of muscle memory. 😄

@jonringer
Copy link
Contributor

jonringer commented May 14, 2020

It already does this, and this works no problem. Its just more idiomatic to use mvim with it. I think I mentioned this a few comments up. So, if the stance is we don't want to support mvim, that's fine its just a somewhat weird experience and have to rewire a decade or so of muscle memory. smile

Oh, right, the mvim isn't being modified by home-manager, so it's just a vanilla experience.

"In computer science, there's three values which make sense: 0, 1, many"
Maybe it would be best to just have an array with executablesToWrap ? [ "vim" "gvim" ]. This would also future proof against other editors which make have different name, but support vim plugins/configuration.

@timokau
Copy link
Member

timokau commented May 14, 2020

Maybe it would be best to just have an array with executablesToWrap ? [ "vim" "gvim" ]. This would also future proof against other editors which make have different name, but support vim plugins/configuration.

If you then make that silently ignore the case where the executable is not present and default to [ "vim" "gvim" "nvim" "mvim" ] that would also cover my suggestion and work without configuration most of the times.

If we wanted to get fancy, we could also add a vimExecutables passthru to the different vims, but that is probably overkill.

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

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