Add Flatpak-CI check for all PRs #24920
Conversation
|
For some reason it is now complaining about mirrors.kodi.tv being unknown, which is odd as GitHub codespaces resolves the hostname properly. Let's hope it is just a temporary hickup. |
|
I don't really like this (hacky) approach personally . If we really don't want the addons in here I would much rather invert this and have the build file in here but without addons, then patch the file for usage on flathub. Anyway, was this a concern about build times? Did somebody actually measure them? |
|
The addons take about 10 minutes to build IIRC. Once the build runs through again we can do some benchmarking, the caching should help a lot with buildtimes. I think a change in here could break the addons, right? So we should try to build the addons if that's somehow reasonable. Especially, since there is a flatpak file falling out at the end, so that should be as complete as possible. I'm a huge fan of doing dynamic modifications of the file, like it's done here, that tends to make maintenance easier as there is only a single file that is modified. It doesn't matter much if it's adding or removing though, but I was under the assumption that flathub does it's own magic when building. |
I would assume that, but I'm not sure |
Flole998
force-pushed
the
flatpakci
branch
4 times, most recently
from
2c50d87
to
cdf799a
Compare
|
Baby steps. Currently we have ZERO insight into flatpak build status. @razzeee does a release when a major release is done (monthly at best). The intent is to be able to have a CI check for PR's. @razzeee also mentioned, currently flathub repo has to be used. Potentially this may change, but as it stands right now, @razzeee will need to maintain both this repo and the flathub repo for consistency of the flatpak build info if the other PR #24887 is used. IMO for the limitations we have right now, this is a better approach to use the flathub repo as the "source" for the build files. Giving only one location for @razzeee to maintain. We dont build addons in PR's for any other platform. Therefore the stripping approach is preferred regardless imo. My first thought was a separate yaml file for PR's that just didnt have the addons, but @Flole998 came up with this (better) approach to dynamically remove the addons from the sole yaml build file from the flathub repo. Simple, easy, the flathub repo still can (and should) build the addons for "actual" builds. We dont actually NEED a buildable artifact to be useable for PR's. Its more just so we can see if theres potentially breaking concerns, and then the PR author can approach the team (ie @razzeee ) about how to fix whatever breaking change is occured at PR time, rather than potentially months down the track. If/when we are able to build from our CI and upload directly to flathub, we can look at @razzeee 's other approach that brings everything in tree, but for right now, i think this does a simple job to allow insight with each PR about the build status of flatpak, of which we have zero right now. |
|
I should also mention, and this is solely my opinion, i like how @Flole998 just explicitly has each step laid out in the GHA, and isnt based on some "random" container that "hides" away some of the setup. We can see clearly what is required for anyone to dev/build a flatpak on their own system. None of that is/was documented anywhere currently, and it was something i had to fumble my way through months ago as well. We absolutely should document this, but in the meantime, i think this at least gives us something we can point people to if they wish to do dev work on a flatpak build for whatever reason. |
|
@Flole998 i assume it would be possible to do a build matrix that would allow building for example, both x86_64 and aarch64? Sort of how we currently build x86_64/arm/aarch64 for linux on jenkins to cover the popular arch types? |
|
The problem is, if you pr something here, that breaks flatpak, you will need to patch the flathub repo now, in addition and it becomes unclear which versions of both are needed. Especially after the fact. |
Flole998
force-pushed
the
flatpakci
branch
2 times, most recently
from
4bf0687
to
d18732a
Compare
| - name: Install dependencies | ||
| run: | | ||
| sudo apt-get update -y | ||
| DEBIAN_FRONTEND=noninteractive sudo apt-get install --force-yes -y cmake git build-essential ccache lsb-release flatpak-builder flatpak |
There was a problem hiding this comment.
You shouldn't install flatpak-builder here, but install it from org.flatpak.Builder then run it via flatpak. Otherwise we might run into intransparent errors/warnings.
There was a problem hiding this comment.
The docs mention
You also need to have installed flatpak-builder, which is usually available from the same repository as the flatpak package (e.g. use apt or dnf). You can also install it as a flatpak with flatpak install flathub org.flatpak.Builder.
which is why I went with the first option. I assume it doesn't change anything further down when going with flatpak install, so I can just add an additional step to do that and leave everything else as-is?
There was a problem hiding this comment.
Flatpak docs are not flathub docs. It's true that it will work for flatpak, but the flatpaked one is patched for flathub AFAIK.
There was a problem hiding this comment.
Got it, I'll change it and see how that goes. I'm currently doing some more testing and benchmarking with the caching to see how that improves the build times.
There was a problem hiding this comment.
You will need to call builder with 'flatpak run org.flatpak.Builder.... " after the change, see flathub docs.
There was a problem hiding this comment.
Is that something we should add to the Makefile? Maybe as an option to specify the builder to use in an environment variable, and default to "flatpak-builder" if none is specified?
There was a problem hiding this comment.
We can. Personally I'm not big on make files - I also think it's only used on personal system/dev case and not in the CI.
Flole998
force-pushed
the
flatpakci
branch
2 times, most recently
from
7982cd2
to
0f1427e
Compare
I dont understand what you mean by this. If flatpak builds fail becuause of a PR to xbmc, why would you do anything in your flathub repo? The intent is to make Flatpak a first class citizen correct? The fix should be in xbmc, and not some monkey patch up the line. |
|
Cause you're checking out the flathub repo and using it for building here, so either you would need to monkey patch here or commit the fix to the flathub repo. |
|
thats what the following is doing. Its checking out the PR via git, and changing the flathub to use that source checkout for the build. |
|
That's only the kodi version, depends are still used from flathub, if I understood what this does |
|
Why would we want to change depends? Your a linux system essentially, you provide the libs however the "distro" wants. In this case, you set your libs in flathub, and the expectation for any changes in kodi would be to work with those libs, just like we would for ubuntu 20.04, or debian 11, or whatever a distro packages specific versions of libs |
|
That's fair, but it will break kodi PRs building, if you add anything that's not in the base set. |
|
And thats the exact info we need WHEN we need, at PR time. |
|
It's checking out the PR source code, then it is checking out the flathub repo and then it is adjusting the path in the flatpak build config to use the local source instead. But it's already the case, that the flathub repo "version" must match the kodi repo "version", that's nothing specific to this PR. The only difference is, that now issues become visible before actually doing a release and trying to build the flatpak. You can still click "bypass checks" to merge something that breaks the build in case that becomes necessary or is intended. Also one thing I didn't know was, that there is a beta branch of the flathub repo, which is now used for builds. So what I imagine will (or should) be done is developing in the beta branch (in case changes do become necessary) and then, once a release is done, merge beta into master, and do a release build. |
Correct, but my pr would get around this. I also think having these sources in tree would make sense from the pov of discoverability and being able to keep stuff in step. For e.g. I usually end up using the hashes from this repo in the flathub repo too, if possible. It would be nice to change these with one commit. |
Yes, but fixing it would mean you need to commit to another repo and figure more stuff out. Possible, but I don't think we want to make the contribution flow that hard. |
|
its extremely rare for something to be introduced that needs a new lib, and for existing lib usage within reason we generally aim to support "supported" versions. (ie python 3.7-3.12, fmt 6-10, etc). I highly doubt your going to get many PR's that would need to update to bleeding edge. Again, we never expect linux to have specific versions of any lib used. Im not against bringing it in tree, but the stuff in the other PR scares the shit out of me. Having 100+ hashes/tarballs to maintain is a nightmare, and again, this still goes back to, if you do bring in tree, you still need to currently maintain the flathub, as we cant upload from our CI correct? |
|
We can certainly do more changes in this PR if that's beneficial (especially since this would kick out one of my dynamic patches), or we do it in multiple steps. We just need to agree on some way of doing it, and then we can move from there. A single PR can have more than a single commit/author in it, so if there's an agreement to move everything in-tree I'm perfectly fine with @razzeee commiting the change into this PR, so it can all be merged at once. Or I can also PR against the other repo so it becomes part of the other PR. For me, none of this is set in stone and it was just an idea during DevCon that someone like me, a Kodi-Dev-Newcomer, can do without spending multiple days to study the codebase. Especially as it came up during the discussions as "something someone should do". |
We can do the same thing in the flathub repo, we're already pulling in the repo, then add the addons with yq. Maintanance would be about the same, locking to newer kodi release, running the addon script, build then test. |
|
@Flole998 I hope I don't come off as mean. Really appreciate you taking a look. I do think this is progress, but I would like it to go a little farther in the integration story. Hopefully making things easier for for people not as familiar with it. I think if we mash up our PRs that would be a better start. |
|
As I understood it there is some "standard build procedure" that flathub goes through and that can not be modified/extended? It's not based on GitHub actions, so it's more difficult to do it in the flathub repo? Or can you tell it to use a flatpak file from another repo? |
|
Maybe we are talking about different things here. The point to this was purely for CI on PR's. it doesnt implicitly have anything to do with actual flathub releases that you do, other than the fact we are potentially highlighted of PR's that are going to cause issues for a flathub release when that time comes. We dont intend to make a release in tree, we just want to know it builds (just like windows, freebsd, the linux variants, apple, etc). we dont care/use the artifact (just like windows/linux). I saw this as a path of least resistance just to have CI checks. nothing changes on the flathub end until you work out whatever you want done (ie in tree, out of tree, CI uploads, etc etc). But anyways, i'll step back, ive got no skin in this game really. @Flole998 is in slack as well @razzeee so maybe hook up in chat there and maybe you guys can work together to whatever goal you deem suitable |
No, the manifest needs to be in that repo, I was thinking, that we could extend the file with some tricks, but not sure that's a good idea. (checking out a submodule should work I think) |
Flole998
force-pushed
the
flatpakci
branch
9 times, most recently
from
d45fb2d
to
e7cc4ce
Compare
|
I've messed around with the caching a little more and got the build time down to less than 30 minutes (without addons). Using the flathub flatpak-builder package doesn't really affect the buildtime, so I am using that now instead of installing through apt. Enabling the addons makes the build take about 70 minutes on the second run (first run is creating the cache, took 90 minutes). The Makefile will need a few changes: The tee-nightmare isn't over, apparently when debians I saw that there is a debug package created? Is that also something that should be put in a flatpak-file and uploaded as a separate artifact? If it provides something that is useful, IMO that should be done. BTW: When I switched from the |
|
I'm a bit confused, my branch with addons needs 45mins in the PR, so that seems weird |
|
You are caching even more than I am (the entire .flatpak-builder directory), I will add that to my cache now and see if that improves the build time (spoiler: it should). EDIT: Right now it doesnt build cause https://ftp2.osuosl.org is down. I can re-try once they are back up. |
Flole998
force-pushed
the
flatpakci
branch
2 times, most recently
from
c1907f1
to
6181938
Compare
|
Got it down to 37 minutes now including the addons. When removing the addons it takes 11 minutes. If you have some time tomorrow/later today @razzeee then we can talk about the open questions (having the Flathub repo in-tree vs. in a separate repo being the biggest question I think). |
| - name: Checkout Flathub config | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: 'flole998/tv.kodi.Kodi' |
There was a problem hiding this comment.
I guess we'll change this before merging? Leaving a comment as a reminder
| CCACHE_DIR: /tmp/ccache | ||
| FLATPAKBUILDER: flatpak run org.flatpak.Builder | ||
| run: cd flathub && dbus-launch make build | ||
| - name: flatpak |
There was a problem hiding this comment.
I guess this is the flatpak build?
There was a problem hiding this comment.
This creates the .flatpak file, yes.
There was a problem hiding this comment.
I guess I'm asking for making the step names a bit more explicit then
There was a problem hiding this comment.
Sure, do you know if there's a "proper" name for these operations? The first one is "Build Kodi" I assume, this one is "Create flatpak-file"?
There was a problem hiding this comment.
make build is doing the build as specified in the Makefile (using flatpak-builder). However, flatpak relies on dbus, so it needs to be setup/running properly. On a "normal" system you could simply run make build, but when doing it in a GitHub actions container this becomes necessary.
I'll have a look at how others are doing it, maybe there's something to learn from it.
The provided actions would now allow kicking out the addons before the build I think?
There was a problem hiding this comment.
I think you meant to write not not now.
If that's the case, no, I think we would need to still do most of them (probably minus the dbus stuff and I guess it should also do caching for us)
There was a problem hiding this comment.
Yes, "not" not "now".
It does caching, yes. My manual approach is still 8 minutes faster for some reason, I assume it is due to caching.
I don't think you can run commands between checkout and doing the build when using the "magic" action. It does everything under the hood, which is fine until you want to customize stuff (like removing addons before building).
There was a problem hiding this comment.
I might be missing something then, I would expect it to just work fine, if the docs here are correct https://github.com/flathub-infra/flatpak-github-actions/blob/master/README.md?plain=1#L26
Why wouldn't you be able to change what you checked out in the line before?
There was a problem hiding this comment.
And if we can figure out how to improve the caching of the action too, that would be huge for everyone involved with flatpak :)
Description
This builds a flatpak file for each PR and reports the status of the build.
Motivation and context
Right now only after an actual release build errors in flatpak can be identified. Also right now, there is no way to easily download a flatpak build of a specific PR and run it. This PR adresses both.
How has this been tested?
Running this on my repo to see if the builds are running through.
What is the effect on users?
Screenshots (if appropriate):
Types of change
Checklist: