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

Add ability to use flatpak-spawn to call nvim when running from a flatpak container #2397

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sqwxl
Copy link

@sqwxl sqwxl commented Mar 4, 2024

Add ability to use flatpak-spawn to call nvim when running from a flatpak container.

I've also opened a pr for flathub that points to my own build for now: flathub/flathub#5017

What kind of change does this PR introduce?

  • Feature

Did this PR introduce a breaking change?

  • No

@fredizzimo
Copy link
Member

fredizzimo commented Mar 5, 2024

Thank you, this is great!

The changes themselves look good, but I'm holding off with merging, until it's clear that we get an exception to be allowed to launch nvim. Preferably, we should also be able to launch a custom binary through that, but I'm not sure if the potential exception will allow us to do that.

Do you need help with the application for it?

I also, saw that you need a better project description, I opened a discussion for that in our discord channel, so hopefully we will be able to provide you with something soon.

@sqwxl
Copy link
Author

sqwxl commented Mar 8, 2024

Hi @fredizzimo, thanks for your reply.

I'd love to contribute to making flatpak-ed neovide a reality. As you can see from the referenced Flathub PR, they are asking for lot's of changes. Most I can do myself, others will need Neovide maintainers. If you can help with the application to get approval for launching binaries on the host, that would be great.

I've been playing around with my own build for the past couple days and it's been pretty nice. Here are some of my observations:

  1. Neovide runs really well from within the flatpak container. It did lock up on me 2 or 3 times, but I haven't investigated why that happened and I don't know if it's a issue with "native" neovide as well.
  2. Allowing neovide to use flatpak-spawn to run the host's nvim binary is key. The alternative -- i.e. bundling nvim within the container -- is severely crippling and barely usable. In this scenario, not only is the sand-boxed nvim not configured like the host's, it also doesn't have access to any other binaries a user would expect (git, node, python, rg, etc).
  3. I couldn't copy and paste to and from the system clipboard. I'm sure there's a way to unlock this, probably at the application level, like with flatpak-spawn, but I haven't looked into it yet.

Going forward, perhaps it would make sense for me to open a parent issue for tracking the tasks involved in getting neovide published to Flathub. This issue would become one of its subtasks. What do you think? I'd be happy to get the ball rolling.

Edit: I've gone ahead and opened that issue (#2412) I propose we continue the discussion there.

@sqwxl sqwxl mentioned this pull request Mar 9, 2024
6 tasks
@sqwxl sqwxl marked this pull request as draft March 12, 2024 13:38
@sqwxl
Copy link
Author

sqwxl commented Mar 12, 2024

Update: Following some feedback from a Flathub maintainer, it was suggested that the flatpak Neovide should still work when the host doesn't have neovim installed (or it just isn't accessible from the container for some reason).
The expected behavior, imho, should be to fallback to a bundled version of neovim when the host isn't available. I'll implement the changes in this branch.

@fredizzimo
Copy link
Member

@sqwxl,

I will look at the other issues a bit later, but just quick comment for now

Notice that you will need some other dependencies as well, at least some of the provider ones:
https://neovim.io/doc/user/provider.html

Without at least the clipboard integration, copy and paste from the system clipboard won't work. I think that should be the bare minimum.

I think we also need to update the documentation, with info that running shell commands, the built-in terminal and LSP integration won't work without an externally installed nvim, since the bundled nvim won't know to use flatpak-spawn for those.

@sqwxl
Copy link
Author

sqwxl commented Mar 12, 2024

@fredizzimo I already added a clipboard provider in the latest manifest which you can try out by following by installing the test build: flatpak install --user https://dl.flathub.org/build-repo/89216/dev.neovide.Neovide.flatpakref

As for the other provider they seem to work out of the box (although that obviously won't be the case with the sandboxed neovim)
Screenshot from 2024-03-12 14-05-15

@Kethku
Copy link
Member

Kethku commented Mar 22, 2024

Update: Following some feedback from a Flathub maintainer, it was suggested that the flatpak Neovide should still work when the host doesn't have neovim installed (or it just isn't accessible from the container for some reason). The expected behavior, imho, should be to fallback to a bundled version of neovim when the host isn't available. I'll implement the changes in this branch.

Is a bundled copy of neovim correct here? That would be 120mb of data that for most uses doesn't actually get run and also includes many small files that can slow down the unpacking/unzipping steps that I presume flatpak uses. I don't personally use flatpak/flathub so I don't have a good sense for what the expected behavior would be. Just figured I'd ask

@fredizzimo
Copy link
Member

fredizzimo commented Mar 24, 2024

Update: Following some feedback from a Flathub maintainer, it was suggested that the flatpak Neovide should still work when the host doesn't have neovim installed (or it just isn't accessible from the container for some reason). The expected behavior, imho, should be to fallback to a bundled version of neovim when the host isn't available. I'll implement the changes in this branch.

Is a bundled copy of neovim correct here? That would be 120mb of data that for most uses doesn't actually get run and also includes many small files that can slow down the unpacking/unzipping steps that I presume flatpak uses. I don't personally use flatpak/flathub so I don't have a good sense for what the expected behavior would be. Just figured I'd ask

I agree, it does not look like they requested to embed Neovim, just to make it more clear in the description that it's required. Perhaps we could even improve our error message when running from Flatpaks?

I think we also need to be able to launch the flatpak version of Neovim if the terminal one is not available. And that should be preferred even if we embed it. Is that possible?

Edit:, I think it should try to launch the flatpak version even if the terminal one is available, but the flatpak version is newer. The use case for this is distribution where Neovim is installed but out of date, and no newer version is available. In those cases, most of these users will go to look for a flatpak version of Neovim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants