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

Build AppImage #8329

Closed
wants to merge 5 commits into from
Closed

Build AppImage #8329

wants to merge 5 commits into from

Conversation

azubieta
Copy link

@azubieta azubieta commented Oct 16, 2020

This PR adds a Github workflow to pack OpenTDD using the AppImage format from commits and PR to master.
The resulting binary is published as an artifact.

appimage-builder is ran twice in order to patch the timidity configuration to use freepaths. This could be fixed once the feature gets added into appimage-builder AppImageCrafters/appimage-builder#50

Closes #8019

@azubieta azubieta force-pushed the master branch 3 times, most recently from b56bdbc to 7392a80 Compare October 16, 2020 18:57
.github/workflows/build-appimage.yml Outdated Show resolved Hide resolved
.github/workflows/build-appimage.yml Outdated Show resolved Hide resolved
@azubieta azubieta force-pushed the master branch 2 times, most recently from a37f55e to ec74ed2 Compare October 16, 2020 19:40
@TrueBrain
Copy link
Member

TrueBrain commented Oct 16, 2020

As IRC doesn't stick, let me write it down here too:

  • I wouldn't publish an Artifacts on pull-request; this allows for people to mislead others in downloading binaries that could do not-so-nice things. Currently what is implemented with Azure DevOps that you can say something to a bot, which starts to produce the artifacts. It is so rarely used, I cannot even remember what the command is :P But this is also the reason the CI currently does not attach binaries to Pull Requests. Not because we cannot do that, but because it is unsafe unless marked otheriwse.
  • I would rename "appimage.yml" to "appimage-builder.yml" or what-ever the build-tool is called, just to make it clear there are several definition formats for different tools in the appimage eco-system. Otherwise, and I have the IRC logs to proof it, this is rather confusing (especially as the YAML file has no reference to the tool it is meant to be used by; possibly a nice addition too, just to mention: # Definition file for <url>, or what-ever floats your boat).
  • I would extend the appimage-builder first to allow for scripting of some sort, so the things in the workflow can go in the definition. This allows for people to test out AppImages without reading in the GitHub workflow how to do this. As I would personally put the downloading of the opengfx / opensfx / openmsx in there, as the fixing up of that one file. It would mean people can do: cmake .. && make && <command for appimage-builder> ../os/linux/appimage-builder.yml.

As a side-note: OpenTTD currently releases nightlies via Azure DevOps. It is planned to migrate to GitHub Actions, but as long as that migration is not finished, adding AppImages here will not make them appear on the nightlies download pages.

@FLHerne
Copy link
Contributor

FLHerne commented Oct 16, 2020

I would extend the appimage-builder first to allow for scripting of some sort, so the things in the workflow can go in the definition. This allows for people to test out AppImages without reading in the GitHub workflow how to do this. As I would personally put the downloading of the opengfx / opensfx / openmsx in there, as the fixing up of that one file. It would mean people can do: cmake .. && make && ../os/linux/appimage-builder.yml.

Alternatively, perhaps add the AppImage build as a CMake target performing these steps, so that users (and the workflow) can just do cmake .. && make appimage.

@azubieta
Copy link
Author

azubieta commented Oct 16, 2020

I have renamed the recipe file to appimage-builder.yml and added a first line comment linking to the tool documentation. Thanks for the hints. I'll update all my recipes right away.

Regarding the integration with cmake. I would like to improve the tool a bit more before investing time on this.

@azubieta azubieta force-pushed the master branch 2 times, most recently from f4e9e9f to 0d64177 Compare October 23, 2020 02:13
@azubieta
Copy link
Author

I've improved the recipe (and appimage-builder), now appimage-builder it's capable of patching stuff and the whole pack process takes a single instruction.

Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

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

Nice improvement compared to previous versions

os/linux/appimage-builder.yml Outdated Show resolved Hide resolved
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

We have been working on moving our release workflow to GitHub Actions, so I was wondering if we could integrate this into it already. Found a few things you could possible address for me!

os/linux/appimage-builder.yml Outdated Show resolved Hide resolved
os/linux/appimage-builder.yml Outdated Show resolved Hide resolved
os/linux/appimage-builder.yml Show resolved Hide resolved
os/linux/appimage-builder.yml Show resolved Hide resolved
os/linux/appimage-builder.yml Outdated Show resolved Hide resolved
@TrueBrain TrueBrain added candidate: most likely This Pull Request is probably going to be accepted candidate: yes This Pull Request is a candidate for being merged OS: Linux Issues specific to Linux builds size: small This Pull Request is small, and should be relative easy to process and removed candidate: yes This Pull Request is a candidate for being merged labels Dec 14, 2020
@TrueBrain
Copy link
Member

With all the work on the release workflow etc done, this can be updated to be integrated with the new release workflow.

The main issue the AppImage has (as well as the other single-package solutions), and which is kinda a blocker for integrating it with our own releases, is that with the inclusion of freepats, the binary becomes unreasonable large. Where most binaries are < 10 MiB, this all of a sudden is ~50 MiB last I checked. The main problem I have with that: it is only for music, which most people disable. So that is a lot of wasted bytes :D

We ran into the same issue with Emscripten, which resulted in packaging a version where sound is just completely disabled. That is not a proper solution for AppImage.

I keep wondering if there isn't a way to slim down freepats, even if it is opinionated, to reduce the download size. I know very little about MIDI, but how I understand it, freepats allows for multiple different synthesizers, where we in fact only need one, really. It would just mean we pick which one that is, instead of allowing to user to pick one. For packages like this and for example emscripten that is totally acceptable to me, but your-miles-may-vary.

This more as a braindump, as I keep seeing this PR, reading up on it, coming to this same conclusion a few times now :D

@azubieta
Copy link
Author

azubieta commented Jan 14, 2021

I the AppImage can use the "freepats" installation from the system. So if it's too big you can remove it and ship that. But being honest I don't care to give away even 30mb for a complete gaming experience (with sounds).

@TrueBrain
Copy link
Member

TrueBrain commented Jan 14, 2021

Well, I mention freepats as I know that from other platforms being an issue, but I was digging through the AppImage, and there are many more things that could be reduced in size:

  • Lot of "changelog.gz" files, which sum up to a nice sum of bytes
  • Lot of other unneeded files, bits and pieces
  • The full ICU data package (which is as big as freepats).

So I take it back, the problem is not with freepats on its own; it is a bit bigger :) And if I remember similar talks with snap and flatpack, we had similar issues there.

