Re: stretch-pu: plasma 5.8.7 LTS pre-approval
(resending with right list address)
Maximiliano Curia writes ("stretch-pu: plasma 5.8.7 LTS pre-approval"):
> The source packages that I would like to update in stretch are:
Thanks. I am not a RM but I am trying to help out by providing review
comments. I have reviewed this request.
tl;dr: Most of them are very good. Two are questionable:
plasma-workspace
plasma-desktop
One caveat for all the packages: they all had big translation updates.
I ignored these. I assume these are fine for stretch-pu.
In each case I have been relying on the accuracy not only of the
provided debdiff but the provided "packaging" diff and upstream
git log. I found the latter particularly helpful - thank you!
Overall I would like to say that I am impressed with the associated
documentation, and what I saw of upstream relase processes. With the
two exceptions I mention above, I was convinced by the thoroughness of
the approach upstream. Even when I didn't understand the code
etc. myself, upstream seemed to be making decisions on the right basis
and with good review.
> bluedevil/4:5.8.7-1+deb9u1
> breeze-gtk/5.8.7-1+deb9u1
> kde-cli-tools/4:5.8.7-1+deb9u1
> kscreenlocker/5.8.7-1+deb9u1
> plasma-pa/4:5.8.7-1+deb9u1
> user-manager/4:5.8.7-1+deb9u1
> kwin/4:5.8.7-1+deb9u1
> libksysguard/4:5.8.7-1+deb9u1
> systemsettings/4:5.8.7-1+deb9u1
These LGTM. I did notice a few things that are IMO not of concern:
The urls
https://gnuservers.com.ar/~maxy/debian/plasma_5.8.7_stretch-pu/kscreenlocker_5.8.4_5.8.7.upstream.gitlog
https://gnuservers.com.ar/~maxy/debian/plasma_5.8.7_stretch-pu/systemsettigns_5.8.4_5.8.7.upstream.gitlog
referred to in the bug report are 404. The urls are wrong and should
be
https://gnuservers.com.ar/~maxy/debian/plasma_5.8.7_stretch-pu/libksysguard_5.8.4_5.8.7.upstream.gitlog
https://gnuservers.com.ar/~maxy/debian/plasma_5.8.7_stretch-pu/systemsettings_5.8.4_5.8.7.upstream.gitlog
Do you generate these requests by hand ?!
Secondly, this in the changelog entry for libksysguard 4:5.8.7-1 is
rather odd:
| * Add new patch: Drop-html-markup-from-polkit-action-file.patch.
| Thanks to Michael Biebl for reporting (Closes: 696905)
...
| * Drop upstream applied patch: Drop-html-markup-from-polkit-action-file.patch
and it confused me briefly.
Now for plasma-workspace and plasma-desktop.
These have a strikingly lower level of explanation in the upstream
commit messages. In many cases I was left wondering whether
- anyone had even considered why or why not this ought to
go to a stable branch, and what its risks might be
- anyone had reviewed the patch
- why it was even being done.
Plus some specific issues:
> plasma-workspace/4:5.8.7-1+deb9u1
There was a lot of noise in the packaging debdiff about the header of
many of the patches. This effectively prevented me from doing a
thorough review, without more admin work.
IMO this needs an explanation, and/or a mechanical verification that
the patches-applied code is unchanged.
> plasma-desktop/4:5.8.7.1-1+deb9u1
The following two changes were particularly questionable IMO:
+ Backport 5.9/Master's GroupDialog code to 5.8.
This brings us to a common baseline on the three active branches
and addresses regressions on the 5.8 branch.
5.9's code added a scrollbar and improved keyboard nav.
Fixes KDE#378042
+ [Task Manager] Keep entry highlighted when context menu or
group dialog is open This makes it easier to see what item the
menu or popup is for. In fact, the item should have stayed
highlighted when the context menu is open but this was broken.
Also, for consistency, use the hover effect also for the group
dialog (it used to be the "active" task).
I think it is probably not practical to separate out these changes
(and indeed other inappropriate ones) from the upstream stable
branches.
I think taken together the changes are probably a net benefit but
there is a nontrivial risk that some of them are more intrusive than
our users expect from Debian sta[b]le.
In any case, the final decision is for the RMs.
Regards,
Ian.
Reply to: