Differential D28701

Add KPackage support to KNewStuffCore
ClosedPublic
Actions

Authored by leinir on Apr 9 2020, 12:41 PM.

Details

Reviewers
ngraham
mart
davidedmundson
broulik
bshah
Group Reviewers
Plasma
KNewStuff
Frameworks
Commits
R304:3f38da8a70d8: Add KPackage support to KNewStuffCore
Summary

Adding support for KPackage directly to KNewStuff means that we are
able to deal more gracefully with things like Plasma's Global Themes
(and indeed any other kpackage based thing).

This is done by adding another archive specialisation to the installer
class, and by also adding a check to the cache to ensure that even
when a kpackage is removed from the system outside of KNewStuff,
it does not remain seemingly installed in the KNS lists.

  • Make sure the cache gets written periodically
  • Add KPackage support to KNSCore::Installation
  • Introduce a getter (and enum) for the uncompress Installation setting
  • Add a redirection to the knsrc documentation location
  • Add a function to clean the cache of functionally stale entries
  • Clean the cache when the uncompression method is set to kpackage
  • Add a fallback for unconverted kpackage based knsrc files
  • Clean up some of the error reporting, and reset the entry's state
  • Check if installedFile is a file, if so bypass KPackage and delete
  • Add a KPackageType property to Installation, for fallback purposes
  • Add documentation for the new knsrc bits
  • Handle adopting an already installed kpackage item
  • Also uninstall not-adopted-but-there possibly installed kpackage bits
  • Add a simple async job wrapper for KPackage operations (and use it for the installation handling tasks in Installation)

BUG:418466

Test Plan

There are two options for testing out this patch:

  1. Use an existing knsrc file which uses kpackage installation (such as plasma themes), which will use the fallback
  2. Manually convert such a knsrc file, by removing the uninstall and installation commands from the knsrc file, and adding in an "Uncompress=kpackage" line instead

Both these should result in the KPackage path being used. You should see this on the command line when attempting to install an item, resulting in lines like "Using KPackage for installation", as well as more pleasant error reporting in the UI in the cases where something goes wrong.

To turn on debug output for KNewStuffCore, add QT_LOGGING_RULES="org.kde.knewstuff*=true" to your command line. For example, you can launch the test dialogue directly by launching the following from your build directory:

QT_LOGGING_RULES="org.kde.knewstuff*=true" ./bin/khotnewstuff-dialog plasma-themes.knsrc

Diff Detail

Repository
R304 KNewStuff
Branch
add-kpackage-support (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24984
Build 25002: arc lint + arc unit
leinir created this revision.Apr 9 2020, 12:41 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 9 2020, 12:41 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
leinir requested review of this revision.Apr 9 2020, 12:41 PM
leinir updated this revision to Diff 79704.Apr 9 2020, 12:46 PM
  • Remove a stray newline
ngraham added a subscriber: ngraham.Apr 9 2020, 4:38 PM

Even with this patch, every attempt to uninstall a newly-installed global theme still fails with:

The uninstallation process failed to successfully run the command kpackagetool5 -t Plasma/LookAndFeel -r /tmp/01-com.github.vinceliuice.McMojave.tar.xz

And every attempt to uninstall an already-installed global theme fails with:

The uninstallation process failed to successfully run the command kpackagetool5 -t Plasma/LookAndFeel -r /tmp/Sweet.tar.xz

And every attempt to install an already-secretly-installed global theme fails with:

Error: Installation of /tmp/Sweet.tar.xz failed: /home/nate/.local/share/plasma/look-and-feel/Sweet already exists

And clicking on the install/uninstall button still incorrectly changes the button to its other state despire the global theme not being correctly installed or uninstalled.

Am I not testing properly?

leinir added a comment.Apr 9 2020, 4:41 PM
In D28701#644943, @ngraham wrote:

Am I not testing properly?

Correct, that's the bit where i've not added instructions as required for switching to using the kpackage support. Short version, comment out the uninstall and install commands, and add an Uncompress=kpackage line, in the lookandfeel.knsrc file :) (also considering perhaps doing some smart lookup type stuff, and switching things which call kpackagetool automatically to using this, but also am not entirely certain if there might not be some fallout from that... not sure, will need to look at that)

ngraham added a comment.Apr 9 2020, 5:14 PM

Okay thanks, can you update the test plan then? Also I think the diff currently does not actually fix 418466.

