xbmc
Add Flatpak-CI check for all PRs
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
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Clean up (non-breaking change which removes non-working, unmaintained functionality)
- [X] Improvement (non-breaking change which improves existing functionality)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that will cause existing functionality to change)
- [ ] Cosmetic change (non-breaking change that doesn't touch code)
- [ ] Student submission (PR was done for educational purposes and will be treated as such)
- [ ] None of the above (please explain below)
Checklist:
- [X] My code follows the Code Guidelines of this project
- [ ] My change requires a change to the documentation, either Doxygen or wiki
- [ ] I have updated the documentation accordingly
- [X] I have read the Contributing document
- [ ] I have added tests to cover my change
- [X] All new and existing tests passed
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 think a change in here could break the addons, right?
I would assume that, but I'm not sure
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.
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.
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.
# Add local git repo copy
yq -i eval '(.modules[] | select(.name == "kodi") | .sources) |= [{"type": "dir", "path": "'"$GITHUB_WORKSPACE"'/xbmc"}] + .' flathub/tv.kodi.Kodi.yml
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.
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.
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.
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.
And thats the exact info we need WHEN we need, at PR time.
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".
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 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
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?
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)
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 dash is used as a shell it now causes problems, so I would like to kick that out completely, a buildlog can still be written by doing make | tee ... if that is intended. Plus the flatpak-builder command to use needs to be configurable.
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 master to the beta branch of the Flathub-repo the dynamic yaml modifications didn't need any adjustments. So that worked out perfectly and just as intended (no manual intervention needed when doing changes).
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.
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).