-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add snap package #16957
base: main
Are you sure you want to change the base?
Add snap package #16957
Conversation
We require contributors to sign our Contributor License Agreement, and we don't have @soumyaDghosh on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
@soumyaDghosh Thanks for kicking this off, I've gone ahead and submitted a request for the name Running Looking into the Snap build processes, I don't think we'll be able to use the default "build from GitHub" because we push releases to their own branches; so we'll need to figure out how to build the snap appropriately. Given that, instead of having the snap metadata be static, it should be a template that can be prefilled (similar to how the flatpak support works). I'm not sure the best place to put the automation to kick off a new snap release, we can probably add it to the CI workflow that publishes the Github releases; though currently they get a manual review before they're "shipped", so it might be better to hook into when the releases are published. I'm also happy if you want to own that piece of the puzzle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this upstream. I'm not a zed maintainer, just a causual user and a small contributor. I left a few comments linline for your consideration.
@@ -0,0 +1,17 @@ | |||
[Desktop Entry] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you don't need top copy the desktop file. See my comment on the snapcraft forum: https://forum.snapcraft.io/t/zed-rust-based-ide/40993/8
The main idea is to do this:
override-stage: |
sed -i -e 's,Icon=zed,Icon=${SNAP}/share/icons/hicolor/1024x1024/apps/zed.png,' "$CRAFT_PART_INSTALL"/share/applications/zed.desktop
craftctl default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is when using rust plugin, the desktop file will not be in the final set files. Also, the file has placeholders. This gets rid of all those complications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should ship multiple copies of the same stuff in the repo. I think right way to do this is to wrap the snap invocation in a shell-script that produces the input files it expects from the same files we use to build the other packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you kindly share some example with us? What do you mean by wrap the snap invocation in a shell-script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/zed-industries/zed/blob/main/script/flatpak/bundle-flatpak is how this is done for flatpak for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be run by a CI?
name: zed-editor | ||
title: Zed | ||
base: core24 | ||
version: '0.149.5' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should adopt-info: zed
and the version should be set in one of the override steps with craftctl set version=...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Their appstream metainfo file will again need to be staged into the snap file. And that too is also filled with placeholders.
title: Zed | ||
base: core24 | ||
version: '0.149.5' | ||
summary: Code at the speed of thought |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be imported from the appstream meta-data so to avoid having to maintain a special copy. Having said that I have not done this in a long while so I don't recall the syntax from the top of my head.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. We can adopt a lot more fields from appstream metadata as I implemented those in snapcraft. But that's will just complicated things even more.
I heard back and was denied for |
This commit adds the snap packaging reciepe to the repo, which can served to the store directly from this repo.
I have kept the snap name |
I have been granted the If not, we can likely figure something out (but it may be slower :D). |
How would you like me to take over the maintainership? I'd like to that the name remains under Zed Industries in the store and may be just add me as a collaborator. |
@notpeter's going to take over on this. Sorry for the glacial speed this is moving, I want to make sure we're doing the right thing, |
confinement: classic | ||
icon: crates/zed/resources/app-icon.png | ||
website: https://zed.dev/ | ||
souce-code: https://github.com/zed-industries/zed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a typo and it should be source-code instead of souce-code
Edit: decided it's confusing to discuss a different branch on this PR, so moved this comment here |
Closes #9922
Release Notes:
EDIT: I forgot to add the process of regsitering and uploading the snap
If the team wants, they can add me as a collaborator for the snap and I can help you with all these processes.