leinir added a comment.Apr 14 2020, 8:30 AM
In D28701#644973, @ngraham wrote:

Okay thanks, can you update the test plan then? Also I think the diff currently does not actually fix 418466.

Your comments (and adding on reviewers) seems to suggest to me that you either haven't noticed the [WIP] at the start of the diff's title... or that i'm marking work in progress patches incorrectly? (and to answer the question, i will of course be adding testing instructions)

leinir updated this revision to Diff 80098.Apr 14 2020, 1:29 PM

Bunch of new functionality, with a fallback which ought to allow it to work
without requiring any kind of changes to the existing knsrc files based on
kpackage, as long as they follow the pattern they seem to mostly follow.

  • Introduce a getter (and enum) for the uncompress Installation setting
  • Add a redirection to the knsrc documentation location
  • Add a function to clean the cache of functionally stale entries
  • Clean the cache when the uncompression method is set to kpackage
  • Add a fallback for uncinverted kpackage based knsrc files
leinir edited the summary of this revision. (Show Details)Apr 14 2020, 1:33 PM
leinir edited the summary of this revision. (Show Details)
leinir updated this revision to Diff 80099.Apr 14 2020, 1:37 PM
  • Make a touch of noise when encountering the fallback
leinir edited the test plan for this revision. (Show Details)Apr 14 2020, 1:54 PM
leinir edited the test plan for this revision. (Show Details)
leinir updated this revision to Diff 80184.Apr 15 2020, 10:33 AM
leinir edited the test plan for this revision. (Show Details)

(the fallback handling needs some more work, but also progress)

  • Clean up some of the error reporting, and reset the entry's state
  • Check if installedFile is a file, if so bypass KPackage and delete
leinir updated this revision to Diff 80195.Apr 15 2020, 1:30 PM

Think we're at the point where testing would be good, now. This update
means we now attempt to adopt already installed kpackages if you try and
install the package from knewstuff, and removal of entries installed
using the previous implementation should now also happen during a fallback
step, intended to make life a bit simpler for those who have used this
before...

  • Add a KPackageType property to Installation, for fallback purposes
  • Add documentation for the new knsrc bits
  • Handle adopting an already installed kpackage item
  • Also uninstall not-adopted-but-there possibly installed kpackage bits
leinir retitled this revision from [WIP] Add KPackage support to KNewStuffCore to Add KPackage support to KNewStuffCore.Apr 15 2020, 1:33 PM
leinir edited the summary of this revision. (Show Details)
leinir added reviewers: ngraham, mart, davidedmundson.Apr 15 2020, 2:01 PM

Tagging in those active in the referenced bug, except for the reporter who doesn't have a phabricator account

ngraham added a comment.EditedApr 15 2020, 3:11 PM

This has fixed the issues I was seeing with installation and uninstallation, nice! However I'm now seeing a very long hang when installing certain global themes--for example Sweet KDE and Layan Look and Feel Theme. It does ultimately work, but the dialog freezes for multiple minutes before finally becoming interactive again.

src/core/installation.cpp
102

Oof.

Is there really no way around this? I mean, it did more or less work before the new QtQuick dialog was rolled out.

leinir added a comment.Apr 16 2020, 7:51 AM
In D28701#649012, @ngraham wrote:

This has fixed the issues I was seeing with installation and uninstallation, nice! However I'm not seeing a very long hang when installing certain global themes--for example Sweet KDE and Layan Look and Feel Theme. It does ultimately work, but the dialog freezes for multiple minutes before finally becoming interactive again.

That freeze really is amazingly annoying, and i'm afraid the only reasonable way to fix that is to move the installer into its own thread... which /should/ work ok, but i'm super scared of threads at the best of times ;) Needs must, though.

src/core/installation.cpp
102

Going by the code (in kpackagetool and here), i don't see how it could've really ever worked the way it looked before - i feel like what happened previously was that it /looked/ like it sort of worked, because installation did, but uninstallation never worked...

The extreme case described in this comment is just that, though - it turns out that unless you clean your tmp dir regularly, you're likely to actually have those downloaded files available, which will cause the items to remain marked as installed, which in turn allows the fallback path in the uninstall step below here to kick in, which will both remove the downloaded file, and also (if the file is identified as a kpackage and we can do a reverse lookup match on an installed kpackage with that type and name) remove that package with kpackage. So... there's a lot of bits in here to try and ensure people won't notice that everything really was very badly broken with kpackages previously, and that it now just sort of works :)

