GNOME Bugzilla – Bug 732867
some libnm-util/libnm-glib cleanup [branch review: danw/libnm-cleanup]
Last modified: 2014-07-15 14:40:42 UTC
some cleanup split out from the libnm work: 98fbe09 libnm-util, libnm-glib: tweak (element-type) annotations in docs This is to reduce the number of differences between libnm-util/libnm-glib and the initial commit of libnm, making it easier for me to rebase. 9559eec libnm-util, libnm-glib: standardize copyright/license headers dd0e1dc libnm-util, libnm-glib: whitespace fixes 0d4ecfc libnm-glib: drop separate test library These are all things I originally planned to do only to libnm, but really there's no reason we can't just do this all in libnm-util/libnm-glib now (and getting rid of libnm-glib-test makes the build a bit faster). 4aa4f22 libnm-util: move NetworkManager.h, etc, from include/ to here The real reason for this is that there are API-incompatible changes I want to make to these headers, so they can't be shared between the old and new libraries. But it seems to make sense anyway... 49cce47 introspection: fix VPN.Plugin State type 731c468 libnm-util: NetworkManager.h/NetworkManagerVPN.h doc fixups 4e74aa6 include: drop nm-settings-flags.h, move NMSecretAgentGetSecretsFlags These are basically drive-by fixes while I was working with the include/ headers.
(In reply to comment #0) > 4e74aa6 include: drop nm-settings-flags.h, move NMSecretAgentGetSecretsFlags actually, this one causes introspection/VAPI ABI breakage, so I'm dropping it for now.
> libnm-util, libnm-glib: whitespace fixes /* B / A: ensure settings in B that are not in A make the comparison fail */ if (g_hash_table_size (NM_CONNECTION_GET_PRIVATE (a)->settings) != - g_hash_table_size (NM_CONNECTION_GET_PRIVATE (b)->settings)) + g_hash_table_size (NM_CONNECTION_GET_PRIVATE (b)->settings)) return FALSE; While I'm not sure we have a policy on this, this line isn't a || or &&, so should the second part really be indented the same as the first? I feel like comparators are inherently different than conditionals... > libnm-glib: drop separate test library This does expose the environment variable as "public" API now though, which is what I was trying to avoid originally with that bit. > libnm-util: move NetworkManager.h, etc, from include/ to here One reason they were kept separate was that they were useful for stuff that doesn't use or link to libnm-util, like Firefox, Evolution, etc. The constants can be used from non-libnm code. So while I like this change, I'm not sure it's feasible...
(In reply to comment #2) > > libnm-util, libnm-glib: whitespace fixes > > /* B / A: ensure settings in B that are not in A make the comparison fail > */ > if (g_hash_table_size (NM_CONNECTION_GET_PRIVATE (a)->settings) != > - g_hash_table_size (NM_CONNECTION_GET_PRIVATE (b)->settings)) > + g_hash_table_size (NM_CONNECTION_GET_PRIVATE (b)->settings)) > return FALSE; > > While I'm not sure we have a policy on this, this line isn't a || or &&, so > should the second part really be indented the same as the first? I feel like > comparators are inherently different than conditionals... That is how I would indent them, but if you want to do something different, I can. It looks like there isn't much precedent elsewhere in NM. (And to be clear, they were already visually indented the same, it's just that it used a tab before, and now it uses spaces.) > > libnm-glib: drop separate test library > > This does expose the environment variable as "public" API now though, which is > what I was trying to avoid originally with that bit. Hm... in theory, we don't even need the functionality inside libnm-glib at all; nm_remote_settings_new() takes a DBusGConnection argument, and while nm_client_new() doesn't, we could fake it by using g_object_new() directly. Let me try rewriting the tests to do that instead... > > libnm-util: move NetworkManager.h, etc, from include/ to here > > One reason they were kept separate was that they were useful for stuff that > doesn't use or link to libnm-util, like Firefox, Evolution, etc. The constants > can be used from non-libnm code. So while I like this change, I'm not sure > it's feasible... Hm... ok, this is a little weirder than I'd thought before; they get installed to the same directory as the libnm-util include files, but (on Fedora at least), they get split into a separate package. Anyway, this change has no effect on the installed system, only on the source tree, so it doesn't affect our ability to split NetworkManager-devel and NetworkManager-glib-devel in any way. In libnm, there are a handful of changes that I was planning to make that affect these files: - rename the VPN-related types to be "NMVpnFoo" rather than "NMVPNFoo" (as part of a larger capitalization synchronization) - rename NetworkManager.h to nm-dbus-interface.h and NetworkManagerVPN.h to nm-vpn-dbus-interface.h - create a new NetworkManager.h that #includes all other public headers, and require external consumers of libnm to include only that file (like glib, gtk, and many other packages do now). I didn't realize that the existing NetworkManager.h / NetworkManagerVPN.h were intended for use by non-libnm-users too though...
(In reply to comment #3) > Hm... in theory, we don't even need the functionality inside libnm-glib at all; > nm_remote_settings_new() takes a DBusGConnection argument, and while > nm_client_new() doesn't, we could fake it by using g_object_new() directly. Let > me try rewriting the tests to do that instead... Actually, test-remote-settings-client was doing that already. And fixing test-nm-client to do the same was simple. Re-pushed with that fix.
(In reply to comment #3) > (In reply to comment #2) > > > libnm-util, libnm-glib: whitespace fixes > > > > /* B / A: ensure settings in B that are not in A make the comparison fail > > */ > > if (g_hash_table_size (NM_CONNECTION_GET_PRIVATE (a)->settings) != > > - g_hash_table_size (NM_CONNECTION_GET_PRIVATE (b)->settings)) > > + g_hash_table_size (NM_CONNECTION_GET_PRIVATE (b)->settings)) > > return FALSE; > > > > While I'm not sure we have a policy on this, this line isn't a || or &&, so > > should the second part really be indented the same as the first? I feel like > > comparators are inherently different than conditionals... > > That is how I would indent them, but if you want to do something different, I > can. It looks like there isn't much precedent elsewhere in NM. > > (And to be clear, they were already visually indented the same, it's just that > it used a tab before, and now it uses spaces.) Eh, whatever :) Ignore me. > > > libnm-glib: drop separate test library > > > > This does expose the environment variable as "public" API now though, which is > > what I was trying to avoid originally with that bit. > > Hm... in theory, we don't even need the functionality inside libnm-glib at all; > nm_remote_settings_new() takes a DBusGConnection argument, and while > nm_client_new() doesn't, we could fake it by using g_object_new() directly. Let > me try rewriting the tests to do that instead... > > > > libnm-util: move NetworkManager.h, etc, from include/ to here > > > > One reason they were kept separate was that they were useful for stuff that > > doesn't use or link to libnm-util, like Firefox, Evolution, etc. The constants > > can be used from non-libnm code. So while I like this change, I'm not sure > > it's feasible... > > Hm... ok, this is a little weirder than I'd thought before; they get installed > to the same directory as the libnm-util include files, but (on Fedora at > least), they get split into a separate package. > > Anyway, this change has no effect on the installed system, only on the source > tree, so it doesn't affect our ability to split NetworkManager-devel and > NetworkManager-glib-devel in any way. Yeah, you're right. It should be pretty easy to adjust the packaging. The only further concern I have is that having them all in the same directory could make us tie them together at some point, which would preclude allowing apps that don't link to libnm-util from using the constants in it. But I suppose we just maintain vigilance. > In libnm, there are a handful of changes that I was planning to make that > affect these files: > > - rename the VPN-related types to be "NMVpnFoo" rather than "NMVPNFoo" > (as part of a larger capitalization synchronization) Sure, sounds fine to me. > - rename NetworkManager.h to nm-dbus-interface.h and NetworkManagerVPN.h > to nm-vpn-dbus-interface.h Sure. > - create a new NetworkManager.h that #includes all other public headers, > and require external consumers of libnm to include only that file > (like glib, gtk, and many other packages do now). So this might break the non-libnm-util/glib consumers... > I didn't realize that the existing NetworkManager.h / NetworkManagerVPN.h were > intended for use by non-libnm-users too though... It just kinda turned out that way... is there a way we can preserve the standalone NetworkManager.h for those users, but still have the single-include stuff for everyone else?
>> libnm-util, libnm-glib: standardize copyright/license headers do you have a script that validates the file headers? That would be nice to include as make-check. >> libnm-util: move NetworkManager.h, etc, from include/ to here could you split the white-space changes to a separate commit? Looks good, except compilation fails now: make[5]: Entering directory `/data/src/NetworkManager/examples/C/qt' CXX add-connection-wired.o add-connection-wired.cpp:34:28: fatal error: NetworkManager.h: No such file or directory #include "NetworkManager.h"
(In reply to comment #5) > is there a way we can preserve the > standalone NetworkManager.h for those users, but still have the single-include > stuff for everyone else? Sure, we can just leave out the "#error out if not included via main .h file" check in the headers we want to be separately-includable. I can add comments to them too reminding that they should be #defines/types only. (In reply to comment #6) > >> libnm-util, libnm-glib: standardize copyright/license headers > > do you have a script that validates the file headers? That would be nice to > include as make-check. No, this was done by hand. > >> libnm-util: move NetworkManager.h, etc, from include/ to here > > could you split the white-space changes to a separate commit? sure > Looks good, except compilation fails now: > > make[5]: Entering directory `/data/src/NetworkManager/examples/C/qt' ah, right, I don't have qt-devel installed...
made remaining fixes and pushed
[For the record] branch merged as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=741ee7d0510adcd60806a12bc33cf9e4cc9e5404