ci: implement auto-update groups #24344
Conversation
cd1991a to
0d47560
Compare
|
Now it seems to be fine. |
|
All valuable changes are under |
| @@ -85,13 +85,6 @@ termux_step_host_build() { | |||
| } | |||
|
|
|||
| termux_step_pre_configure() { | |||
| # Version guard to keep flang in sync | |||
There was a problem hiding this comment.
Shouldn't this have a mutual TERMUX_PKG_ALIGN_VERSION_WITH=flang to coincide with Flang's TERMUX_PKG_ALIGN_VERSION_WITH=libllvm?
I wouldn't expect any issues from such a "loop" in alignment.
And if that does cause an issue, it should be handled by the alignment solver.
It should be visible from every package that has an alignment with another package that that alignment exists.
Just as every package in an update group should visibly be part of said update group.
There was a problem hiding this comment.
TERMUX_PKG_ALIGN_VERSION_WITH handling code check package referenced in this variable and all packages referencing current package in their TERMUX_PKG_ALIGN_VERSION_WITH if any. So it is kinda being checked both ways.
Update group does not mean all packages in update group must have same versions. It only means they should be updated in single push/pull_request to avoid dependency update hell.
There was a problem hiding this comment.
I would still prefer to have a visual/text indicator of alignment or group membership in every package with such a relation.
Makes it significantly easier to search for such relations.
So in this case adding a,
TERMUX_PKG_ALIGN_VERSION_WITH=flangmakes it obvious from both package's build script that they require alignment with the other.
We should probably also turn TERMUX_PKG_ALIGN_VERSION_WITH into a comma separated list like TERMUX_PKG_LICENSE for example, since more than 2 packages may need to be version aligned with each other.
There was a problem hiding this comment.
Ofc you can do that to make it obvious, but it is not required. Anyway when you will try to update one of them without updating another one you will get version mismatch error.
There was a problem hiding this comment.
Please cross-reference it for all packages that we're adding this to at the inception of the feature so we don't have to track these down later.
I'm even considering adding a linter rule to enforce full cross-referencing of alignments so this doesn't become a mess to figure out.
Note
Also in case you didn't catch it earlier,
Turning TERMUX_PKG_ALIGN_VERSION_WITH into a comma separated list
like TERMUX_PKG_LICENSE would be helpful since more than 2 packages may need to have their versions aligned.
There was a problem hiding this comment.
Probably it will be better to make checking code to find all packages referencing same package in TERMUX_PKG_ALIGN_VERSION_WITH to make sure version guard works for all of them.
Otherwise we might end up with referencing 60+ packages in every 60 buildscripts of them. And it will end up with a tree wide updating buildscripts if we add more kframeworks6 libraries.
like TERMUX_PKG_LICENSE would be helpful since more than 2 packages may need to have their versions aligned.
I solved this by mentioning same package in all of packages requiring version aligning.
There was a problem hiding this comment.
Probably it will be better to make checking code to find all packages referencing same package in
TERMUX_PKG_ALIGN_VERSION_WITHto make sure version guard works for all of them.Otherwise we might end up with referencing 60+ packages in every 60 buildscripts of them. And it will end up with a tree wide updating buildscripts if we add more kframeworks6 libraries.
That's where the update group comes in though, no?
It doesn't imply version alignment, but it does imply a grouping.
like TERMUX_PKG_LICENSE would be helpful since more than 2 packages may need to have their versions aligned.
I solved this by mentioning same package in all of packages requiring version aligning.
Thanks. I feel like this is gonna be a mess otherwise so I wanna get ahead of it.
There was a problem hiding this comment.
That's where the update group comes in though, no? It doesn't imply version alignment, but it does imply a grouping.
Yep. TERMUX_PKG_ALIGN_VERSION_WITH should come in separate PR, but it changes same files so I not decided if I should move it away from this PR. And also it helps us make sure nobody will break packages with randoms PRs updating specific packages despite of existing (or not yet existing) version guards.
| @@ -6,7 +6,8 @@ TERMUX_PKG_MAINTAINER="Joshua Kahn @TomJo2000" | |||
| TERMUX_PKG_VERSION="0.40.0" | |||
| TERMUX_PKG_SRCURL=https://github.com/mpv-player/mpv/archive/v${TERMUX_PKG_VERSION}.tar.gz | |||
| TERMUX_PKG_SHA256=10a0f4654f62140a6dd4d380dcf0bbdbdcf6e697556863dc499c296182f081a3 | |||
| TERMUX_PKG_AUTO_UPDATE=false | |||
| TERMUX_PKG_AUTO_UPDATE=true | |||
| TERMUX_PKG_AUTO_UPDATE_GROUP=x11/mpv | |||
There was a problem hiding this comment.
Right, so how did you choose the names for the update groups?
Because It's obviously not based on the file paths.
And I'd prefer to not bring channel information into the group names.
If the update group names are "arbitrary" I'd prefer them to follow the project name or a suitable proxy like kf6
| TERMUX_PKG_AUTO_UPDATE_GROUP=x11/mpv | |
| TERMUX_PKG_AUTO_UPDATE_GROUP="mpv" |
There was a problem hiding this comment.
Name of update group is copied directly to PR description and, as a consequence, to git commit in the case if you squash two commits after PR reviewing.
There was a problem hiding this comment.
I think it'd probably be better to not put semantic meaning into the name of the update group.
What would the current commit messages be anyway?
I'd expect it to just bundle up a set of bump($CHANNEL/$PKG_NAME): $NEW_VERSION commits into a PR under the title of:
if (( "${#versions[*]}" == 1 )); then
title="bump(${group}): ${versions[@]}"
else
title="bump(${group}): group update"
fiIn which case bump(x11/kframeworks6): group update vs. bump(kf6): group update is a purely cosmetic difference in the PR title.
There was a problem hiding this comment.
There may also be a case where a package may be part of multiple autoupdate groups.
Although that is probably best left for when it actually happens.
There was a problem hiding this comment.
In which case
bump(x11/kframeworks6): group updatevs.bump(kf6): group updateis a purely cosmetic difference in the PR title.
All packages under kf6 group have version aligning enabled so commit message will be, i.e., bump(x11/kf6): 6.14.0 which complies with commit messages we had before.
Currently all other update groups or have single package (big package) or have version aligning.
There may also be a case where a package may be part of multiple autoupdate groups.
It should not happen. It is not designed this way.
There was a problem hiding this comment.
In which case
bump(x11/kframeworks6): group updatevs.bump(kf6): group updateis a purely cosmetic difference in the PR title.All packages under
kf6group have version aligning enabled so commit message will be, i.e.,bump(x11/kf6): 6.14.0which complies with commit messages we had before. Currently all other update groups or have single package (big package) or have version aligning.
I don't think the versions always matching or all packages in a group always being updated at the same time is a particularly good assumption to make.
Say I'd like to make an update group for tree-sitter later for example,
those versions don't match, and they're often but not always updated at the same time.
Although I suppose I shouldn't be conflating this with a "rebuilds group".
There may also be a case where a package may be part of multiple autoupdate groups.
It should not happen. It is not designed this way.
Not right now.
But yeah no point implementing a feature we don't have a need for yet.
There was a problem hiding this comment.
In my understanding auto-update group definition is similar to rebuild group: group of packages which are required to be updated in the same PR/push because of version or ABI mismatch. I planned to implement something like creating autoupdate group for, i.e. boost with automatic revbumping of all revdeps (because of possible ABI mismatch of newer boost version).
But yeah no point implementing a feature we don't have a need for yet.
As I said before my understanding of update group is similar to rebuild group so I do not see the point.
There was a problem hiding this comment.
Suggested change
TERMUX_PKG_AUTO_UPDATE_GROUP=x11/mpv TERMUX_PKG_AUTO_UPDATE_GROUP="mpv"
Since these two packages exist in different repos we can omit the x11/ preffix. But PR title will be less fine after this 😞 .
There was a problem hiding this comment.
I'm gonna reiterate that I don't think repo channel information should be part of the update group names.
There was a problem hiding this comment.
Probably it should be in the case if all packages in update group exist in the same repo. Just for consistency with other bump commits.
0d47560 to
b449aac
Compare
|
Or maybe it is here somewhere in the large list of changes and I just can't find it on my screen, I am sorry if that is the case. |
b449aac to
3ce3d95
Compare
Yep. Added v4l-utils to the list as well. |
|
Oh ok, and if that one is added, then maybe |
I remember you brought up making |
Probably I can open PR for this. |
|
Ok, I filed PR for brotli: #24397. |
This change make it possible to: 1. Perform package updates in separate PR (and perform building for all archs, unlike auto-update workflow where package is build only for one arch). 2. Update all packages in update-group in a single step (and ensure all versions are aligned if needed). 3. Perform auto-updating big packages in separate run, no matter what uptime in auto-update workflow is. [no ci]
3ce3d95 to
9c349a6
Compare
|
I wrote a small script to check if there are more packages that might need version aligning, but results are not as useful as I thought. result |
|
BTW after merging this somebody should add some info about |
|
Seems useful, 👍 thanks for working on this. |
probably such a script would be more helpful if it also takes into consideration the |
I do not think so since they can not be installed simultaneously, they are replacing each other. |
Unfortunately it turns out that maybe I am sorry, it is kind of my fault because I did not notice it until someone else found the error. |
This change make it possible to:
Also this PR implements 2 new package variables:
TERMUX_PKG_ALIGN_VERSION_WITHfor version guards. If versions of package being built and the package the variable refers not match build will fails with error likeERROR: Version mismatch between mpv and mpv-x..TERMUX_PKG_AUTO_UPDATE_GROUP: all packages in same update group will be updated together, in separate PR created by @termuxbot2 . Auto-merging PR in the case of successful build is not a part of this PR.All version guards are removed in prior to
TERMUX_PKG_ALIGN_VERSION_WITH.Also this PR implements 5 auto-update groups:
x11/firefox,x11/thunderbird(because these 2 are big packages),x11/mpv,x11/kframeworks6(because their versions should be aligned) andx11/webkit(both because these packages building takes 2 hours each and their versions must be aligned).[no ci]