Added snap packaging #3931

kenvandine · 2024-12-29T19:29:36Z

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.

tristan957

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

tristan957 · 2024-12-29T20:19:09Z

snap/snapcraft.yaml

+      case "$CRAFT_TARGET_ARCH" in
+        amd64)  arch=x86_64    ;;
+        arm64)  arch=aarch64  ;;
+        *)      arch=""     ;;


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.

tristan957 · 2024-12-29T20:19:36Z

snap/snapcraft.yaml

+      - libgtk-4-dev
+      - libadwaita-1-dev
+      - git
+    stage-packages:


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

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

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

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

kenvandine · 2024-12-29T21:14:34Z

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

.github/workflows/snap.yaml

tristan957 · 2024-12-29T22:14:29Z

.github/workflows/snap.yaml

+
+jobs:
+  build:
+    runs-on: ubuntu-24.04


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

tristan957 · 2024-12-29T22:14:56Z

snap/local/launcher

+#!/usr/bin/env bash
+
+export XDG_CONFIG_HOME="$SNAP_REAL_HOME/.config"
+export XDG_DATA_HOME="$SNAP_REAL_HOME/.local/share"
+
+exec "$@"


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

kenvandine · 2024-12-30T19:42:51Z

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

snap/snapcraft.yaml

mitchellh

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 · 2025-01-02T22:30:23Z

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 · 2025-01-02T22:31:39Z

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 · 2025-01-02T22:56:15Z

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 · 2025-01-02T22:57:55Z

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 · 2025-01-02T22:59:18Z

Want me to go ahead and start the dispute?

kenvandine · 2025-01-03T15:54:57Z

I created a name dispute request in the store.

mitchellh · 2025-01-03T16:31:47Z

I created a name dispute request in the store.

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

kenvandine · 2025-01-03T17:44:36Z

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

kenvandine · 2025-01-07T21:00:49Z

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

mitchellh · 2025-01-08T18:41:08Z

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 · 2025-01-08T20:10:45Z

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

tristan957 · 2024-12-30T21:54:58Z

snap/local/launcher

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


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

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

tristan957 · 2024-12-30T21:57:02Z

snap/snapcraft.yaml

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


What about all the other completion scripts for other shells

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

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

dobicinaitis · 2025-01-09T09:32:41Z

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 · 2025-01-09T15:20:13Z

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

kenvandine · 2025-01-09T22:24:31Z

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 · 2025-01-12T02:45:36Z

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 · 2025-01-12T04:05:46Z

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

mitchellh · 2025-01-12T20:29:15Z

@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 · 2025-01-12T22:02:47Z

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

dependencies from the host

reliable across more distros as a classic snap.

distros and unset all SNAP environment variables that could leak at runtime

mitchellh · 2025-01-24T00:30:20Z

Alright! Our CI seems to be hung up on this issue that our CI provider found: canonical/craft-providers#719 I'm not sure if there is a workaround we can apply to that yet but that's the major issue right now...

kenvandine · 2025-01-24T12:00:12Z

This is my first time touching zig, check out my changes to src/termio/Exec.zig which should help ensure the snap isn't leaking environment variables that were necessary to launch ghostty but shouldn't be set in there resulting terminal shell.

tristan957 · 2025-01-24T18:27:40Z

src/termio/Exec.zig

+            env.remove("DRIRC_CONFIGDIR");
+            env.remove("__EGL_EXTERNAL_PLATFORM_CONFIG_DIRS");
+            env.remove("__EGL_VENDOR_LIBRARY_DIRS");
+            env.remove("LD_LIBRARY_PATH");
+            env.remove("LIBGL_DRIVERS_PATH");
+            env.remove("LIBVA_DRIVERS_PATH");
+            env.remove("VK_LAYER_PATH");
+            env.remove("XLOCALEDIR");


This would unconditionally remove these environment variables from the shell. Reading your launcher file above, you append the various snap paths if the environment variable exists. Wouldn't we just want to remove the snap paths from the environment variables if the user already had them set?

No, the problem is the resulting shell in ghostty uses these library paths, drivers, etc for any processes started from ghostty which won't work mixing libs from in the container with host libs. This effectively drops everything needed to start ghostty without leaking those paths into the resulting shell. If a user is setting any of them in /etc or .bashrc, etc those will be honored by the shell anyway.

jcollie · 2025-01-24T19:20:57Z

src/termio/Exec.zig

@@ -732,6 +732,19 @@ const Subprocess = struct {
            try env.put("GHOSTTY_RESOURCES_DIR", dir);
        }

