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

Flatpak Build System & Support, v2 #11949

Closed
wants to merge 29 commits into from

Conversation

someone13574
Copy link
Contributor

@someone13574 someone13574 commented May 17, 2024

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

Old content:

ping #6687

This PR is the successor PR to #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:

  • Fix panic when closing window (something isn't exiting properly).
  • Fix terminal process names (they are currently including a ton of --env parameters from flatpak-spawn, should only show bash, zsh, etc.)
  • Fix warning when running tasks
  • Make CLI work
  • Disable auto-update Make auto-update use flatpak update.
  • Add CI for finding use of std::process::Command and smol::process::Command so that they aren’t accidentally used. Have ignore file for the process wrapper and any other places where the wrapper isn’t needed (eg. Mac only, build.rs, etc)
  • (maybe) Switch from compile flags to detection of flatpak at runtime and use pre-built binaries instead of re-building (flathub might not like this).

TODO in future PR's:

  • Flatpak-bundle action
  • Flatpak CI (?)
  • Semi-automate release note generation (flathub is very particular about release notes; just copying the main release notes won't work and they must be trimmed down).
  • Get somebody more design-inclined to review dev.zed.Zed.metainfo.xml's content, description, branding colors, and screenshots.
  • Release (?) preview clearly labelled experimental builds as Flatpak bundles for feedback and stabilization before moving forward with anything more official. Flatpak issue tag.

Release Notes:

  • N/A

someone13574 and others added 26 commits April 19, 2024 09:48
Fixes the flatpak terminal by prefixing the shell command with [host-spawn](https://github.com/1player/host-spawn). This runs the shell on the host-system, giving access to the programs which a user would expect. Note that this breaks through the sandbox. host-spawn is used over flatpak-spawn, the command built into flatpaks, because it allocates a pty for the spawned process, fixing issues with sudo and other commands.
This avoids recompiling it when zed needs to recompile
This fixes an issue where the checkout would fail due to it checking out to a point where one of the files no longer existed in tree-sitter-cpp
process and terminal need to be a dependency of zed even though they are unused because the flatpak compile flag must be passed workspace wide.
@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label May 17, 2024
@someone13574 someone13574 changed the title Flatpak Flatpak Build System & Support, v2 May 17, 2024
@ConradIrwin
Copy link
Collaborator

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 kovidgoyal/kitty#6965, alacritty/alacritty#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:

  • Today we ship two binaries: cli (which confusingly is what you hit when you type zed in a terminal) and zed (which is the actual editor).
  • The cli is responsible for managing the zed process - if it's not running it spawns it in the background, and if it is it sends it commands to open new files/directories etc over an abstract domain socket.

Could we:

  • 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).
  • 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.
  • 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?

@someone13574
Copy link
Contributor Author

someone13574 commented May 17, 2024

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 kovidgoyal/kitty#6965, alacritty/alacritty#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
Copy link
Collaborator

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:

  • "give up things such as desktop entries" - I think (though this is maybe distro specific) that the existing desktop files in the tarball work if you drop them in ~/.local/share/applications. Is this not true on Arch?
  • How does running a flatpak app from the terminal work? Would we be able to use our current approach of symlinking the cli binary into your path so that you can zed README.md from a terminal? (I am not familiar with how the sandbox works under the hood, but it seems like if that was possible it'd be a big problem, but I don't know how you'd stop people doing that).

@ConradIrwin ConradIrwin mentioned this pull request May 17, 2024
9 tasks
@someone13574
Copy link
Contributor Author

someone13574 commented May 17, 2024

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.

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
Copy link

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 someone13574 deleted the flatpak branch May 17, 2024 17:35
@someone13574
Copy link
Contributor Author

someone13574 commented May 17, 2024

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
Copy link
Collaborator

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
Copy link

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

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

@someone13574
Copy link
Contributor Author

someone13574 commented May 17, 2024

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).

ConradIrwin pushed a commit that referenced this pull request May 28, 2024
ping #6687 

This is the third iteration of this PR ([v2
here](#11949)) and uses a
different approach to the first two (the process wrapper lib was a
maintainability nightmare). While the first two attempted to spawn the
necessary processes using flatpak-spawn and host-spawn from the app
inside the sandbox, this version first spawns the cli binary which then
restart's itself *outside* of the sandbox using flatpak-spawn. The
restarted cli process than can call the bundled app binary normally,
with no need for flatpak-spawn because it is already outside of the
sandbox. This is done instead of keeping the cli in the sandbox because
ipc becomes very difficult and broken when trying to do it across the
sandbox.

Gnome software (example using nightly channel and release notes
generated using the script):
<img
src="https://github.com/zed-industries/zed/assets/81528246/6391d217-0f44-4638-9569-88c46e5fc4ba"
width="600"/>

TODO in this PR:
- [x] Bundle libs.
- [x] Cleanup release note converter.

Future work:

- [ ] Auto-update dialog
- [ ] Flatpak auto-update (complete 'Auto-update dialog' first)
- [ ] Experimental
[bundle](https://docs.flatpak.org/en/latest/single-file-bundles.html)
releases for feedback (?).

*(?) = Maybe / Request for feedback*

Release Notes:

- N/A

---------

Co-authored-by: Marshall Bowers <[email protected]>
Co-authored-by: Mikayla Maki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants