search

zed-industries / zed

Code at the speed of thought – Zed is a high-performance, multiplayer code editor from the creators of Atom and Tree-sitter.
https://zed.dev
Other
35.32k stars 1.79k forks source link

Flatpak Build System & Support, v2 #11949

Closed someone13574 closed 2 weeks ago

someone13574 commented 2 weeks ago

This PR has been closed for the reasons stated here. Draft of new PR is here.

Old content:

ping https://github.com/zed-industries/zed/issues/6687

This PR is the successor PR to https://github.com/zed-industries/zed/pull/10754. It adds the bulk of what is needed for eventual Flatpak builds, including a Flatpak manifest, appstream metadata, and a process crate which wraps std::process::Command and smol::process::Command to easily allow the modification of all processes spawned by Zed to be run with flatpak-spawn, which is already available to all Flatpak's. This differs from the old PR which used a 3rd party program, host-spawn. Below is an image of how it appears in gnome-software as well as a list of todo's for this PR and future PR's.

Gnome Software:

TODO in this PR:

TODO in future PR's:

Release Notes:

ConradIrwin commented 2 weeks ago

Thanks for this!

I really like the idea of making Zed easy to install for everyone; but I am worried about causing users to have a worse experience actually using Zed and adding extra development/support burdens for us.

It looks like the flatpak approach doesn't work super well for editors or terminal editors right now (based on the limitations described at https://flathub.org/apps/com.visualstudio.code, and the decision of various terminal emulators in the space to avoid it https://github.com/kovidgoyal/kitty/issues/6965, https://github.com/alacritty/alacritty/issues/2571, etc.).

In particular I'm nervous about the sandbox (which as currently implemented requires rebuilding the app for flatpack, and remembering for every feature we add to spawn processes this way), and the disabled auto-updates (which I worry will lead to people stuck on older versions with known bugs - we ship multiple releases a week).

I wonder if we could use the way that Zed is architected to provide a simpler approach:

Could we:

someone13574 commented 2 weeks ago

Hi, thanks for the response.

It looks like the flatpak approach doesn't work super well for editors or terminal editors right now (based on the limitations described at https://flathub.org/apps/com.visualstudio.code, and the decision of various terminal emulators in the space to avoid it https://github.com/kovidgoyal/kitty/issues/6965, https://github.com/alacritty/alacritty/issues/2571, etc.).

This PR already adds all the changes needed for the terminal to work by instead spawning the process outside of the sandbox. It does break the sandbox, thus losing and security it would normally offer, but it seems to be the standard way to deal with it by say gnome-builder for example.

which as currently implemented requires rebuilding the app for flatpack

Yeah, this is very much a pain point. You can package binaries for flatpak iirc, but that would break all the supporting code gated using #[cfg(feature = flatpak)] which make the application usable. (there aren’t a ton of these flags, just the process wrapper and two in the terminal crate, but they are essential for good support).

remembering for every feature we add to spawn processes this way

I can write a basic CI check for any usage of Command or smol’s command that would check for any usage of these outside of a list of premitted files.

and the disabled auto-updates (which I worry will lead to people stuck on older versions with known bugs - we ship multiple releases a week).

It might be a bit unconventional but I don’t see why I couldn’t just run flatpak update using flatpak-spawn whenever the normal check would run wouldn’t work. It would still then allow flatpak’s normal update system to function but also allow in-app updates.

Do the flatpak detection at runtime, so that we can re-use the same binaries instead of burning CPU-time on having the rust compiler rebuild the app two ways.

That could totally work, I’d just need to replace the few instances of compile gating with a check for an environment variable which flatpak sets. The only downside would be app stores mistakenly showing the app as proprietary because a binary is being shipped, but I’m not fully sure whether other appstream info would stop that or not.