+        // Unset environment varies set by the snap
+        if (env.get("SNAP") != null) {


The "Zig" way to do this is:

if (env.get("SNAP")) |_| {

…ck for environment variable.

tristan957 reviewed Dec 29, 2024

View reviewed changes

kenvandine force-pushed the add_snap_package branch from 8cc94d5 to c5096fb Compare December 29, 2024 21:09

kenvandine force-pushed the add_snap_package branch from 039e0fb to 8d06e3c Compare December 29, 2024 22:12

tristan957 reviewed Dec 29, 2024

View reviewed changes

kenvandine force-pushed the add_snap_package branch from 1aac9c2 to f166cd7 Compare December 30, 2024 20:29

matifali reviewed Dec 31, 2024

View reviewed changes

snap/snapcraft.yaml Show resolved Hide resolved

mitchellh requested changes Jan 2, 2025

View reviewed changes

mitchellh force-pushed the add_snap_package branch from f166cd7 to 4fadb87 Compare January 8, 2025 18:39

tristan957 reviewed Jan 8, 2025

View reviewed changes

mitchellh force-pushed the add_snap_package branch from d98a199 to c789c18 Compare January 12, 2025 20:28

kenvandine and others added 22 commits January 23, 2025 16:26

Only run snap workflow on push and PR

aa50e85

Changed shebang in launcher script

c73ccb5

Set a more meaningful version for the snap

cbf90b3

Add snap to nix, add arm64 builders

d4a8f7b

Ensure LD_LIBRARY_PATH is set appropriately

6e36c20

CRAFT_TARGET_ARCH is deprecated, use CRAFT_ARCH_BUILD_FOR

19f2102

EGL fixes, ensure necessary env variables are set to isolate

6eef3fb

dependencies from the host

Stage more depends to ensure we aren't getting leaks from the host

3c28f58

Enable patch-elf for libs part

232696a

Drop patchelf

409779b

Stage gnome-text-editor to open configuration, this makes it more

1b81b3d

reliable across more distros as a classic snap.

no-patchelf for DRI and tidy up the mesa bits

bed1d08

Exit with error if building for unsupported arch

cf7668d

Set GHOSTTY_RESOURCES_DIR

fe32aeb

enable-patchelf is more repliable for classic snaps

f62bbf4

Don't stage shells

c825c1a

Removed duplicated stage-packages

c391823

ci: temporary apt installs required for namespace

d074b8d

Use patch-rpath which improves our cross distro support

8b235f1

Improved environment handling to ensure the snap will work across

55a1b91

distros and unset all SNAP environment variables that could leak at runtime

Stage libglib2.0-0t64 to insure we don't mix in the host's lib

239b8f3

Only export XDG_CONFIG_HOME and XDG_DATA_HOME if they aren't already set

720dca4

mitchellh force-pushed the add_snap_package branch from abc18d5 to 720dca4 Compare January 24, 2025 00:26

kenvandine added 2 commits January 24, 2025 13:58

Unset environment varies set by the snap

dd62471

Clean up environment variable while launching the shell

8be4c6d

tristan957 reviewed Jan 24, 2025

View reviewed changes

jcollie reviewed Jan 24, 2025

View reviewed changes

Per PR review feedback, this is the more "ziggy" way of doing the che…

4da51fa

…ck for environment variable.

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

Added snap packaging #3931

Are you sure you want to change the base?

Added snap packaging #3931

Conversation

kenvandine commented Dec 29, 2024

tristan957 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenvandine commented Dec 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenvandine commented Dec 30, 2024

mitchellh left a comment

Choose a reason for hiding this comment

tristan957 commented Jan 2, 2025

mitchellh commented Jan 2, 2025

kenvandine commented Jan 2, 2025

mitchellh commented Jan 2, 2025

kenvandine commented Jan 2, 2025

kenvandine commented Jan 3, 2025

mitchellh commented Jan 3, 2025

kenvandine commented Jan 3, 2025

kenvandine commented Jan 7, 2025

mitchellh commented Jan 8, 2025

mitchellh commented Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

tristan957 Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dobicinaitis commented Jan 9, 2025

mitchellh commented Jan 9, 2025

kenvandine commented Jan 9, 2025

kenvandine commented Jan 12, 2025

mitchellh commented Jan 12, 2025

mitchellh commented Jan 12, 2025

kenvandine commented Jan 12, 2025

mitchellh commented Jan 24, 2025

kenvandine commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchellh commented Jan 8, 2025 •

edited

Loading

tristan957 Jan 21, 2025 •

edited

Loading