Further, a detail i noticed earlier, is that the uninstall call would just outright mark the entry you asked to uninstall as deleted, whether or not that was actually successful (which incidentally may happen and is there to allow some manual cleaning up if they've just deleted stuff underneath kns). I've moved that around just a touch, so that it doesn't happen if a kpackage failed to be uninstalled.

leinir updated this revision to Diff 80289.Apr 16 2020, 3:24 PM

Address @ngraham's (and my own) worry about the synchronous behaviour exhibited by KPackage... Something a bit like this probably wants to go into KPackage itself, perhaps we can consider this after we've done a bit of testing of its solidity here...

  • Add a simple async job wrapper for KPackage operations
  • Add the KPackage job to the build
  • Switch KNSCore::Installation to using the new async KPackageJob
leinir edited the summary of this revision. (Show Details)Apr 16 2020, 3:25 PM
leinir added reviewers: broulik, bshah.Apr 17 2020, 10:38 AM

Tagging in a couple of people who were in the original chat about doing this integration... :)

ngraham added a comment.Apr 17 2020, 5:38 PM

Thanks, the hang is gone now. However I have a new problem:

  1. Open Global Themes KCM and click new new global themes
  2. Install Sweet KDE and McMojave LAF global themes
  3. Close the GHNS dialog
  4. Open the dialog again
  5. Uninstall the McMojave LAF theme
  6. Close the dialog

Both McMojave LAF and also Sweet KDE have disappeared from the KCM. Sweet KDE still shows up as installed in the GHNS dialog and Discover, but does not appear in the KCM.

The same thing happens for plasma themes too. It seems that deleting one item from the GHNS dialog marks all of the installed GHNS items of that type as deleted.

leinir added a comment.Apr 17 2020, 5:46 PM
In D28701#650616, @ngraham wrote:

Thanks, the hang is gone now. However I have a new problem:

  1. Open Global Themes KCM and click new new global themes
  2. Install Sweet KDE and McMojave LAF global themes
  3. Close the GHNS dialog
  4. Open the dialog again
  5. Uninstall the McMojave LAF theme
  6. Close the dialog

    Both McMojave LAF and also Sweet KDE have disappeared from the KCM. Sweet KDE still shows up as installed in the GHNS dialog and Discover, but does not appear in the KCM.

    The same thing happens for plasma themes too. It seems that deleting one item from the GHNS dialog marks all of the installed GHNS items of that type as deleted.

i can confirm this, and looking at it a bit with some debug output i have a suspicion that i might be holding some part of kpackage's api incorrectly... Perhaps someone with a longer experience with KPackage than me can tell me whether that is the case perhaps?

org.kde.knewstuff.core: Install:  "Sweet KDE"  from  "/tmp/AfOprO-Sweet.tar.xz"
org.kde.knewstuff.core: installdir:  "/tmp"
org.kde.knewstuff.core: Using KPackage for installation
org.kde.knewstuff.core: Package metadata is valid
org.kde.knewstuff.core: Service type discovered as "Plasma/Theme"
org.kde.knewstuff.core: About to attempt to install "Sweet" into "/home/leinir/.local/share/plasma/desktoptheme/"
org.kde.knewstuff.core: Attempting to perform an installation operation of type 2 on the package "/tmp/AfOprO-Sweet.tar.xz" of type "Plasma/Theme" in the package root "/home/leinir/.local/share/plasma/desktoptheme/"
org.kde.knewstuff.core: Service type understood
org.kde.knewstuff.core: Installer successfully created and has a valid structure
kf5.kpackage: Generated  "/home/leinir/.local/share/plasma/desktoptheme//kpluginindex.json"  ( 2  plugins)
org.kde.knewstuff.core: Created job, now let's wait for it to do its thing...
org.kde.knewstuff.core: Install job finished with no error and we now have files "/home/leinir/.local/share/plasma/desktoptheme/Sweet"
org.kde.knewstuff.core: Write registry
org.kde.knewstuff.core: about to uninstall entry  "1294174"
org.kde.knewstuff.core: Attempting to perform an installation operation of type 3 on the package "/home/leinir/.local/share/plasma/desktoptheme/Sweet" of type "Plasma/Theme" in the package root "/home/leinir/.local/share/plasma/desktoptheme/"
org.kde.knewstuff.core: Service type understood
org.kde.knewstuff.core: Installer successfully created and has a valid structure
org.kde.knewstuff.core: Created job, now let's wait for it to do its thing...
org.kde.knewstuff.core: Write registry
org.kde.knewstuff.core: about to uninstall entry  "1305006"
org.kde.knewstuff.core: Write registry
leinir added a comment.Apr 20 2020, 8:07 AM

