search

greatscottgadgets / packetry

A fast, intuitive USB 2.0 protocol analysis application for use with Cynthion.
BSD 3-Clause "New" or "Revised" License
123 stars 24 forks source link

Add CI workflow for building the Linux AppImage executable. #140

Closed antoinevg closed 1 month ago

antoinevg commented 1 month ago

This PR adds support for building the Linux AppImage binary during CI workflow.

Overview

The workflow file is .github/workflows/appimage.yml and is structured as follows:

Step 1: Checks if the docker container exists or needs to be updated, if not, go to Step 3.

Step 2: Builds the appimage/docker/Dockerfile for compiling packetry and creating an appimage.

Step 3: Runs pretty much the same workflow as the rust.yml workflow to compile & test packetry inside the docker container.

Step 4: Invokes a custom action appimage/action.yml to build the Linux AppImage binary.

Step 5: Uploads the Linux AppImage binary artifact.

Docker Image Notes

Outstanding issues

Closes #117

antoinevg commented 1 month ago

Firstly, thank you for a great and gentle review @martinling! :-)

  • appimage/docker/files/etc/passwd: presumably this is required because the Docker image needs an /etc/passwd for various tools to function, but there are a lot of what seem like extraneous lines here. Where did these come from, and do we really need all of them? Can we trim this down, or copy most of it from somewhere and just add what we need at build time?

So this is an excellent point. At the time of review this was pulled from the Docker image and modified before adding it to the repo but I've since updated the Dockerfile to just do an in-place edit of the /etc/passwd file.

  • appimage/packetry.AppDir/usr/share/doc/*/copyright: These are presumably Debian copyright files - but from what set of package versions, and why those particular packages? I thought at first perhaps it was just libgtk-4-1 and its dependencies, but libharfbuzz-dev is also included. And why are they committed to the Packetry repo, rather than just being copied from the versions used at build time?

This was the stickiest issue of all of them. The short version is that the AppImage linuxdeploy tool can only provide the licenses for packages that are installed on the system as the machine-readable Debian copyright format is not part of the source distribution. So basically, I had to get the gtk4 licenses from upstream and add them as manual licenses.

I talk a bit more about our longer-term options in the maintenance notes.

ps. I figured out why libharfbuzz was being pulled in and it's no longer part of the build.

  1. There is a lot of hand-written addition of .so symlinks here, using filenames of specific library versions. Can this be made less fragile? I would have thought that ldconfig with the right parameters could create the symlinks automatically, for instance.

The short version is that ldconfig is being a right royal pain in the posterior with the gtk4 libs so I just did it manually. That said, this has now been replaced by a small shell script that should spark more joy.

  1. There are hand-coded lists of library dependencies that aren't actually linked to in the current version. But code changes in Packetry or gtk-rs could easily cause some of these to start being linked in as we vary our usage of GTK. Is this something we can automate, with some scripting using ldd and dpkg -S perhaps?

So there are two kinds of lib here:

  1. Libraries being pulled in even though we don't link to them. I've managed to eliminate all of these with some judicious surgery on installed system packages.
  2. Libraries being pulled in that we do link to, but which we really don't want in there because they're going to break things if the runtime environment is using different versions. Normally we'd be excluding these explicitly via the linuxdeploy command line but the linuxdeploy-plugin-gtk messes with this somewhat.

That said, I think our newly streamlined list no longer has anything left in it that will cause problems if we change our usage of gtk4:

rm -f libX*
rm -f libblkid*
rm -f libbsd*
rm -f libpixman*
rm -f libxcb-render*
rm -f libzstd*

The answer to any or all of these things may be "these things are hardcoded because automating it is more faff than it's worth right now", and that's not unreasonable, but as it stands, if I were looking at needing to make a new release tomorrow without you, I wouldn't know what I needed to do.

So I think if we're not able to automate these details, then we at least need a bit of documentation in the repo that explains how the AppImage build process works, what steps may be needed to update it, and when and how those steps need to be done.

Fair point! I've added some docs to the directory that should help: appimage/README.md

In the longer run, when there's a bit more faff available I would like to work on two things:

martinling commented 1 month ago
  • appimage/docker/files/etc/passwd

So this is an excellent point. At the time of review this was pulled from the Docker image and modified before adding it to the repo but I've since updated the Dockerfile to just do an in-place edit of the /etc/passwd file.

Great, that's a lot tidier!

  • appimage/packetry.AppDir/usr/share/doc/*/copyright

