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 Package Build System & Support #10754

Closed
wants to merge 21 commits into from

Conversation

someone13574
Copy link
Contributor

@someone13574 someone13574 commented Apr 19, 2024

This PR adds support for building Flatpak packages, as well as the necessary metainfo/desktop entries needed by Zed and fixing the terminal to function within a Flatpak. For to moment I've forgone adding screenshots and release notes to the appstream manifest.

In order to fix the terminal, the Flatpak also compiles and bundles host-spawn which allows it to spawn the shell process outside of the sandbox. This is used over the normal flatpak-spawn due to problems which it has with running processes which require a tty/pty.

Useful Docs:

Screenshot:

Screenshot from 2024-04-24 22-53-02

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Apr 19, 2024
@someone13574 someone13574 changed the title Flatpak Package Building Flatpak Package Build System Apr 19, 2024
@someone13574 someone13574 marked this pull request as draft April 19, 2024 13:24
@someone13574 someone13574 marked this pull request as draft April 19, 2024 18:08
@someone13574
Copy link
Contributor Author

someone13574 commented Apr 19, 2024

Seems like the terminal is having problems with flatpaks sandboxing. Marking as a draft until I fix it.

Edit: It seems like Alacritty, which Zed currently uses for the built-in terminal, doesn't support and isn't planning on supporting running in Flatpak. See #10791

Edit 2: Nvm

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.
@someone13574 someone13574 marked this pull request as draft April 21, 2024 20:27
@someone13574
Copy link
Contributor Author

Seems like git-blame and dev-extension installation is also failing due to trying to spawn a process on the host. These will need to run through flatpak-spawn or host-spawn as well.

@Rotonen
Copy link

Rotonen commented Apr 22, 2024

VS Code had the same kind of flapping about when it started to get packaged and distributed as a Flatpak (things got fixed one thing at a time over versions, but also kept breaking when things changed). Disclaimer: I am only a user of VS Code.

I suggest to consider adding (at least down the line, if a regression actually arises) a separate smoketest build for the Flatpak package as that stuff is fragile and the whole landscape around it is moving.

@someone13574
Copy link
Contributor Author

VS Code had the same kind of flapping about when it started to get packaged and distributed as a Flatpak (things got fixed one thing at a time over versions, but also kept breaking when things changed). Disclaimer: I am only a user of VS Code.

I suggest to consider adding (at least down the line, if a regression actually arises) a separate smoketest build for the Flatpak package as that stuff is fragile and the whole landscape around it is moving.

The problems that would arise wouldn’t really be possible to test just with a build, as it isn’t the build failing but rather the editor trying to spawn a process at runtime, which it cannot locate because it’s in the sandbox. If a problem does occur, it will be runtime only and will likely be from a new feature being added which doesn’t call host-spawn. This means that you cannot really write a test for it since the feature doesn’t exist at the moment. As for test builds, that already happens as clippy is configured with the —all-features flag. This doesn’t actually build the flatpak, but it does compile the code used for the separate spawning logic. Adding a test which actually compiles in the flatpak environment would be good though.

@Rotonen
Copy link

Rotonen commented Apr 22, 2024

Please excuse the loose language. Build == GHA Workflow. Collides with static language build, thus causing the confusion above.

Exactly, as the problems are runtime specific, one would need a novel trivial end to end test run (== build == new check in the per commit / PR build status box) for the resulting Flatpak via flatpak run. Orchestrating that is probably non-trivial on a CI as one would need to figure out an UI testing framework which would work here in an xvfb context or somesuch. All of which is wildly out of scope here for this PR.

We do not ultimately disagree here. And we do not need to continue the discussion further at this point in time - I am already draining resources further than intended. Willing to volunteer down the line in case the direction becomes desirable: simply poke me with an @ on an issue or a PR and I'll figure signing the CLA out.

@mikayla-maki
Copy link
Contributor

mikayla-maki commented Apr 22, 2024

@someone13574 Since we're going to need to customize the commands for flatpak in many different locations, what do you think about building a little zed-process library that wraps a Command call and contains the flatpak #[cfg] calls? Then we can route these other uses (git, LSPs, etc.) through that interface.

@someone13574
Copy link
Contributor Author

@someone13574 Since we're going to need to customize the commands for flatpak in many different locations, what do you think about building a little zed-process library that wraps a Command call and contains the flatpak #[cfg] calls? Then we can route these other uses (git, LSPs, etc.) through that interface.