As i thought, i was indeed holding the KPackage APi incorrectly ;) The culprit is hinted at in the line

org.kde.knewstuff.core: Attempting to perform an installation operation of type 3 on the package "/home/leinir/.local/share/plasma/desktoptheme/Sweet" of type "Plasma/Theme" in the package root "/home/leinir/.local/share/plasma/desktoptheme/"

which says that it is installing a package, which has the full path, and not only the package name. The kpackage api expects a package name, and so handing it a directory name is not as helpful. So, delightfully simple fix, really, but yeah, annoying on a Friday night while cooking dinner ;) Updated patch incoming momentarily...

leinir updated this revision to Diff 80617.Apr 20 2020, 8:11 AM

Thank you to @ngraham for noticing this one! It only really pokes its head
out if you have multiple things installed and then try and uninstall one of
them - if you only have the one thing installed, it looks very much like
as though it's just cleaning up after itself and removing the containing
folder... Easy enough fix, though :)

  • Make sure we use package name, not install location, when uninstalling
ngraham added a comment.Apr 20 2020, 6:21 PM

Great. There are still a few more bugs though:

When you install certain global themes, they ask for authentication so install an SDDM theme. However when you uninstall that theme, it doesn't request authentication to remove them SDDM theme. So /usr/share/sddm/themes accumulates a growing collection of unused themes:

ls /usr/share/sddm/themes/
breeze  breeze-openSUSE  elarun  Layan  maldives  maya  McMojave  plasmaX  Sweet

When I install and uninstall the Sweet global theme, its Plasma theme still shows up in the Plasma style KCM. And looking in ~/.local/share/plasma/desktoptheme/, there are several orphaned plasma themes left over from global themes that I deleted from the GHNS dialog on the global themes KCM:

ls ~/.local/share/plasma/desktoptheme/
Arc-Dark  kpluginindex.json  Layan  Sweet

Their color schemes and icon themes are still there too.

leinir added a comment.Apr 21 2020, 9:39 AM
In D28701#652898, @ngraham wrote:

Great. There are still a few more bugs though:

When you install certain global themes, they ask for authentication so install an SDDM theme. However when you uninstall that theme, it doesn't request authentication to remove them SDDM theme. So /usr/share/sddm/themes accumulates a growing collection of unused themes:

ls /usr/share/sddm/themes/
breeze  breeze-openSUSE  elarun  Layan  maldives  maya  McMojave  plasmaX  Sweet

Not a huge amount knewstuff can do about that, that'll need to be done by the sddm kpackage plugin (mind you, having not looked i don't imagine this would be a huge issue, more a bit of forgotten implementation work in said plugin, since it already has the logic to ask on installation i don't imagine it would be a huge amount of effort to get it to do that on uninstallation as well).

When I install and uninstall the Sweet global theme, its Plasma theme still shows up in the Plasma style KCM. And looking in ~/.local/share/plasma/desktoptheme/, there are several orphaned plasma themes left over from global themes that I deleted from the GHNS dialog on the global themes KCM:

ls ~/.local/share/plasma/desktoptheme/
Arc-Dark  kpluginindex.json  Layan  Sweet

Their color schemes and icon themes are still there too.

i can confirm this is happening, yup - however, this isn't knewstuff specifically, this is kpackage in general (you will notice this with any kpackage which has dependencies). I would suggest it is out of the scope for this specific patch to fix that problem, but it's definitely something we'll want to sort out (but also, if you have ever run apt-get purge or autoremove, that's the territory we're veering into here... that is to say, doable, but not trivial or automatically doable, as there's no guarantee that nothing else depends on that package being there).

