GNOME Bugzilla – Bug 752472
[review] lr/libnm: Port openvpn plugin to libnm
Last modified: 2015-08-19 20:26:11 UTC
$SUBJ
WIP in lr/libnm
lr/libnm ready for review: 78b3411 build: split the plugin into two halves a7f76c4 build: check for libnm & libnma 275de79 properties: port to libnm d72b309 properties: use the NM error types & quarks d440e3a properties: rename the classes to be more libnm-ish 85cf065 service: port to libnm d990bed auth-dialog: port to libnm 4c56d5b test: port to libnm defa685 build: allow build without libnm-glib Builds & works with: NetworkManager:th/libnm-vpn-plugin-bgo749877 network-manager-applet:th/vpn-plugin-info-bgo749877
We need some G_DISABLE_DEPRECATED or whatever that pragma stuff is in service-helper, becuase with --enable-more-warnings=error I get: nm-openvpn-service-openvpn-helper.c: In function ‘nm_utils_ip6_routes_to_gvalue’: nm-openvpn-service-openvpn-helper.c:370:3: error: ‘g_value_array_new’ is deprecated (declared at /usr/include/glib-2.0/gobject/gvaluearray.h:67): Use 'GArray' instead [-Werror=deprecated-declarations] array = g_value_array_new (4); ^ (and more) I guess you could C&P stuff from nm-gvaluearray-compat.h as needed? The rest LGTM.
I have the same comment as here: https://bugzilla.gnome.org/show_bug.cgi?id=752499#c6
(In reply to Dan Williams from comment #3) > We need some G_DISABLE_DEPRECATED or whatever that pragma stuff is in > service-helper, becuase with --enable-more-warnings=error I get: > > nm-openvpn-service-openvpn-helper.c: In function > ‘nm_utils_ip6_routes_to_gvalue’: > nm-openvpn-service-openvpn-helper.c:370:3: error: ‘g_value_array_new’ is > deprecated (declared at /usr/include/glib-2.0/gobject/gvaluearray.h:67): Use > 'GArray' instead [-Werror=deprecated-declarations] > array = g_value_array_new (4); > ^ > (and more) > > I guess you could C&P stuff from nm-gvaluearray-compat.h as needed? > > The rest LGTM. Ported to GDBus, the offending snippet is gone now. (In reply to Thomas Haller from comment #4) > I have the same comment as here: > https://bugzilla.gnome.org/show_bug.cgi?id=752499#c6 Addressed https://git.gnome.org/browse/network-manager-openvpn/log/?h=lr/libnm
[libnm] plugin=/usr/local/lib/NetworkManager/libnm-vpn-plugin-openvpn I think we should not put files directly under /usr/local/lib/NetworkManager/ (but with a sub-dir). OTOH, for libraries, they anyway should go to /usr/local/lib64/NetworkManager/*.so
(In reply to Thomas Haller from comment #6) > [libnm] > plugin=/usr/local/lib/NetworkManager/libnm-vpn-plugin-openvpn > > I think we should not put files directly under > /usr/local/lib/NetworkManager/ (but with a sub-dir). > > OTOH, for libraries, they anyway should go to > /usr/local/lib64/NetworkManager/*.so Hmm... we do already store it there. I'm not sure a move is relevant to this review? Maybe track that in a different bug report?
(In reply to Lubomir Rintel from comment #7) > (In reply to Thomas Haller from comment #6) > > [libnm] > > plugin=/usr/local/lib/NetworkManager/libnm-vpn-plugin-openvpn > > > > I think we should not put files directly under > > /usr/local/lib/NetworkManager/ (but with a sub-dir). > > > > OTOH, for libraries, they anyway should go to > > /usr/local/lib64/NetworkManager/*.so > > Hmm... we do already store it there. I'm not sure a move is relevant to this > review? Maybe track that in a different bug report? I mean: [VPN Connection] name=openvpn service=org.freedesktop.NetworkManager.openvpn program=@LIBEXECDIR@/nm-openvpn-service +[libnm] +plugin=@PLUGINDIR@/libnm-vpn-plugin-openvpn I argue, that plugin should be stored to plugin=@LIBEXECDIR@/libnm-vpn-plugin-openvpn
You were right... ignore my previous two comments. I rebased your branch, so that all fixup! commits follow immediately their target commit. The original version is there as lr/libnm-2. Added one more fixup commit. Other then that, LGTM
Merged: 7703ef9 merge: port to libnm, libnma & gdbus 1b8f089 build: install to a proper service directory eb0af39 build: allow build without libnm-glib 93d38d8 test: port to libnm 9d439a6 auth-dialog: port to libnm 1a18e02 service: port to libnm fa5b937 properties: rename the classes to be more libnm-ish a7628ff properties: use the NM error types & quarks 1440e17 properties: port to libnm cd8c106 build: check for libnm & libnma d06c373 build: split the plugin into two halves