Dolphin: Implement package kit for deb/rpm/pacman service packages
ClosedPublic

Authored by alex on Apr 23 2020, 5:25 AM.

Details

Summary

The deb/rpm/pacman packages are now installed/uninstalled using packagekit.

Test Plan

Try to install deb package from kde store (search for jetbrains). Then uninstall it.

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
meven added inline comments.Apr 24 2020, 6:32 AM
src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
90

Indentation seems of :/ way soo spacey

alex added inline comments.Apr 24 2020, 6:36 AM
src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
90

How would it be better? Maybe define the lambdas before?

meven added inline comments.Apr 24 2020, 6:38 AM
src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
90

That's a solution, or adding a function.

alex updated this revision to Diff 81068.Apr 24 2020, 6:57 AM

Define lambdas for uninstall before using them

meven added a comment.Apr 24 2020, 1:43 PM

You can mark what you did as done.

Seems to good me

Let's wait a little to have @elvisangelaccio review this.

alex marked 4 inline comments as done.Apr 24 2020, 2:59 PM

Does PackageKit work on Windows/MacOS? I don't think so. We should probably run this code only on Unix, no?

Does PackageKit work on Windows/MacOS? I don't think so. We should probably run this code only on Unix, no?

PackageKit is not a thing on Gentoo either.

alex updated this revision to Diff 81292.Apr 27 2020, 5:39 AM

Make PackageKit optional

alex updated this revision to Diff 81295.Apr 27 2020, 6:04 AM

target_link_libraries only if packagekit is available

anthonyfieroni added inline comments.
src/settings/services/servicemenuinstaller/CMakeLists.txt
9

It should be optional.

src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
383

Why limited to these 2? Pacman packages should be included as well.

alex added inline comments.Apr 27 2020, 6:24 AM
src/settings/services/servicemenuinstaller/CMakeLists.txt
9

Does this comment still apply after the latest update to the diff?

anthonyfieroni added inline comments.Apr 27 2020, 6:28 AM
src/settings/services/servicemenuinstaller/CMakeLists.txt
9

It's ok now.

alex updated this revision to Diff 81297.Apr 27 2020, 6:33 AM

Make list of binaryPackages static

alex added inline comments.Apr 27 2020, 6:46 AM
src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
383

I choose only these two because they are very common package formats and quite a few services in the store already have deb/rpm packages available.

But I am unsure on how to check the mimetype for pacman packages, would it be enough to check for application/x-xz?

anthonyfieroni added inline comments.Apr 27 2020, 7:25 AM
src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
383
alex updated this revision to Diff 81311.Apr 27 2020, 8:44 AM

Add mimetypes for pacman packages

alex marked 6 inline comments as done.Apr 27 2020, 8:46 AM
src/settings/services/servicemenuinstaller/CMakeLists.txt
5

Unrelated change, keep private linking.

src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
34

const, QStringLiteral to strings.

80

Installation process may fail due to unsatisfied depends can we get more info about?

You can rename summary to inform that Debian, RPM and Pacman packages are supported.

alex updated this revision to Diff 81328.Apr 27 2020, 10:41 AM
alex retitled this revision from Dolphin: Implement package kit for deb/rpm service packages to Dolphin: Implement package kit for deb/rpm/pacman service packages.
alex edited the summary of this revision. (Show Details)
alex edited the test plan for this revision. (Show Details)

Display error details instead of status code

If you uninstall a package that has already been uninstalled:
"Could not find package(s)"
Missing dependencies:
"The following packages have unmet dependencies:\n dolphin-jetbrainsservicemenu: Depends: kiooooo (>= 5.44) but it is not installable\n"

alex marked 3 inline comments as done.Apr 27 2020, 10:41 AM

+1, well done. Wait ship it from maintainer.

alex added a comment.Apr 27 2020, 10:52 AM

And thanks for the feedback :-).

I get a CMake error:

CMake Error at src/settings/services/servicemenuinstaller/CMakeLists.txt:11 (target_link_libraries):
  The keyword signature for target_link_libraries has already been used with
  the target "servicemenuinstaller".  All uses of target_link_libraries with
  a target must be either all-keyword or all-plain.

  The uses of the keyword signature are here:

   * src/settings/services/servicemenuinstaller/CMakeLists.txt:5 (target_link_libraries)