This was the stickiest issue of all of them. The short version is that the AppImage linuxdeploy tool can only provide the licenses for packages that are installed on the system as the machine-readable Debian copyright format is not part of the source distribution. So basically, I had to get the gtk4 licenses from upstream and add them as manual licenses.

I talk a bit more about our longer-term options in the maintenance notes.

I'm not sure if I'm understanding correctly.

We build GTK4 from source rather than having libgtk-4-1 installed on buster, so we can't just grab /usr/share/doc/libgtk-4-1/copyright, I get that. But we're not building GTK's dependencies from source. So can't we just copy the copyright files from the buster system into the AppDir, for all of:

Or are we bundling newer versions of those libraries, from a later distro version, when we actually build the AppImage? And if so... how do we know what versions those are, and how do we know whether we still have the correct corresponding license and copyright info for them? As far as I can see, we can't tell.

I'm fine with using the buster versions, because that distro is no longer getting updates, and I'm fine with the GTK4 version, because the version used for that is explicitly set in the build scripts.

But if anything else is ending up in the AppImage, then that's a moving target which can become out of sync with what we have in the Packetry repo, at any time, and without any warning.

The short version is that ldconfig is being a right royal pain in the posterior with the gtk4 libs so I just did it manually. That said, this has now been replaced by a small shell script that should spark more joy.

Joy is duly sparked, thank you.

So there are two kinds of lib here:

  1. Libraries being pulled in even though we don't link to them. I've managed to eliminate all of these with some judicious surgery on installed system packages.

Why are these pulled in at all? Is this due to the difference in dependencies between the minimal GTK we build from source, and that of the distro packages?

  1. Libraries being pulled in that we do link to, but which we really don't want in there because they're going to break things if the runtime environment is using different versions. Normally we'd be excluding these explicitly via the linuxdeploy command line but the linuxdeploy-plugin-gtk messes with this somewhat.

These are supposed to be dealt with by the AppImage excludelist, aren't they?

That said, I think our newly streamlined list no longer has anything left in it that will cause problems if we change our usage of gtk4:

rm -f libX*
rm -f libblkid*
rm -f libbsd*
rm -f libpixman*
rm -f libxcb-render*
rm -f libzstd*

I'm fine with hardcoding this list if all of our dependencies are hardcoded to fixed versions, and if CI will fail in some way if we get out of sync. Is that the case, though?

Fair point! I've added some docs to the directory that should help: appimage/README.md

These are very helpful, thank you! I think I'm still not fully understanding though, as is probably clear from the above.

In the longer run, when there's a bit more faff available I would like to work on two things:

  • Come up with a solution for the gtk4 licenses that won't require us to track upstream license changes.

I'm fine with tracking upstream license changes by hand, if we're also updating dependency versions by hand.

But if we're going to have a situation where the binaries can get out of sync with the license/copyright text, just because a distro shipped new packages and the CI picked them up, that's not really OK.

  • Deprecate our use of linuxdeploy-plugin-gtk in favour of a homebrewed solution.

Is the original beyond improvement?

antoinevg commented 1 month ago

I'm not sure if I'm understanding correctly.

We build GTK4 from source rather than having libgtk-4-1 installed on buster, so we can't just grab /usr/share/doc/libgtk-4-1/copyright, I get that. But we're not building GTK's dependencies from source. So can't we just copy the copyright files from the buster system into the AppDir, for all of:

* libgdk-pixbuf-2.0-0

* libglib2.0-0

* libgraphene-1.0-0

* libpango-1.0-0

* libpangocairo-1.0-0

* libpangoft2-1.0-0

Or are we bundling newer versions of those libraries, from a later distro version, when we actually build the AppImage?

Ah, now I understand the confusion.

So all of those are generated by the gtk4 build. This means they will always correspond to the release version of the sources we're compiling.

This confused the hell out of me as well because at one point we were pulling libharfbuzz from the buster packages because it got pulled in by some other utterly random dependency that we also didn't need but the gtk build scripts die if it's not there.

This was causing a whole bunch of other stuff to not be built from gtk4 sources and then getting pulled into the AppImage from buster that should have been supplied by the sources.

So to answer your first question:

