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

Added snap packaging #3931

Merged
merged 88 commits into from
Feb 15, 2025
Merged

Added snap packaging #3931

merged 88 commits into from
Feb 15, 2025

Conversation

kenvandine
Copy link
Contributor

Added snap packaging, fixes #3153

If whoever registered the name in the snap store could add me as a collaborator, I can handle getting it released in the store, setup automated builds, and request the necessary classic permissions in the store.

Copy link
Collaborator

@tristan957 tristan957 left a comment

Choose a reason for hiding this comment

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

How do all of the shell completion files and vim/neovim files get exported? I don't see any logic related to them here.

I'd like to see a GitHub Actions workflow added, so this doesn't regress :).

case "$CRAFT_TARGET_ARCH" in
amd64) arch=x86_64 ;;
arm64) arch=aarch64 ;;
*) arch="" ;;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Presumably it would be good to exit non-zero in this case with a good error message before curl tries to download something that doesn't exist.

- libgtk-4-dev
- libadwaita-1-dev
- git
stage-packages:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not too familiar with Snap. What are stage packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stage-packages are ubuntu packages that satisfy runtime dependencies which get bundle in the snap.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, why would we want to bundle shells? Do classic snaps not have access to the host binaries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does have access to anything on the host, so these could be removed.

@kenvandine
Copy link
Contributor Author

How do all of the shell completion files and vim/neovim files get exported? I don't see any logic related to them here.

I'd like to see a GitHub Actions workflow added, so this doesn't regress :).

I've added a workflow to ensure the snap builds. I'll work on the shell completion. I hadn't done anything for that, but will


jobs:
build:
runs-on: ubuntu-24.04
Copy link
Collaborator

Choose a reason for hiding this comment

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

We will probably want to use one of the project's runners. I'll let @mitchellh give a better review here.

Comment on lines 1 to 45
#!/usr/bin/env bash

export XDG_CONFIG_HOME="$SNAP_REAL_HOME/.config"
export XDG_DATA_HOME="$SNAP_REAL_HOME/.local/share"

exec "$@"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like we could just use /bin/sh for the shebang.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@kenvandine
Copy link
Contributor Author

@tristan957 I think I've responded to all the feedback so far.

Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

I'm still not 100% sure I want to maintain a snap but it does seem to be more generally accepted. I'm going to think about that more.

My main request here is does snap/ have to be a top-level folder? I think it makes more sense to put into dist/

@tristan957
Copy link
Collaborator

I think there is slight value in maintaining a flatpak and snap. They are "universal" formats that can run on any (if snap is supported) distro. It also gives us the ability to have an official Linux distribution channel(s) that we can recommend for people in the event downstreams can't properly package Ghostty for users.

Of note, the author of this PR works at Canonical, so perhaps they could be interested in helping to maintain the Snap into the future :)

@mitchellh
Copy link
Contributor

Yeah. I'm leaning towards it. 😄

@kenvandine if you'd be happy to hang around and help out I'd absolutely love that too.

@kenvandine
Copy link
Contributor Author

snapcraft looks for the packaging in a few places. We'd have to jump through some hoops if we wanted it under dist. We could do it, but I would like to keep things as simple as possible for automated builds.

I am happy to help look after the snap! I maintain quite a few already and I plan to be a daily user of ghostty too.

Who registered the name in the snap store?

@mitchellh
Copy link
Contributor

snapcraft looks for the packaging in a few places. We'd have to jump through some hoops if we wanted it under dist. We could do it, but I would like to keep things as simple as possible for automated builds.

Understood. We can keep it where it is.

Who registered the name in the snap store?

Not me. We may have to register a dispute.

@kenvandine
Copy link
Contributor Author

Want me to go ahead and start the dispute?

@kenvandine
Copy link
Contributor Author

I created a name dispute request in the store.

@mitchellh
Copy link
Contributor

I created a name dispute request in the store.

Appreciate it. After the dispute, do I need to transfer it to my account?

@kenvandine
Copy link
Contributor Author

Yeah, once we get everything in place we'll get it transferred.

@kenvandine
Copy link
Contributor Author