anthonyfieroni added inline comments.Apr 27 2020, 3:07 PM
src/settings/services/servicemenuinstaller/CMakeLists.txt
12

Add PRIVATE as well here.

cblack added a subscriber: cblack.Apr 27 2020, 3:31 PM
cblack added inline comments.
src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
34

This should include application/x-bzip2 as well. Also, it would probably be optimal to query what PackageKit backend is currently being used and set associations based on that.

alex added inline comments.Apr 27 2020, 4:13 PM
src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
34

qWarning() << "Mime types:" << PackageKit::Daemon::mimeTypes(); returns an empty list, I don't know why.

And application/x-bzip2 is already in the list of file formats used for decompression. (Line 150).

alex updated this revision to Diff 81362.Apr 27 2020, 4:15 PM

Make cmake library private, fix cmake error

Installation for the JetBrains plugin claims to succeed, the my package manager says it's not actually installed, and indeed it doesn't appear in the plugins list in Dolphin. It is marked as installed in the GHNS dialog though, ans clicking "uninstall" displays and error message that is also not totally visible (which not your fault; this would need to be fixed in the GHNS dialog):

It seems like the installation did not in fact succeed, even though it claims to have.

What kind of GHNS dialog you spot about? It's a service menu, right click on supported file format (deb, rpm, zst)

alex added a comment.Apr 27 2020, 4:59 PM

I can't reproduce the install issue.
And it being marked as installed even if it is not installed (in the dolphin dialog)is another issue in KNS.
But the uninstall is generally broken, see D29101.

alex added a comment.Apr 27 2020, 5:00 PM

What kind of GHNS dialog you spot about? It's a service menu, right click on supported file format (deb, rpm, zst)

kcmshell5 kcmdolphinservices
And the the download new services button.

elvisangelaccio requested changes to this revision.May 3 2020, 6:20 PM
elvisangelaccio added inline comments.
CMakeLists.txt
87

Please call it HAVE_PACKAGEKIT instead.

88–91

Please create a config-packagekit.h.cmake file instead. See src/CMakeLists.txt as example:

configure_file(config-baloo.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/config-baloo.h)
configure_file(config-kactivities.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/config-kactivities.h)
src/settings/services/servicemenuinstaller/servicemenuinstaller.cpp
74

Please call it exitWithError() or similar instead, since this is what this lambda actually does.

89

Same here.

112

Same here: please don't use booleans as arguments.

115–116

Please use QFileInfo::exists() instead.

This revision now requires changes to proceed.May 3 2020, 6:20 PM
alex updated this revision to Diff 81817.May 3 2020, 6:49 PM

Implement requested changes

alex marked 9 inline comments as done.May 3 2020, 6:52 PM
elvisangelaccio accepted this revision.May 3 2020, 7:33 PM

Please fix the last two inline comments and then feel free to ship it.

CMakeLists.txt
88–90

Not needed, this is the default ;)

src/settings/services/servicemenuinstaller/CMakeLists.txt
11

Please move it to src/CMakeLists.txt

alex updated this revision to Diff 81834.May 4 2020, 5:08 AM

Fix last comments

And thanks for the feedback and review :-)

alex marked 2 inline comments as done.May 4 2020, 5:09 AM
meven accepted this revision.May 4 2020, 6:29 AM
This revision is now accepted and ready to land.May 4 2020, 6:29 AM
alex updated this revision to Diff 81890.EditedMay 4 2020, 2:00 PM

Make sure to exit

When you install for example an rpm package on KDE Neon with the servicemenu installer
you get the warning that the mime type is not supported. But not every time.
In that case finished is emitted but errorCode not.
Now there is a timer (500 ms) which exits after the packagekit
operation unsucessfully finished. In this time
the errorCode slot can be called and we can be sure that the installer will exit.

But now this patch should be done ;-)

Hmm, I'm still not seeing this work for me. PackageKit clearly trie to install it (as verified by pkmon) but the transaction still fails:

