#include <dbus/dbus-glib.h>
in src/shell-network-agent.c should be dropped

Thanks for working on this, Lubomir!

With the dbus-glib.h header removed, gnome-shell 3.26.1 compiled fine on Debian sid. Then I tried to setup a new wireless network using the wireless menu of gnome-shell's user menu. This did not work. In the journal I got:

Nov 02 16:47:25 pluto gnome-shell[32414]: JS ERROR: Exception in callback for signal: activate: TypeError: this._device.request_scan_simple is not a function
                                          NMWirelessDialog<._onScanTimeout@resource:///org/gnome/shell/ui/status/network.js:787:9
                                          wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
                                          NMWirelessDialog<._init@resource:///org/gnome/shell/ui/status/network.js:745:9
                                          wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
                                          _Base.prototype._construct@resource:///org/gnome/gjs/modules/_legacy.js:18:5
                                          Class.prototype._construct/newClass@resource:///org/gnome/gjs/modules/_legacy.js:117:20
                                          NMDeviceWireless<._showDialog@resource:///org/gnome/shell/ui/status/network.js:1252:24
                                          wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
                                          PopupMenuBase<.addAction/<@resource:///org/gnome/shell/ui/popupMenu.js:480:13
                                          _emit@resource:///org/gnome/gjs/modules/signals.js:126:27
                                          PopupBaseMenuItem<.activate@resource:///org/gnome/shell/ui/popupMenu.js:166:9
                                          wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
                                          PopupBaseMenuItem<._onButtonReleaseEvent@resource:///org/gnome/shell/ui/popupMenu.js:127:9
                                          wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
Nov 02 16:47:40 pluto gnome-shell[32414]: JS ERROR: TypeError: this._device.request_scan_simple is not a function
                                          NMWirelessDialog<._onScanTimeout@resource:///org/gnome/shell/ui/status/network.js:787:9
                                          wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22

Created attachment 362841 [details]
journal

since bugzilla mangled the error message, I've also attached it as txt

I also see this in the journal:

Nov 02 17:02:08 pluto gnome-shell[32414]: g_object_get_is_valid_property: object class 'NMRemoteSettings' has no property named 'wireless-hardware-enabled'
N

Created attachment 362850 [details]
[PATCH v2] network: port to libnm