Good idea. Might also be useful for allowing zed to work in the web later on.

[
{
"type": "archive",
"url": "https://github.com/someone13574/host-spawn/archive/refs/tags/temp.zip",
Copy link
Contributor Author

@someone13574 someone13574 Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporary: DO NOT MERGE.

This should wait until 1player/host-spawn#36 is merged upstream. This adds working directory support.

@someone13574 someone13574 marked this pull request as ready for review April 25, 2024 02:31
@someone13574 someone13574 changed the title Flatpak Package Build System Flatpak Package Build System & Support Apr 25, 2024
@someone13574 someone13574 force-pushed the flatpak branch 3 times, most recently from 9e9ea05 to 3a49ce0 Compare April 25, 2024 21:46
@mikayla-maki
Copy link
Contributor

@someone13574 Since this PR has a comment saying 'do not merge', I'm going to reset it to a draft. When you're ready to merge let's come back to it :D

@mikayla-maki mikayla-maki marked this pull request as draft April 26, 2024 20:40
@someone13574
Copy link
Contributor Author

@someone13574 Since this PR has a comment saying 'do not merge', I'm going to reset it to a draft. When you're ready to merge let's come back to it :D

Sounds good. I was just thinking that merging while bundling a fork owned by me wouldn't be the best idea, even if the the only change from upstream is pretty trivial. The host-spawn maintainer was open to the idea, so once the PR gets merged upstream I'll switch over to that and then mark as ready. I don't foresee any other major changes at this moment other using upstream for host-spawn.

@mikayla-maki
Copy link
Contributor

Sounds good, I'll give this a code review at some point :)

@mikayla-maki
Copy link
Contributor

mikayla-maki commented Apr 26, 2024

Thank you for working on this!

I took a look through this PR and I don't think we're interested in supporting flatpak until the official flatpak-spawn command is able to support our terminal natively. I'd rather not depend on a smaller project at such a critical part of the interface. It seems there's an alternative solution, involving smarter integration with the process spawned, that we'd be more interested in.

Either way though, I'm going to close this PR for now so we can focus on stabilizing Linux builds. We'll be releasing .deb builds at some point soon, if it's not possible to wrap those builds for flatpak somehow, please file an issue and we can re-visit this once our Linux builds are more mature :)

@someone13574
Copy link
Contributor Author

someone13574 commented Apr 27, 2024

Thank you for working on this!

I took a look through this PR and I don't think we're interested in supporting flatpak until the official flatpak-spawn command is able to support our terminal natively. I'd rather not depend on a smaller project at such a critical part of the interface. It seems there's an alternative solution, involving smarter integration with the process spawned, that we'd be more interested in.

Either way though, I'm going to close this PR for now so we can focus on stabilizing Linux builds. We'll be releasing .deb builds at some point soon, if it's not possible to wrap those builds for flatpak somehow, please file an issue and we can re-visit this once our Linux builds are more mature :)

I believe that the openpty command currently used by alacritty_terminal already sets O_NOCTTY. The real problem is that one needs to use the --forward-fd argument of flatpak-spawn, but since the pty is opened from inside alacritty, you don't have access to what file descriptors id's will be allocated, as they haven't been opened by alacritty until after you construct the command. Host-spawn was getting around this by just opening their own, second pty, but I agree that relying on a small project like that isn't a good solution.

As I currently see it, there are really only two ways which flatpak support could work in the future, if it is wanted. First would be building a small rust binary target into Zed which does what host-spawn does, calling the underlying org.freedesktop.Flatpak.Development dbus interface and allocating a second pty (which means we can open & pass our own set of file-descriptors). The only difference from host-spawn would be that the code would then be hosted in the Zed repository instead of being external, and it would be written in rust like the rest of the project. The second way would be dropping alacritty partially or fully, which isn't really feasible without tons of effort due to the extent of it within Zed's terminal.

As for wrapping .deb builds for flatpak, that would only encounter the same problem of any processes designed to be run on the host being run in the sandbox.

@someone13574 someone13574 deleted the flatpak branch April 27, 2024 16:05
@someone13574
Copy link
Contributor Author

I’ve gotten a change merged in alacritty which allows us to open the pty ourselves instead of alacritty doing it, which means the file descriptor argument of flatpak-spawn can be used instead of relying on host-spawn. This should allow host-spawn to be dropped entirely if flatpak support is perused in the future.

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

4 participants