And if so... how do we know what versions those are, and how do we know whether we still have the correct corresponding license and copyright info for them? As far as I can see, we can't tell.

We're only extracting and copying the copyright file from the upstream .deb files. They have nothing to do with the build process except for the hour I spent copying them into the git repo and will only need to be checked for updates if we switch to a newer gtk4 source release.

This is why I think we're okay with the current approach.

But if anything else is ending up in the AppImage, then that's a moving target which can become out of sync with what we have in the Packetry repo, at any time, and without any warning.

Nope, nothing else from upstream ending up in the AppImage except the copyright files.

So I reckon we're good?

  1. Libraries being pulled in that we do link to, but which we really don't want in there because they're going to break things if the runtime environment is using different versions. Normally we'd be excluding these explicitly via the linuxdeploy command line but the linuxdeploy-plugin-gtk messes with this somewhat.

These are supposed to be dealt with by the AppImage excludelist, aren't they?

Yeah exactly that, in a non linuxdeploy* workflow we'd be doing something like that.

That said, I think our newly streamlined list no longer has anything left in it that will cause problems if we change our usage of gtk4:

rm -f libX*
rm -f libblkid*
rm -f libbsd*
rm -f libpixman*
rm -f libxcb-render*
rm -f libzstd*

I'm fine with hardcoding this list if all of our dependencies are hardcoded to fixed versions, and if CI will fail in some way if we get out of sync. Is that the case, though?

All dependencies are hardcoded to fixed versions.

Unless we change:

a) the gtk release version we're building from or b) the base image we're using for the Dockerfile

…it will keep running until the day that CI breaks because our gtk4 release is too old to compile packetry anymore. 🫠

I'm fine with tracking upstream license changes by hand, if we're also updating dependency versions by hand.

👌

But if we're going to have a situation where the binaries can get out of sync with the license/copyright text, just because a distro shipped new packages and the CI picked them up, that's not really OK.

💯

  • Deprecate our use of linuxdeploy-plugin-gtk in favour of a homebrewed solution.

Is the original beyond improvement?

It's more a case of what I'd like it (and in some cases linuxdeploy) to not do.

Having now spent a couple of weeks working with them both I think there's a strong case to be made for keeping the tool surface area as narrow as possible. Both those tools are (understandably) making very broad assumptions that we then have to undo as part of our workflow.

It would probably be a lot cleaner and more maintenance-friendly in the long run to just generate packetry.AppDir ourselves. Then have appimagetool generate the final redistributable. But that's another conversation for another day…

martinling commented 1 month ago

So all of those are generated by the gtk4 build. This means they will always correspond to the release version of the sources we're compiling.

Ahh, OK! I hadn't realised that the GTK4 tarball build would pull those other projects in directly.

Please add something about this to the docs! :smile:

The other detail that had made this confusing for me was that the action.yml didn't seem to be referenced anywhere (grepping the diff for action.yml only finds the reference in the docs), so I wasn't clear on where that was getting run from, and what environment it would be executing in.

I've now figured out that it's this step:

- name: Run build appimage action (Linux)
  uses: ./appimage/

...and that the action.yml part must be implicit, but I'd missed that on my first pass through. So a comment at that spot explaining that the action definition can be found in appimage/action.yml would be helpful.

We're only extracting and copying the copyright file from the upstream .deb files. They have nothing to do with the build process except for the hour I spent copying them into the git repo and will only need to be checked for updates if we switch to a newer gtk4 source release.

This is why I think we're okay with the current approach.

Yes, I agree that's fine.

It's more a case of what I'd like it (and in some cases linuxdeploy) to not do.

Having now spent a couple of weeks working with them both I think there's a strong case to be made for keeping the tool surface area as narrow as possible. Both those tools are (understandably) making very broad assumptions that we then have to undo as part of our workflow.

It would probably be a lot cleaner and more maintenance-friendly in the long run to just generate packetry.AppDir ourselves. Then have appimagetool generate the final redistributable. But that's another conversation for another day…

Yup, I don't see a need to try to streamline this right now.

antoinevg commented 1 month ago

Ahh, OK! I hadn't realised that the GTK4 tarball build would pull those other projects in directly.

Please add something about this to the docs! 😄

Done 😆

...and that the action.yml part must be implicit, but I'd missed that on my first pass through. So a comment at that spot explaining that the action definition can be found in appimage/action.yml would be helpful.

Done, good call!