(In reply to Michael Biebl from comment #1)
> #include <dbus/dbus-glib.h>
> in src/shell-network-agent.c should be dropped

Thanks, fixed.

(In reply to Michael Biebl from comment #2)
> Nov 02 16:47:25 pluto gnome-shell[32414]: JS ERROR: Exception in callback
> for signal: activate: TypeError: this._device.request_scan_simple is not a
> function

Should be fixed too. (Tested too)

(In reply to Michael Biebl from comment #5)
> I also see this in the journal:
> 
> Nov 02 17:02:08 pluto gnome-shell[32414]: g_object_get_is_valid_property:
> object class 'NMRemoteSettings' has no property named
> 'wireless-hardware-enabled'

This is a libnm bug, fixed in:
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=nm-1-8&id=34035ceee

IIRC, libnm will abort when used together with libnm-glib. That means that extensions that still use the old API will crash the session, as will ported extensions that are loaded into an older shell.

So ideally we'd come up with a mechanism to make those imports fail (i.e. throw an exception) before making the change. I'm not sure off-hand whether this can be achieved entirely in GIRepository (make g_irepository_require() fail) or need gjs changes as well. Looking into this has been on my TODO list for a while, but so far I haven't found the time :-(

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

(In reply to Florian Müllner from comment #7)
> IIRC, libnm will abort when used together with libnm-glib. That means that
> extensions that still use the old API will crash the session, as will ported
> extensions that are loaded into an older shell.

Yes, indeed. What makes this worse is that this happens via an abort() so is not caught by gnome-shell, and there is thus no fail-whale which would disable the faulty extensions.
 
> So ideally we'd come up with a mechanism to make those imports fail (i.e.
> throw an exception) before making the change. I'm not sure off-hand whether
> this can be achieved entirely in GIRepository (make g_irepository_require()
> fail) or need gjs changes as well. Looking into this has been on my TODO
> list for a while, but so far I haven't found the time :-(

That's an interesting idea.
Lubomir has been grepped through the list of extensions at extensions.gnome.org and we found 9 extensions which use the old libnm-glib based typelibs:

banshee-gnome-shell-doubanFM-indicator@mengzhuo.org.v10
disconnect-wifi@kgshank.net.v16
docker-integration@jan.trejbal.gmail.com.v5
gnome-stats-pro@thrallingpenguin.com.v3
NetMonitor@zdyb.tk.v2
netspeed@hedayaty.gmail.com.v27
show-ip@kyle.aims.ac.za.v1
show-ip@sgaraud.github.com.v8
system-monitor@paradoxxx.zero.gmail.com.v32

That list is not that long. We could just have a (versioned) blacklist in gnome-shell assuming this is easier to implement.

(In reply to Lubomir Rintel from comment #6)

> (In reply to Michael Biebl from comment #2)
> > Nov 02 16:47:25 pluto gnome-shell[32414]: JS ERROR: Exception in callback
> > for signal: activate: TypeError: this._device.request_scan_simple is not a
> > function
> 
> Should be fixed too. (Tested too)

I'm getting a bit further when trying to create a wireless connection with gnome-shell. Now the dialog which shows the available networks pops up, but trying to select one does not show the prompt for a passphrase. Instead I get a failure message in the journal (attached as error2.txt)

Created attachment 362851 [details]
error when trying to connect to wifi

Created attachment 362916 [details]
[PATCH v3] network: port to libnm

(In reply to Michael Biebl from comment #11)
> Created attachment 362851 [details]
> error when trying to connect to wifi

Thanks for this. Should be fixed in the new patch.

(In reply to Florian Müllner from comment #7)
> IIRC, libnm will abort when used together with libnm-glib. That means that
> extensions that still use the old API will crash the session, as will ported
> extensions that are loaded into an older shell.
> 
> So ideally we'd come up with a mechanism to make those imports fail (i.e.
> throw an exception) before making the change. I'm not sure off-hand whether
> this can be achieved entirely in GIRepository (make g_irepository_require()
> fail) or need gjs changes as well. Looking into this has been on my TODO
> list for a while, but so far I haven't found the time :-(

Well, on #introspection I was told that a change that would add conflict of expressing conflicts in typelibs is a no-go. Alternative way of solving this would be a shim library that dlopen the real libnm-glib only if it's a safe thing to do. That would avoid a crash and make further dlsym()s just return NULL, making it possible to turn it into a Javascript exception along the way.

So much for avoiding a crash. As for actually not breaking the extensions, this is what I've come up so far:

https://github.com/NetworkManager/gnome-extension-show-ip/commit/b8e6c79f.patch

(In reply to Lubomir Rintel from comment #12)
> Created attachment 362916 [details]
> [PATCH v3] network: port to libnm
> 
> (In reply to Michael Biebl from comment #11)
> > Created attachment 362851 [details]
> > error when trying to connect to wifi
> 
> Thanks for this. Should be fixed in the new patch.

Thanks, v3 works much better! I did some testing of basic functionality, like creating new wifi connections, and all works fine so far.

Created attachment 362984 [details]
[PATCH v4] network: port to libnm

(In reply to Michael Biebl from comment #13)
> Thanks, v3 works much better! I did some testing of basic functionality,
> like creating new wifi connections, and all works fine so far.

I've done some more testing and discovered a couple more bugs. Updating the patch.

Here's my notes about the extensions:
https://public.etherpad-mozilla.org/p/libnm-shell-extensions

Feel free to update it with some more notes (e.g. which of them are included in Debian -- I've lost the link you've shared on the IRC).

Unless we'll discover a better way to detect whether libnm-glib is already loaded, I'll go ahead and send the patches to the respective extension maintainers in a few days.

@lubko: updated the pad, for completeness sake, Debian ships the following affected extensions:

 gnome-shell-extension-show-ip
 gnome-shell-extension-system-monitor
 gnome-shell-extension-disconnect-wifi

libnm now includes a patch that installs empty typelibs with the names of the old modules -- works well enough for letting the old extensions fail gracefully in the new shell: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=4d1f090

Also, I've submitted patches to all affected extension maintainers so that they're compatible with both libnm and libnm-glib to allow smooth transition away from libnm-glib.

I think the above is sufficient to prevent the disaster in any case and avoid breaking anything if the user keeps their extensions up to date (once the extension maintainers pick the patches up).

Created attachment 363148 [details]
[PATCH v5] network: port to libnm

More testing, some more fixes.

(In reply to Lubomir Rintel from comment #17)
> Created attachment 363148 [details]
> [PATCH v5] network: port to libnm
> 
> More testing, some more fixes.

I tested this version. When activating a VPN connection via gnome-shell, I no longer get the lock icon and it doesn't show an active VPN connection.

Created attachment 364590 [details]
[PATCH v6] network: port to libnm

(In reply to Michael Biebl from comment #18)
> (In reply to Lubomir Rintel from comment #17)
> > Created attachment 363148 [details]
> > [PATCH v5] network: port to libnm
> > 
> > More testing, some more fixes.
> 
> I tested this version. When activating a VPN connection via gnome-shell, I
> no longer get the lock icon and it doesn't show an active VPN connection.

How about this one?

Created attachment 365122 [details] [review]
[PATCH v7] network: port to libnm

Fixed one more typo & more warnings, possibly unrelated to libnm.

v7 patch LGTM

Review of attachment 365122 [details] [review]:

::: js/ui/components/networkAgent.js
@@ +583,3 @@
                                               });
 
         this._dialogs = { };

Nit: vpnRequest

@@ +596,3 @@
 
+        try {
+            this._native.init (null);

Would it be possible to use:

this._native.init_async(GLib.PRIORITY_DEFAULT, null, (o, res) => {
    try {
        this._native.init_finish(res);
        this._native.connect(...);
    } catch(e) {
        this._native = null;
        logError(e, ...);
    }
});

Otherwise nit: No space before parenthesis

::: meson.build
@@ +105,3 @@
 if get_option('networkmanager')
+  nm_deps += dependency('libnm', version: nm_req)
+  nm_deps += dependency('libnma', version: nm_req)

Unneeded as far as I can see (i.e. libnma is only used from javascript via gobject-introspection)

::: src/shell-network-agent.c
@@ -22,3 @@
 #include "config.h"
 #include <string.h>
-#include <dbus/dbus-glib.h>

Yay!

Created attachment 366656 [details]
[PATCH v8] network: port to libnm

Thanks for the review.

(In reply to Florian Müllner from comment #22)
> Review of attachment 365122 [details] [review] [review]:
> 
> ::: js/ui/components/networkAgent.js
> @@ +583,3 @@
>                                                });
>  
>          this._dialogs = { };
> 
> Nit: vpnRequest

Sorry, I don't understand what you mean here.

> @@ +596,3 @@
>  
> +        try {
> +            this._native.init (null);
> 
> Would it be possible to use:
> 
> this._native.init_async(GLib.PRIORITY_DEFAULT, null, (o, res) => {
>     try {
>         this._native.init_finish(res);
>         this._native.connect(...);
>     } catch(e) {
>         this._native = null;
>         logError(e, ...);
>     }
> });

Actually, yes, but... turns out the async path is broken [1] :(

[1] https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=603daa5b2

The fix will be included in next NetworkManager release (that's due either today or tomorrow). I've appended a fix to the end of the v8 patch, but perhaps it should not be applied right away.

> Otherwise nit: No space before parenthesis

Fixed.

> ::: meson.build
> @@ +105,3 @@
>  if get_option('networkmanager')
> +  nm_deps += dependency('libnm', version: nm_req)
> +  nm_deps += dependency('libnma', version: nm_req)
> 
> Unneeded as far as I can see (i.e. libnma is only used from javascript via
> gobject-introspection)

Fixed.

> ::: src/shell-network-agent.c
> @@ -22,3 @@
>  #include "config.h"
>  #include <string.h>
> -#include <dbus/dbus-glib.h>
> 
> Yay!

(In reply to Lubomir Rintel from comment #23)
> (In reply to Florian Müllner from comment #22)
> >          this._dialogs = { };
> > 
> > Nit: vpnRequest
> 
> Sorry, I don't understand what you mean here.

Yeah, looks like splinter gets confused by multiple patches in a single attachment. The second patch adds a local "vpn_request" variable, but our javascript style is to use camelCase for variable names (thus "vpnRequest").

Oh, and one more thing - please add the URL of this bug to the end of the commit messages.

Created attachment 366716 [details] [review]
[PATCH v9] network: port to libnm

All done.

Okay if I merge it now?

(In reply to Lubomir Rintel from comment #26)
> Okay if I merge it now?

If I understood you correctly, initializing the client requires a bleeding edge NetworkManager, so maybe best to leave out the last commit for now, as suggested in comment #23.

The rest looks good to me now, thanks for getting this done!

Thanks. I've applied these now:

16a1c35e2 network: unregister the agent when it's disabled
baacd216d network: remove the vpn request when it's serviced
d71af5e57 network: port to libnm

Keeping this open, assigned to me, so that I'll remind myself to apply the remaining patch at some later date (after GNOME 3.28 I suppose).

I backported those 3 commits(In reply to Lubomir Rintel from comment #28)
> Thanks. I've applied these now:
> 
> 16a1c35e2 network: unregister the agent when it's disabled
> baacd216d network: remove the vpn request when it's serviced
> d71af5e57 network: port to libnm
> 
> Keeping this open, assigned to me, so that I'll remind myself to apply the
> remaining patch at some later date (after GNOME 3.28 I suppose).

I backported those 3 commits to 3.26 and gave it a try. I also recompiled all my VPN plugins to drop the legacy dbus-glib support (i.e. no more /etc/NetworkManager/VPN/*.name)

Here are some findings:

a/ with the legacy vpn plugins, gnome-shell fails to activate a VPN connection
I had to replace the following code in gnome-shell:

--- gnome-shell-3.26.2.orig/js/ui/components/networkAgent.js
+++ gnome-shell-3.26.2/js/ui/components/networkAgent.js
@@ -594,7 +594,7 @@ var NetworkAgent = new Lang.Class({
         this._vpnRequests = { };
         this._notifications = { };
»
-        this._pluginDir = Gio.file_new_for_path(GLib.build_filenamev([Config.SYSCONFDIR, 'NetworkManager/VPN']));
+        this._pluginDir = Gio.file_new_for_path(GLib.build_filenamev(['/usr/lib/NetworkManager/VPN']));

In Debian, the VPN .name files live in /usr/lib/NetworkManager/VPN. Hard-coding this value is probably not a good solution.
We should probably get this dynamically during build via "pkg-config --variable=vpnservicedir libnm"

b/ after fixing that, I was able to activate VPN connections, but I get the following warnings/errors in the journal:

Jan 21 20:22:04 pluto NetworkManager[32106]: <info>  [1516562524.2809] audit: op="connection-activate" uuid="XXXX" name="FOO" pid=1500 uid=1000 result="success"
Jan 21 20:22:04 pluto gnome-shell[859]: JS ERROR: TypeError: item is undefined
                                        NMVpnSection<.setActiveConnections/<@resource:///org/gnome/shell/ui/status/network.js:1515:17
                                        NMVpnSection<.setActiveConnections@resource:///org/gnome/shell/ui/status/network.js:1512:9
                                        wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22
                                        NMApplet<._syncVpnConnections@resource:///org/gnome/shell/ui/status/network.js:1854:9
                                        wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22

(In reply to Michael Biebl from comment #29)
> Here are some findings:
> 
> a/ with the legacy vpn plugins, gnome-shell fails to activate a VPN
> connection

Yes, see commit 3f3e514ff2da.


> In Debian, the VPN .name files live in /usr/lib/NetworkManager/VPN.
> Hard-coding this value is probably not a good solution.
> We should probably get this dynamically during build via "pkg-config
> --variable=vpnservicedir libnm"

Ah, I was only looking in NetworkManager (see meson.build), that looks indeed nicer (although the current path should work for Debian as well)

Created attachment 367181 [details] [review]
networkAgent: Pick up VPN service dir from pkg-config

It turns out that NetworkManager does export the directory as pkg-config
variable after all, so use that instead of building the path ourselves
from the prefix.

Review of attachment 367181 [details] [review]:

lgtm.

Comment on attachment 367181 [details] [review]
networkAgent: Pick up VPN service dir from pkg-config

Attachment 367181 [details] pushed as 1d3154a - networkAgent: Pick up VPN service dir from pkg-config

Hi,

I ran into another issue while testing these patches.
I've applied them atop of 3.26 in
https://salsa.debian.org/gnome-team/gnome-shell/commit/e6344708b5e0e68feb8200dd3c9ea46eb0df5468

Now, when I don't have a VPN connection setup, the VPN menu has no label at all. See the attached screenshot

Created attachment 367494 [details]
vpn no labels

(In reply to Michael Biebl from comment #34)
> I ran into another issue while testing these patches.

No, that's another unrelated issue. See commit f91fbd7728 for the fix.

Comment on attachment 366716 [details] [review]
[PATCH v9] network: port to libnm

Commited as d71af5e5795bba3a3d5557cfccaca9d2adcd03a2

(In reply to Lubomir Rintel from comment #28)
>
> Keeping this open, assigned to me, so that I'll remind myself to apply the
> remaining patch at some later date (after GNOME 3.28 I suppose).

note: the "remaining patch" refers to comment 23 to use this._native.init_async and this still still not done.

Mmh, OK. Although I'm not sure how useful the bugzilla bug still is given that the remaining patch should be filed as merge request on gitlab rather than attached here ...

GNOME is going to shut down bugzilla.gnome.org in favor of  gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/

Thank you for your understanding and your help.