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

AppImage #2089

Merged
merged 4 commits into from
Apr 20, 2022
Merged

AppImage #2089

merged 4 commits into from
Apr 20, 2022

Conversation

mtoohey31
Copy link
Contributor

@mtoohey31 mtoohey31 commented Apr 12, 2022

Closes #1073.

Depends on #283.

So far this PR only contains a simple script for generating the AppImage, based on neovim's genappimage script. Since AppImages seem to require a desktop file and a logo file, #283 needs to be handled first. Also, as pickfire mentioned here, once we've added the desktop file we'll need to let package maintainers know.

Questions

  • I've made sure that the runtime folder is being detected (tree-sitter grammars work properly), is there anything else that might be problematic with this packaging format that I should check?
  • How should we work this into the release process? Should I add the commands in the script as a step in the publish job of the release.yml action file, or is there somewhere else I should put them?

TODO

  • /lib64/ld-linux-x86-64.so.2 seems to be hardcoded into the hx binary in the resulting AppImage. Like I mentioned in Flatpak package #2076 I'm new to AppImage so I'm not sure if this is incorrect, but it might be. This site seems to indicate that /lib64/ld-linux-x86-64.so.2 is required for AppImages anyways, so I believe this is fine.

@the-mikedavis
Copy link
Member

I've made sure that the runtime folder is being detected (tree-sitter grammars work properly), is there anything else that might be problematic with this packaging format that I should check?

The setup of the HELIX_RUNTIME var looks good, I don't think there should be any other pitfalls 👍

How should we work this into the release process? Should I add the commands in the script as a step in the publish job of the release.yml action file, or is there somewhere else I should put them?

Yep, that sounds perfect 👍

@mtoohey31 mtoohey31 marked this pull request as ready for review April 14, 2022 15:54
@mtoohey31
Copy link
Contributor Author

Ok, after the latest few commits I believe everything is ready to go. The workflow runs as expected (see the logs here), and updates with appimageupdatetool also work. With the current master, the size of the AppImage is 12.5 MB, which is a bit larger than the releases for linux and macos (which makes sense because more things will be bundled into the binary), but it's still smaller than the windows release.

Is it ok to merge this with the placeholder logo then update it once we've settled on an official logo, or do we want to decide on a logo before merging?

@the-mikedavis
Copy link
Member

I'm ok with the purple dot icon for now - it seems like a good simple placeholder. What do others think?

Comment on lines +189 to +190
mv "$APP-$VERSION-$ARCH.AppImage" \
"$APP-$VERSION-$ARCH.AppImage.zsync" dist
Copy link
Member

Choose a reason for hiding this comment

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

This should build a separate archive. I don't want to bundle it together with the binaries because it will bloat the archive size for users that don't want the AppImage.

Copy link
Member

Choose a reason for hiding this comment

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

Might make sense to do this as a separate workflow altogether?

Copy link
Contributor Author

@mtoohey31 mtoohey31 Apr 15, 2022

Choose a reason for hiding this comment

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

Do you mean the workflow artifacts, or the release assets? I'm pretty sure none of the other release assets contain the AppImage, just the AppImage itself (see my test release here vs the sizes of the latest official release), though the AppImage does get bundled into the linux release workflow artifacts if that's a problem.

Copy link
Member

Choose a reason for hiding this comment

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

though the AppImage does get bundled into the linux release artifacts if that's a problem.

Yes, that's what I meant. Various distributions simply repackage the linux release, bundling AppImage into the same archive will increase the download size. The AppImage is a separate package/distribution format and it should be packaged into it's own archive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah shoot, I meant to say workflow artifacts. Just to make sure we're on the same page, the AppImage gets bundled into the linux artifacts saved here in the workflow run, but not here on the release page. Are distributions using the workflow artifacts instead of the release artifacts?

Copy link
Member

@the-mikedavis the-mikedavis Apr 15, 2022

Choose a reason for hiding this comment

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

Oh I see. I think this is currently fine: the AppImage is built in the build step for the x86_64 linux image and it also ends up in the workflow artifacts but then it ends up as a separate release artifact. (see here)

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

Based on the runs in your fork this looks good 👍

Thanks for working on this!

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Thanks!

@archseer archseer merged commit e452b97 into helix-editor:master Apr 20, 2022
@mtoohey31 mtoohey31 deleted the appimage branch April 20, 2022 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support desktop entry / Add application entry
3 participants