In general, I don't mind things like AppImage, but I do not see a way that we can distribute it with our current infrastructure; which means it cannot be part of our release workflow. It might be better if these files are distributed by third-party, as that is already the case with snap and flatpack (and even Docker images, for that matter).

For me what it comes down to: our goals here don't align. We cannot handle such big files for releases, and these images are, possibly by definition, fat.

This needs some more consideration on my end :)

@azubieta
Copy link
Author

If your issue is the infrastructure, you can use Github Workflows to build the packages and Github Releases to store the final binaries. This is already done by many others.

The idea is that the users don't have to trust or depend on a third party to run your software.

Comment on lines +32 to +33
- libsdl1.2debian
- libsdl2-2.0-0
Copy link
Member

Choose a reason for hiding this comment

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

having both of these is redundant

- libpng16-16
- libsdl1.2debian
- libsdl2-2.0-0
- libxdg-basedir1
Copy link
Member

Choose a reason for hiding this comment

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

can be removed now

- libsdl1.2debian
- libsdl2-2.0-0
- libxdg-basedir1
- timidity
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably want to prefer fluidsynth over timidity

@TrueBrain
Copy link
Member

As promised, I was going to come back to this.

For our Steam release, we had to invent a linux-generic, which we have been pushing out as nightly for the last 2 weeks. This is a bundle that contains the executable + .so files you are most likely missing. We create this for amd64, based on CentOS7, which means that most modern Linux installs can run it (+/- 10 years or younger). For us, this is a much easier thing to maintain, and is a relative small download size.

So for us, there is no direct reason to have AppImage, other than giving people options. But I think this is better left for 3rd parties to do so. Similar that we do not publish "official" Docker images, flatpaks, snaps, etc. We leave this, for now at least, to other people do run this. We might reconsider this in the future, but for now, we are fine with this. (and if we change this, we most likely will do it for all these targets, so AppImage, Docker, flatpak, ..).

In general, we removed most other ways we published for Linux, and reduced it to: what-ever CPack can produce. We noticed this is much less likely to break, and at least is a lot easier for us to maintain (read: it requires no maintenance from our side). So there is also no more rpm etc in our repo. These all moved downstream, and I am really happy people picked that up and are now maintaining them (seriously, if you are reading this and maintain rpm, flatpak, ..., thank you!)

So that leads me to a conclusion: I am going to close this Pull Request for now. Not, as I made clear in earlier comments, because I don't like this, but more because currently OpenTTD is not ready for this. And this is also why it took so long to come to this result ... because we are a bit conflicted about it :D

Sorry about this, and really thank you for your effort here. Who knows in a few months we get back to this :) Cheers!

@TrueBrain TrueBrain closed this Feb 18, 2021
@azubieta
Copy link
Author

Sorry to hear that, I'll setup some regular CI build to keep the binaries up to date.
If you like you can reference the following page as a source for getting the AppImages:
https://www.appimagehub.com/p/1425360/

@TrueBrain TrueBrain mentioned this pull request Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: most likely This Pull Request is probably going to be accepted OS: Linux Issues specific to Linux builds size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AppImage support
5 participants