Now, what we /could/ do is allow the user to just go "yup, please remove all the dependencies as well" for packages which have dependencies listed, but i am thinking that that is starting to sound quite dangerous... As in, what happens when a user goes "yes, i definitely know what i'm doing" and clicks that option, only to then discover that no, those dependencies were also needed by some other thing (i imagine a fair few global themes would have similar dependencies) and now that other thing just outright breaks when attempting to apply it. Again, this should hopefully be solvable, but i'm not sure how much work that would be. Arguably in scope here if we want to ask, but it's not a regression (by which i mean kpackage -r Sweet -t Plasma/LookAndFeel would also leave those other bits behind).

ngraham added a comment.Apr 21 2020, 5:40 PM

I mean, GHNS basically *is* a userspace package manager, and things like dependency management and removal of stale files are why package managers exist. :)

If it needs to be fixed/implemented in other patches, that's okay.

leinir added a comment.Apr 21 2020, 6:51 PM
In D28701#653954, @ngraham wrote:

I mean, GHNS basically *is* a userspace package manager, and things like dependency management and removal of stale files are why package managers exist. :)

If it needs to be fixed/implemented in other patches, that's okay.

Definitely, yes, it wants implementing, though i think we need to consider precisely where we want it to go... Thinking it probably wants to go in KPackage itself for this case (since that's what installs those dependencies - it's at the end of the packagejobthread's installPackage function, but i'm thinking the optional removal thing likely needs to go into the packagejob, possibly as an overload... just brainfarting a bit here), but there's also a dependencies and referencing type thing in OCS (and consequently KNewStuff) which we'll need to implement more... properly than it is right now. But yup, that's sort of orthogonal to this patch anyway, more a "this wants to be done" type thing :)

leinir updated this revision to Diff 80862.Apr 22 2020, 10:28 AM

Some documentation and whitespace fixes for Frameworksiness

  • Fix up a few whitespace errors
  • Add some documentation to KPackageJob, and a few @since
alex added a subscriber: alex.Apr 24 2020, 9:52 AM
mart added inline comments.Apr 24 2020, 2:33 PM
src/core/jobs/kpackagejob.cpp
55 ↗(On Diff #80862)

is it worth encapsulating it in a runnable? the installation of a package is already in a separathe thread.

97 ↗(On Diff #80862)

this would freeze a thread waiting another thread is done, while just using the job in an async wayshouldn't be much more heavy on the gui thread?

leinir added inline comments.Apr 24 2020, 2:40 PM
src/core/jobs/kpackagejob.cpp
55 ↗(On Diff #80862)

Annoyingly, the caller waits for that thread to complete, so the KPackage internals work synchronously, and block the UI thread... (which is how we ended up doing this)

97 ↗(On Diff #80862)

The Runner doesn't inherently have an event loop, so without this, the job just sort of... sits there and does nothing (doesn't block, but also doesn't do anything)

leinir added a comment.Apr 28 2020, 8:31 AM

Ping team framework and such? (i realise we're all a tiny bit more stressy than usual...)

mart added a comment.Apr 30 2020, 8:11 AM

+1 from me.
does it need to go into next release or can also get in next+1?

leinir added a comment.Apr 30 2020, 8:14 AM
In D28701#660186, @mart wrote:

+1 from me.

Thanks! :)

does it need to go into next release or can also get in next+1?

It really wants to go in soon as possible (since it fixes the whole global themes nastiness that people have been reporting - transparently, of course, what with the fallback sniffing going on in the ctor), same as the update fix in D29222

leinir updated this revision to Diff 81859.May 4 2020, 9:57 AM

Since we have had a new Frameworks release while this is waiting
for the thumbs up, increase the @since to the next version.

  • Merge branch 'master' into add-kpackage-support
  • Merge branch 'master' into add-kpackage-support
  • I was trying to avoid this. Update @since to 0.71
mart accepted this revision.May 4 2020, 10:29 AM
This revision is now accepted and ready to land.May 4 2020, 10:29 AM
Closed by commit R304:3f38da8a70d8: Add KPackage support to KNewStuffCore (authored by leinir). · Explain WhyMay 4 2020, 10:45 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
MCMakeLists.txt (1 line)
Msrc/core/CMakeLists.txt (1 line)
Msrc/core/cache.cpp (3 lines)
Msrc/core/installation.cpp (477 lines)

Diff 79703

View Options

CMakeLists.txt

Loading...
View Options

src/core/CMakeLists.txt

Loading...
View Options

src/core/cache.cpp

Loading...
View Options

src/core/installation.cpp

Loading...