-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Install app on macOS for neovide #319061
Install app on macOS for neovide #319061
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your first contribution, please check Contributing.md to get more insights on how to build a great PR!
Title suggestion: neovide: add darwin app
Commit message suggestion: neovide: add darwin app
The app does not run properly on aarch64-darwin
- I would be expect it to be wrapped so neovim
is added to its PATH
?
EDIT: See @ck3d comment below about relying on neovim
being present on user system
Thanks for the informative comments @matteo-pacini. Looks I was a bit premature in making this PR. I thought it would work because I followed what was done in nixpkgs/pkgs/applications/terminal-emulators/alacritty/default.nix Lines 94 to 98 in 57d6973
|
I assume it's acceptable to amend and force push to change the commit message? LMK what my alternative are otherwise. |
@triarius It is, ideally for this kind of update there should be two commits:
Something else to keep in mind when producing macOS apps derivations (happened to me recently with #318047 and #318535 ).
In your case, I think you only need to |
4c42657
to
4a0226a
Compare
pkgs/by-name/ne/neovide/package.nix
Outdated
@@ -94,9 +94,14 @@ rustPlatform.buildRustPackage.override { stdenv = clangStdenv; } rec { | |||
|
|||
wrapProgram $out/bin/neovide \ | |||
--prefix LD_LIBRARY_PATH : ${libPath} | |||
--prefix PATH : ${lib.makeBinPath [ neovim python3 ]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neovide has no dependency to python3, please remove.
Neovide has a dependency to neovim, but we should rely on the user's environment. I think you increase the closure size of this package with no need. Further more, if the user installed in his environment, it would be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed them. I only put them in because it was not working for @matteo-pacini, presumably because there, it was run in an environment with no neovim install.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically, this:
The app does not run properly on aarch64-darwin - I would be expect it to be wrapped so neovim is added to its PATH?
But I see now that you both agree that it should use a neovim from the environment. So I've removed it.
pkgs/by-name/ne/neovide/package.nix
Outdated
postInstall = if stdenv.isDarwin then '' | ||
mkdir -p $out/Applications | ||
cp -r extra/osx/Neovide.app $out/Applications | ||
ln -s $out/bin/.neovide-wrapped $out/Applications/Neovide.app/Contents/MacOS/neovide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You created a wrapped neovide, but you use the unwrapped one, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake, I misinterpreted this comment to mean I was not using the wrapped executable in the .app bundle. I then I thought .neovide-wrapped
would be the wrapped one, as its name suggested. But I see now, by inspecting the neovide
executable, that it's just a script that execs .neovim-wrapped
. Hence, it is the wrapper for .neovide-wrapped
.
4a0226a
to
c095367
Compare
86bde81
to
ded9632
Compare
Description of changes
Produce .App for neovide on macOS environments.
Fixes: #301984
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.