Build the flatpak bundle from assets included in the .tar.gz that we currently have (e.g. https://github.com/zed-industries/zed/releases/download/v0.136.2-pre/zed-linux-x86_64.tar.gz), or are more/different files needed?

The only new file we need is a appstream/metainfo file, plus some screenshots to display on store interfaces. Everything else I’m pulling from the existing crates/zed/resources directory which has stuff like icons and the desktop entry already made (which only need a minor adjustment to change the icon name to dev.zed.Zed).

Update the cli to spawn zed using flatpak-spawn outside of the sandbox, so that zed itself is able to access all files and run arbitrary processes (which although scary sounding, is actually what you need to write software productively).

We could but you would need to give up things such as desktop entries and being published on flathub. Additionally, this already has permission to access all files and is essentially already running arbitrary processes through flatpak-spawn. With a basic check for anything not using the process wrapper this could be pretty easily maintained (switching to the process wrapper is 2-3 lines of code).

but I am worried about causing users to have a worse experience actually using Zed and adding extra development/support burdens for us.

Totally fair, it’s ultimately up to you. Maybe there could be bundles that have a clear “experimental” warning and then a flatpak issue tag so that I can fix anything which pops up. This would let it stabilize for a while without the expectation of everything working perfectly (though at this point I’ve gotten it to a point where it works pretty much identically to Linux as a whole, but I still need to do some more dogfooding).

At this point I think I’ve added enough of the infrastructure needed that any flatpak-specific fixes would be pretty small and quick (there is only really two classes of problems: incorrect permissions which is a single line change or a process spawning inside of the sandbox, which is a few line change). Everything I’ve tested so far works identically to a normal build: the terminal, language servers, dev extensions, normal extensions, channels, sign-in, file access, settings, etc.

ConradIrwin commented 2 weeks ago

I think the approach of wrapping the process crate is not a good idea, it's a lot of code, and is a concern that every contributor needs to think about. 2-3 lines of code in one place is not a problem, but 2-3 lines of code everyone to remember is. Adding linters makes the whole thing more complicated and fragile, not less, and this is only covering process spawning – I don't want to have to maintain our flatpak sandbox configuration as we add more support for network-related stuff, debuggers, extensions, etc.

If we can find a way to isolate flatpak support to one or two places in the codebase, and ensure that 99% of zed is not running in the sandbox, it will make the whole thing more robust and easier to maintain.

I hadn't thought about the case where you launch it from the desktop environment. Currently the desktop files just run the zed process directly (and I imagine that flatpak would do the same). Maybe even easier than adding support to the CLI is to have the zed binary itself detect this situation and do the rust equivalent of exec flatpak-spawn --host "$0" "$@"?

Two things I want to clarify:

someone13574 commented 2 weeks ago

Ok, I think that this could work. Just to clarify, is this what you are proposing?

Closed since this approach doesn’t have much overlap with this PR outside of the metainfo file. I’ll start with a fresh branch.

clseibold commented 2 weeks ago

Wanted to note that flathub also wants compilation/building to be offline. You might be able to get away with using precompiled tarballs (I think some flatpaks do this), but I'm not sure. The other option is to store a vendor folder that contains all of the require packages in the repository.

someone13574 commented 2 weeks ago

Wanted to note that flathub also wants compilation/building to be offline. You might be able to get away with using precompiled tarballs (I think some flatpaks do this), but I'm not sure. The other option is to store a vendor folder that contains all of the require packages in the repository.

It already uses flatpak-cargo-generator and thus can be built offline.

ConradIrwin commented 2 weeks ago

Ok, I think that this could work. Just to clarify, is this what you are proposing?

  • Have the flatpak include both the cli and the app (either by rebuilding or straight copying the binary).
  • Only the cli is exposed in the flatpak. *The cli spawns the app binary on the host, which is bundled but not accessible externally (this still lets the install/uninstall be clean).
  • This means no changes need to be made to the app binary, only to the cli.
  • The desktop entry and meta info file could still exist, but the desktop entry spawns the cli instead.

Yes, I think that would work, and be a lot easier to maintain going forward. Thank you!

clseibold commented 2 weeks ago

It already uses flatpak-cargo-generator and thus can build offline.

Ah, right. I completely forgot that that was a thing.

someone13574 commented 2 weeks ago

Yes, I think that would work, and be a lot easier to maintain going forward. Thank you!

Sounds good. Since this approach is a bit simpler and a lot of the metainfo stuff is done already, I’ll try to get it done over the weekend (no promises though).