GNOME Bugzilla – Bug 766170
[review] split GTK vpn editor plugin and add shared files [th/vpn-editor-split-bgo766170]
Last modified: 2016-05-17 16:42:10 UTC
Refactor nm-vpnc: - add "shared" directory - enable running properties tests for libnm-glib-based plugin - refactor tests to get rid of ASSERT()/FAIL() macros - split GTK part out ouf libnm-based plugin All this is very similar to what was done for nm-openvpn. An issue of this branch, likely applies to nm-openvpn as well (see also bug 765732) Please review
LGTM (pushed a trivial fixup)
"Why?" below means that perhaps a better commit message would help me understand why the commit is a good change. > a5e7b34 gitignore: add files to git-ignore file okay > 79de084 build: move "nm-vpnc-service-defines.h" to "shared/" directory > d95ad1c shared: add shared files > 35698f4 shared: use "nm-default.h" header I don't know. Suppose it's fine if Beniamino is fine with this. > 6631f58 properties/tests: refactor tests to run via TESTS instead of check-local target Please don't change tabs to spaces in Makefiles: * It's an extra whitespace change that obfuscates the patch * It's always going to be inconsistent there since tabs are sometimes significant * We usually use tabs and I personally find them more convenient > 4e2731b properties/tests: refactor assertion macros This looks good. > 04b9708 properties/tests: use nmtst_init() to setup test Why? > a62ca1a properties/tests: run tests via g_test_run() Looks good. > d584717 properties/tests: enable properties tests for libnm-glib based library Good. > f84d007 build: add "--with-more-asserts" configure option Why? > a4eae86 src/tests: refactor test-vpnc-output Why? > 382d9dc build: cleanup configure.ac > 519d49a properties: split "nm-vpnc-editor-plugin.c" out of "nm-vpnc.c" > dad9111 properties: add linker version script to libnm plugin > 86ceabb properties: rename file "nm-vpnc.c" to "nm-vpnc-editor.c" All looks good. > 6d549b3 properties: split GTK dependent editor plugin Please consider moving nm_vpn_plugin_utils_load_editor() to libnm -- there's enough logic there to warrant a library function. > 3dfcaf8 build: rename define "NM_VPNC_OLD" to "NM_VPN_OLD" Okay. > d7c2520 properties/trivial: rename VPNC_EDITOR_PLUGIN_ERROR to NMV_EDITOR_PLUGIN_ERROR Good. > 71010ce build: don't install appdata when building --without-gnome Good. > 779ba97 build: drop unused configure option --with-tests Good. Perhaps you could merge the obvious fixes/cleanups earlier?
(In reply to Lubomir Rintel from comment #2) > > 6d549b3 properties: split GTK dependent editor plugin > > Please consider moving nm_vpn_plugin_utils_load_editor() to libnm -- there's > enough logic there to warrant a library function. yes, can be done in a future step. Fixed the rest (I think), extended commit messages, and repushed. Thanks
> cb4babd build: add "--with-more-asserts" configure option > > There is an assertion macro nm_assert() which is disabled > to be a NOP by default, unless explicitly enabling via > "--with-more-asserts". So, add the same configuration option > as also for NetworkManager to support this kind of assertions. We don't use it here, and I don't believe we should. Even if we for some reason found that to be useful then g_assert() and G_DISABLE_ASSERT are well known and probably a better choice. > 34a1f74 properties/tests: use nmtst_init() to setup test > > The test will be converted to run via g_run_tests(), thus we must also > call g_test_init(), in addition to g_type_init(). > > nmtst_init() does that all for us, and additionally it provides useful > functionality for tests, among others: > > - initializes a GRand instance (optionally with seed from environment > NMTST_SEED_RAND > - allows to disable g_test_expect_message() by setting NMTST_DEBUG > - set G_MESSAGES_DEBUG to enable g_debug() messages when running > with NMTST_DEBUG=debug We use none of this functionality. Why don't we just run g_test_init(), which is well documented and more familiar to everyone?
In general LGTM, though I think I agree with lubomir about the --with-more-asserts stuff; none of that is used in the vpnc plugin right now. I don't really have an opinion on nmtst_init().
(In reply to Dan Williams from comment #5) > In general LGTM, though I think I agree with lubomir about the > --with-more-asserts stuff; none of that is used in the vpnc plugin right > now. I don't really have an opinion on nmtst_init(). I removed the configure option --with-more-asserts and dropped the commit.
after discussion, merged to master: https://git.gnome.org/browse/network-manager-vpnc/commit/?id=7aedb95661d7d0043625592266fcbdcf54752a81