Bug 393255

*** Bug 393256 has been marked as a duplicate of this bug. ***

jgrulich, I added you as export of nm+Qt integration :)

There is also a related bug for kde-runtime (https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=862883); we don't release it anymore, but I suspect that the fix for kdelibs4support could be used easily there too).

See also the reply from Lubomir Rintel, one of NetworkManagers upstream developers:

Hi,

Note that the NetworkManager check seems broken too: the
-DNM_BACKEND_ENABLED is never actually enabled because "NM_0_7" is
never set.

Enabling the NetworkManager backend reveals there's actually a syntax
error (MOC stuff copy & pasted into C++?) in the NetworkManager
support.

This is what it takes to make it actually work (not actually submitted
upstream since it's not clear to me whether it makes sense):

https://github.com/NetworkManager/kdelibs4support/commit/3d88ab2b785ed372eb43d21d55a3ddb23695cd5e

Perhaps just dropping the Build-Depends, disabling the futile attempt
to use NetworkManager would be a better option than bothering with a
patch?

Lubo

(In reply to mbiebl from comment #3)
> Perhaps just dropping the Build-Depends, disabling the futile attempt
> to use NetworkManager would be a better option than bothering with a
> patch?

That's my current plan. As soon as kdesignplugin leaves NEW, I'll upload 5.45, which drops the networkmanager build dependency.

Happy hacking,

(In reply to Maximiliano Curia from comment #4)
> (In reply to mbiebl from comment #3)
> > Perhaps just dropping the Build-Depends, disabling the futile attempt
> > to use NetworkManager would be a better option than bothering with a
> > patch?
> 
> That's my current plan. As soon as kdesignplugin leaves NEW, I'll upload
> 5.45, which drops the networkmanager build dependency.

I guess it would still make sense to fix this code upstream. If that is not going to happen, maybe it 
(In reply to Maximiliano Curia from comment #4)
> (In reply to mbiebl from comment #3)
> > Perhaps just dropping the Build-Depends, disabling the futile attempt
> > to use NetworkManager would be a better option than bothering with a
> > patch?
> 
> That's my current plan. As soon as kdesignplugin leaves NEW, I'll upload
> 5.45, which drops the networkmanager build dependency.

As lubo correctly noted, due to the bug in build system, the networkmanagerstatus.cpp code was never compiled, as NM_0_7 was never set, even if NetworkManager.pc was found.

So the question is, whether to rip out the NetworkManager code completely (upstream) or fix it. 

For the latter, I've taken lubo's initial patch and slighly modified it.

Created attachment 112102 [details]
networkmanagerstatus: port to libnm

Created attachment 112105 [details]
[PATCH] networkmanagerstatus: port to libnm

Replace NetworkManager.h include with nm-dbus-interface.h to not pull in any glib/gio related headers.

(In reply to mbiebl from comment #5)
> (In reply to Maximiliano Curia from comment #4)
> > (In reply to mbiebl from comment #3)
> > > Perhaps just dropping the Build-Depends, disabling the futile attempt
> > > to use NetworkManager would be a better option than bothering with a
> > > patch?

> > That's my current plan. As soon as kdesignplugin leaves NEW, I'll upload
> > 5.45, which drops the networkmanager build dependency.

> I guess it would still make sense to fix this code upstream. If that is not
> going to happen, maybe it 
> (In reply to Maximiliano Curia from comment #4)
> > (In reply to mbiebl from comment #3)
> > > Perhaps just dropping the Build-Depends, disabling the futile attempt
> > > to use NetworkManager would be a better option than bothering with a
> > > patch?

> > That's my current plan. As soon as kdesignplugin leaves NEW, I'll upload
> > 5.45, which drops the networkmanager build dependency.
> 
> As lubo correctly noted, due to the bug in build system, the
> networkmanagerstatus.cpp code was never compiled, as NM_0_7 was never set,
> even if NetworkManager.pc was found.

> So the question is, whether to rip out the NetworkManager code completely
> (upstream) or fix it. 

Well kdelibs4support is a porting aid, a library that provides some interfaces that were available in kde4 but are not part of kde frameworks 5 (they have a suitable replacement in the kf5 family but it might not always be trivial to migrate to), as such it's highly disrecommended to use it. So, I wouldn't bet on upstream fixing, or even accepting a patch to fix, something that was always broken.

It might make more sense to drop the dead code (as long as it doesn't break any api).


Happy hacking,

(In reply to Maximiliano Curia from comment #8)
> Well kdelibs4support is a porting aid, a library that provides some
> interfaces that were available in kde4 but are not part of kde frameworks 5
> (they have a suitable replacement in the kf5 family but it might not always
> be trivial to migrate to), as such it's highly disrecommended to use it. So,
> I wouldn't bet on upstream fixing, or even accepting a patch to fix,
> something that was always broken.

To clarify: The build was broken in kdelibs4support, it is not broken in kde-runtime.

> It might make more sense to drop the dead code (as long as it doesn't break
> any api).

I don't know KDE too well, so I don't know which KF5 programs still rely on the functionality of the networkstatus kded module or if it's safe to drop it. If you think dropping the functionality is the way to go, do you think the same should be applied to kde-runtime?

(In reply to mbiebl from comment #9)
> (In reply to Maximiliano Curia from comment #8)
> > Well kdelibs4support is a porting aid, a library that provides some
> > interfaces that were available in kde4 but are not part of kde frameworks 5
> > (they have a suitable replacement in the kf5 family but it might not always
> > be trivial to migrate to), as such it's highly disrecommended to use it. So,
> > I wouldn't bet on upstream fixing, or even accepting a patch to fix,
> > something that was always broken.
 
> To clarify: The build was broken in kdelibs4support, it is not broken in
> kde-runtime.

Yeah, the NM_0_7 define is present in kde-runtime.
 
> > It might make more sense to drop the dead code (as long as it doesn't break
> > any api).
 
> I don't know KDE too well, so I don't know which KF5 programs still rely on
> the functionality of the networkstatus kded module or if it's safe to drop
> it. If you think dropping the functionality is the way to go, do you think
> the same should be applied to kde-runtime?

kde-runtime is no longer being maintained by kde.org, the repository is still available, but that's it, so the patch won't be applied upstream. On the Debian side, we don't really know if somebody uses the networkstatus kded module, a quick search in codesearch showed no results, but the idea is to drop kde-runtime (and qt4 for that matter) for buster, so disabling an optional feature should be fine.

Anyway, you might want to forward this patch or the one that drops the use of network manager from kdelibs4support to the codereview system: https://phabricator.kde.org

Upstream usually has a lower latency when a code review is requested.

Happy hacking,

(In reply to Maximiliano Curia from comment #10)
> Anyway, you might want to forward this patch or the one that drops the use
> of network manager from kdelibs4support to the codereview system:
> https://phabricator.kde.org

This means creating yet another user account (it seems the user account I created for bugs.kde.org is not accepted for phabricator.kde.org).

Maxy, if you think this should be forwarded to phabricator.kde.org, would you mind doing that?

(In reply to mbiebl from comment #11)
> (In reply to Maximiliano Curia from comment #10)
> > Anyway, you might want to forward this patch or the one that drops the use
> > of network manager from kdelibs4support to the codereview system:
> > https://phabricator.kde.org

> This means creating yet another user account (it seems the user account I
> created for bugs.kde.org is not accepted for phabricator.kde.org).

> Maxy, if you think this should be forwarded to phabricator.kde.org, would
> you mind doing that?

If I forward the patch, then I'll be playing ball for every change requested upstream. And I think that the change should drop the code that was not being used. Anyway, it's way better if you can be the one responding the review.

The documentation on how to deal with phabricator is here:
 - https://community.kde.org/Infrastructure/Phabricator

Patch available at: https://phabricator.kde.org/D12378 (Jan, can you please add a reference to the bug there?)

(In reply to Luigi Toscano from comment #13)
> Patch available at: https://phabricator.kde.org/D12378 (Jan, can you please
> add a reference to the bug there?)

@Jan: I think the patch I attached to this bug report is slightly better then the one at https://phabricator.kde.org/D12378
- The cmake module talks about libnm, which reflects the name of the pc file
- It uses #include <nm-dbus-interface.h>, which avoid any issues with glib/gio and does not require any signals trickery.

While looking at the patch in phabricator, I also noticed that you used 
#include <libnm/NetworkManager.h>

You should drop the libnm/ prefix, as the include path is already set by pkg-config.

Cflags: -I${includedir}/libnm

So, say you have install libnm to an unusual prefix, using the linm/ prefix would mean the header is no longer found

I updated the review with your comments.

kdelibs4support was already in pure maintenance mode in kf5, for kf6 it got removed,
there will be no future work spend on this beside critical security issues, if
at all.