. Here's the pkmon log:
1daemon connected=1
2Transactions:
3 1 /2_acdbecda
4/2_acdbecda allow_cancel 1
5/2_acdbecda percentage -1
6/2_acdbecda role get-updates
7/2_acdbecda status setup
8/2_acdbecda status query
9/2_acdbecda percentage 0
10/2_acdbecda status refresh-cache
11/2_acdbecda percentage 8
12/2_acdbecda percentage 16
13daemon locked=1
14Transactions:
15 1 /2_acdbecda
16 2 /1_ddebcdeb
17/1_ddebcdeb allow_cancel 1
18/1_ddebcdeb percentage -1
19/1_ddebcdeb role install-files
20/1_ddebcdeb status wait
21/2_acdbecda percentage 25
22/2_acdbecda percentage 41
23/2_acdbecda percentage 50
24/2_acdbecda percentage 58
25/2_acdbecda percentage 66
26/2_acdbecda percentage 75
27/2_acdbecda percentage 83
28/2_acdbecda percentage 100
29/2_acdbecda percentage 40
30/2_acdbecda percentage 80
31/2_acdbecda percentage 100
32/2_acdbecda status finished
33/2_acdbecda exit code: success
34daemon locked=0
35Transactions:
36 1 /1_ddebcdeb
37/1_ddebcdeb status setup
38/1_ddebcdeb status dep-resolve
39/1_ddebcdeb percentage 0
40/1_ddebcdeb percentage 100
41/1_ddebcdeb status install
42/1_ddebcdeb percentage 0
43/1_ddebcdeb status download
44/1_ddebcdeb package-id dolphin-jetbrainsservicemenu;1.1.0-1;x86_64;PK_TMP_DIR
45/1_ddebcdeb package downloading:dolphin-jetbrainsservicemenu;1.1.0-1;x86_64;PK_TMP_DIR:Plugin to open folders in Dolphin with the Jetbrains IDEs
46/1_ddebcdeb status finished

However, the GHNS dialog still reports success.

alex added a comment.May 4 2020, 2:59 PM

@ngraham Does the implementation in kdenetwork-filesharing work for you?

alex added a comment.May 4 2020, 3:15 PM

:-(
Does it work if you install install the deb package using apt on the command line?

I'm on openSUSE, so I can try the RPM one by hand.

Ah, it fails doing it by hand too:

nate@spectre:~/kde/src/kirigami$  (master) sudo zypper install /home/nate/Desktop/dolphin-jetbrainsservicemenu-1.1.0-Linux.rpm 
Loading repository data...
Reading installed packages...
Resolving package dependencies...

The following NEW package is going to be installed:
  dolphin-jetbrainsservicemenu

1 new package to install.
Overall download size: 50.8 KiB. Already cached: 0 B. After the operation, additional
120.5 KiB will be used.
Continue? [y/n/v/...? shows all options] (y): y
Retrieving package dolphin-jetbrainsservicemenu-1.1.0-1.x86_64
                                                     (1/1),  50.8 KiB (120.5 KiB unpacked)
dolphin-jetbrainsservicemenu-1.1.0-Linux.rpm:
    Package is not signed!

dolphin-jetbrainsservicemenu-1.1.0-1.x86_64 (Plain RPM files cache): Signature verification failed [6-File is unsigned]
pkcon install-local /home/nate/Desktop/dolphin-jetbrainsservicemenu-1.1.0-Linux.rpm 
Installing files              [=========================]         
Testing changes               [=========================]         
Finished                      [                         ] (0%)  
The following packages have to be installed:
 dolphin-jetbrainsservicemenu-1.1.0-1.x86_64    Plugin to open folders in Dolphin with the Jetbrains IDEs
Proceed with changes? [N/y] y

                              [=========================]         
Installing files              [=========================]         
Waiting for authentication    [=========================]         
Starting                      [=========================]         
Resolving dependencies        [=========================]         
Installing packages           [=========================]         
Finished                      [=========================]         
Fatal error: Installation has been aborted as directed.
nate@spectre:~/kde/src/kirigami$  (master) echo $?
7

So packagekit doesn't give us the best error message in the world compared to the package manager, but it does appear to have one and be return a non-zero status, which we should be catching here if possible.

alex updated this revision to Diff 81994.May 5 2020, 2:58 PM

Show error message in kdialog

I was going to do this in a follow up patch but I guess it is ok here :-).

However, the GHNS dialog still reports success.

This is a problen in GHNS, I will write a patch for this.

ngraham accepted this revision.May 5 2020, 3:22 PM

Yay!

I wouldn't exactly call this error message useful though:

However that's PackageKit's fault, not yours. Shipit!

alex added a comment.May 5 2020, 3:24 PM

Awesome 😃

This revision was automatically updated to reflect the committed changes.