I've requested permission for classic confinement at https://forum.snapcraft.io/t/request-classic-confinement-for-ghostty/44523

@mitchellh
Copy link
Contributor

I updated the builders to use Namespace, but it looks like there are issues there. I'll reach out to support for that.

I tested this locally and works great, but will be important for CI to be able to run this. 😄

@mitchellh
Copy link
Contributor

mitchellh commented Jan 8, 2025

FYI Namespace has a fix in the pipeline so we'll probably have CI working soon. Probably within a week.

Comment on lines 3 to 5
export XDG_CONFIG_HOME="$SNAP_REAL_HOME/.config"
export XDG_DATA_HOME="$SNAP_REAL_HOME/.local/share"
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is this supposed to work if XDG_CONFIG_HOME and XDG_DATA_HOME are defined to be elsewhere for a user?

Copy link
Collaborator

@tristan957 tristan957 Jan 21, 2025

Choose a reason for hiding this comment

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

I still have a concern here in relation to users not using these paths for XDG_CONFIG_HOME or XDG_DATA_HOME. Like I said, could be me not understanding snap

ghostty:
command: bin/ghostty
command-chain: [ bin/launcher ]
completer: share/bash-completion/completions/ghostty.bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about all the other completion scripts for other shells

Copy link
Collaborator

Choose a reason for hiding this comment

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

to be fair, I don't much about snap, so take this comment with a grain of salt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the snap completer logic only knows bash for now.

@dobicinaitis
Copy link

Hi! I was about to request classic confinement for my own Ghostty snap when I noticed the request to approve the upstream one. Awesome! I'll stop working on my implementation, but you might find something useful in the GitHub Actions workflows I set up (link). They were designed to periodically check for new Ghostty releases and create snaps for both the stable and tip channels. However, all the version change detection would be redundant for workflows in this repository.

@mitchellh
Copy link
Contributor

Your Git history here is getting messed up, btw. You probably see it already but just in case you don't...

@kenvandine
Copy link
Contributor Author

Your Git history here is getting messed up, btw. You probably see it already but just in case you don't...

Ugh, thanks I hadn't noticed. I've been rebasing while I iterate. Currently working to make sure the classic snap works well on non-ubuntu distros, which can be tedious. I'll get the history cleaned up soon.

@kenvandine
Copy link
Contributor Author

Your Git history here is getting messed up, btw. You probably see it already but just in case you don't...

I'm struggling to see why the history is whacky. I've been rebasing my branch from main regularly and the number of files showing up in the PR as changed is growing. Since my rebases have been from main, those changes shouldn't show in my diff. I've never seen this happen before. Any pointers?

@mitchellh
Copy link
Contributor

I'll take a look and try to force push a fix onto the branch tomorrow if I can 😄

@mitchellh
Copy link
Contributor

@kenvandine I cleaned up the git history. I'm not sure if I did a couple conflicts correctly so please review the files... but the conflicts were very small so I think they'd be minor.

@kenvandine
Copy link
Contributor Author

@kenvandine I cleaned up the git history. I'm not sure if I did a couple conflicts correctly so please review the files... but the conflicts were very small so I think they'd be minor.

Thanks, that looks much better. There was one small change missing, I just pushed that as a new commit. I'll avoid rebasing for now to prevent that weirdness again.

@mitchellh mitchellh requested a review from a team as a code owner February 15, 2025 15:22
@mitchellh
Copy link
Contributor

Tests are green! I moved the env var setting from termio to the apprt since that's where they go now.

One more green run and I'll merge this then we can consider release management and other things in future work.

Thank you @kenvandine

@kenvandine
Copy link
Contributor Author

Tests are green! I moved the env var setting from termio to the apprt since that's where they go now.

One more green run and I'll merge this then we can consider release management and other things in future work.

Thank you @kenvandine

That's awesome! I think the snap is in pretty good shape, I've tested on several Ubuntu versions, Debian 11 and 12, and fedora 41 myself.

@mitchellh mitchellh merged commit aeccbd2 into ghostty-org:main Feb 15, 2025
28 checks passed
@github-actions github-actions bot added this to the 1.2.0 milestone Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants