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

@0paIescent 0paIescent commented May 16, 2020

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

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

@0paIescent 0paIescent commented May 16, 2020

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

@doronbehar doronbehar commented May 16, 2020

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.

Andrew DeFilippo
@0paIescent
Copy link
Author

@0paIescent 0paIescent commented May 16, 2020

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

@teto
Copy link
Contributor

@teto 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

@0paIescent 0paIescent commented May 18, 2020

@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

@doronbehar doronbehar commented May 18, 2020

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

@0paIescent 0paIescent commented May 18, 2020

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

@doronbehar doronbehar commented May 18, 2020

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

@0paIescent 0paIescent commented May 18, 2020

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 doronbehar mentioned this pull request May 19, 2020
1 of 10 tasks complete
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

@Infinisil Infinisil commented Jul 31, 2020

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

@Infinisil Infinisil closed this Jul 31, 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

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