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: expose the neovim-unwrapped manpage #87929

Closed
wants to merge 2 commits into from

Conversation

0paIescent
Copy link

Motivation for this change

Install the neovim manpage along with the neovim package. Before, the manpage was only installed with neovim-unwrapped.

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 (Fedora, Void)
  • 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) (Difference of +624)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

This wrapper gets more and more complicated, I wonder why doesn't it use symlinkJoin as it would just grab everything there is in neovim-unwrapped. But that's out of scope for this PR. Could you move your addition a bit upwards? Before optionalString (!stdenv.isDarwin) so Darwin users will enjoy this enhancement it as well?

@0paIescent
Copy link
Author

I had thought about that as well, I don't think symlinkJoin would be the best solution since the binary gets built from makeWrapper, and share is the only other folder involved. I don't see why /share/{locale,man,nvim,pixmaps} couldn't be symlinked, and if not on darwin then symlink and subtitute the desktop file.

I'll submit another PR to add that, but since that PR will just update what gets pushed in this one, should I just close this one and submit the other to avoid a useless commit?

@doronbehar
Copy link
Contributor

Sometimes, in my own overlays, I do something like this:

          octave-wrapped = super.symlinkJoin rec {
            name = "octave-wrapped";
            paths = [ super.octave ];
            pythonEnv = super.python3.withPackages(ps: [
              ps.sympy
            ]);
            nativeBuildInputs = [ super.makeWrapper ];
            # along with some fixes to the desktop file
            postBuild = ''
              cp -L $out/bin/octave $out/bin/octave.tmp
              mv -f $out/bin/octave.tmp $out/bin/octave
              wrapProgram $out/bin/octave --prefix PATH : ${pythonEnv}/bin
              cp -L $out/share/applications/org.octave.Octave.desktop $out/share/applications/org.octave.Octave.desktop.tmp
              mv -f $out/share/applications/org.octave.Octave.desktop.tmp $out/share/applications/org.octave.Octave.desktop
              substituteInPlace $out/share/applications/org.octave.Octave.desktop \
                --replace "Terminal=false" "Terminal=true" \
                --replace "octave --gui" octave
            '';
          };

Which may seem a bit hacky but I think it's not that bad. If I were the original writer of the neovim wrapper function I'd do this. I think it'll be best for you to wait a few days and see what the neovim maintainers think about this.

@0paIescent
Copy link
Author

Right, that is much cleaner. I'll just wait and see what happens.

@teto
Copy link
Member

teto commented May 17, 2020

#87929 (comment) sounds good, no need to open another PR. Thanks for doing this, it has been on my TODO for a while and was surprised no one complained about it (I sure did xD)

@0paIescent
Copy link
Author

@teto Sounds good to me, I might as well mention I've got another branch where I replaced mkDerivation with symlinkJoin if we want to merge that in while we're messing with neovim.

@doronbehar
Copy link
Contributor

That branch looks good. Maybe this is not related to the issue at hand, but if someone wants to wrap Neovim with a configure argument, isn't the end executable gets wrapped twice?

@0paIescent
Copy link
Author

Yeah it'll be wrapped twice, but the arguments passed to the first wrap will be propagated up to the final binary. I don't really see a clean way to only wrap once without another optionalString, I feel like that would clutter up the postBuild.

@doronbehar
Copy link
Contributor

I don't really see a clean way to only wrap once without another optionalString , I feel like that would clutter up the postBuild .

I disagree that there's no clean way to do it. The problem is that the current way the wrapper function is implemented is of low quality (if you ask me). I once wrote a generic wrapper Nix function that "compiles" a string which in turn becomes a list of arguments to wrapProgram. For Neovim's wrapper function it should be fairly easy to do something similar, it's just a bit hard to dive into the functional programming concept.

@0paIescent
Copy link
Author

Fair point, however that would be way over my head in terms of how skilled I am in Nix, I wouldn't even know where to start in making something like that.

doronbehar added a commit to doronbehar/nixpkgs that referenced this pull request Jul 31, 2020
Cleanups:
- Removed unneeded neovim.meta.description reset.
- Remove unnecessary -x checks in `postBuild`.
- Use a ${placeholder "out"} if needed.

Changes:
- Switch to symlinkJoin, so e.g manpages link to the environment (NixOS#87929).
- Use nvim-node from $out/bin/ just like all other providers.
- Compute all arguments to makeWrapper in pure Nix "before" `postBuild`.
- Prevent double wrapping in case `configure != {}` and rplugin.vim
  needs to be generated.

Co-authored-by: Silvan Mosberger <contact@infinisil.com>
@infinisil
Copy link
Member

With #88110, the man pages and everything else is now in the wrapped neovim

@infinisil infinisil closed this Jul 31, 2020
braack pushed a commit to braack/nixpkgs that referenced this pull request Oct 29, 2020
Cleanups:
- Removed unneeded neovim.meta.description reset.
- Remove unnecessary -x checks in `postBuild`.
- Use a ${placeholder "out"} if needed.

Changes:
- Switch to symlinkJoin, so e.g manpages link to the environment (NixOS#87929).
- Use nvim-node from $out/bin/ just like all other providers.
- Compute all arguments to makeWrapper in pure Nix "before" `postBuild`.
- Prevent double wrapping in case `configure != {}` and rplugin.vim
  needs to be generated.

Co-authored-by: Silvan Mosberger <contact@infinisil.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants