Implemented the split now for openvpn plugin. See th/vpn-editor-split-bgo765732 branch, in core and nm-openvpn.

(at this point, it makes nm-connection-editor crashes. To be fixed...).

I reworked the branch in the openvpn plugin so that all the logic of loading the plugin is there -- no more changes to libnm are required anymore and NetworkManager's th/vpn-editor-split-bgo765732 branch is obsolte.

See the last fixup
  "squash! properties: split libnm-vpn-plugin-openvpn.so in two libraries"



The idea is to have the logic in "nm-vpn-plugin-utils.h", which could be copied over to other plugins so that they all do the same.
    


the crash in nm-connection-editor is still there. I don't know how to fix it.

(crash is fixed)

The branch from libnm/NetworkManager core is gone. This only needs changes to the plugin.

Looks sane in general. I do still think we should ditch the GTK-specific parts entirely, and have it just return a blob of HTML/JavaScript for the GUI to do with as it sees fit. But this approach also works as an interim measure.

My main concern is that I don't want all that dlopen() editor-factory code duplicated across all the VPN plugins. I want *less* generic duplicated code in my own VPN plugin, not *more* :)

Can we shift that somewhere it can be shared, and *not* "copied over"?

(In reply to David Woodhouse from comment #5)

Thank you

> Looks sane in general. I do still think we should ditch the GTK-specific
> parts entirely, and have it just return a blob of HTML/JavaScript for the
> GUI to do with as it sees fit. But this approach also works as an interim
> measure.

Agree. A better, more reusable form of UI plugins might be desirable, but does not conflict with this split.


> My main concern is that I don't want all that dlopen() editor-factory code
> duplicated across all the VPN plugins. I want *less* generic duplicated code
> in my own VPN plugin, not *more* :)
> 
> Can we shift that somewhere it can be shared, and *not* "copied over"?

The "shared" directory is intended to contain source files that are copied to other VPN plugins and used there.

The loading code is now in "shared/nm-vpn-plugin-utils.c" and nm_vpn_plugin_utils_load_editor() is intended to be used as-is by all other plugins.

-- it's really just the two files "nm-vpn-plugin-utils.[ch]" that need to be shared this way.

I think this "reuse-by-coping" does not necessarily result in bad code duplication, as long as we avoid modifications to cause diverging.

merged to master: https://git.gnome.org/browse/network-manager-openvpn/commit/?id=49d0b0e30e892d7cd64df17eefd16c438bbd09a3

(In reply to Thomas Haller from comment #6)
> -- it's really just the two files "nm-vpn-plugin-utils.[ch]" that need to be
> shared this way.
> 
> I think this "reuse-by-coping" does not necessarily result in bad code
> duplication, as long as we avoid modifications to cause diverging.

Didn't auth-helpers.c (in openconnect and openvpn) start out the same way?

(In reply to David Woodhouse from comment #8)
> (In reply to Thomas Haller from comment #6)
> > -- it's really just the two files "nm-vpn-plugin-utils.[ch]" that need to be
> > shared this way.
> > 
> > I think this "reuse-by-coping" does not necessarily result in bad code
> > duplication, as long as we avoid modifications to cause diverging.
> 
> Didn't auth-helpers.c (in openconnect and openvpn) start out the same way?

If they did, it resulted in bad code duplication and we failed to avoid modifications which caused diverging :)

I think, VPN plugins were created by forking and modifying an existing plugin. That didn't have the notion of "shared" files which should stay in sync after the fork.




To avoid this, we need a convention and awareness that these files are supposed to be identical in every plugin. It requires self-control when modifying one of these shared files.

E.g.: never modify one of the files
      - shared/gsystem-local-alloc.h
      - shared/nm-dbus-compat.h
      - shared/nm-glib.h
      - shared/nm-macros-internal.h
      - shared/nm-shared-utils.c
      - shared/nm-shared-utils.h
      - shared/nm-test-utils.h
in the VPN plugin. You must first adjust them in NetworkManager's shared directory, and then only re-import the entire file as one.


Alternatives to a convention could be using git-submodule, but some dislike submodules and then the dist-tarball is no longer self-contained...

> > > I think this "reuse-by-coping" does not necessarily result in bad code
> > > duplication, as long as we avoid modifications to cause diverging.
> > 
> > Didn't auth-helpers.c (in openconnect and openvpn) start out the same way?
> 
> If they did, it resulted in bad code duplication and we failed to avoid
> modifications which caused diverging :)

:)

> I think, VPN plugins were created by forking and modifying an existing
> plugin. That didn't have the notion of "shared" files which should stay in
> sync after the fork.

Yes — and that's my fault; I did the copying. But that's why I feel guilty, and responsible for the auth-helpers mess, and that's why I'm concerned about *more* shared code :)

(A lot of the auth-helpers code is actually about the certificate selection dialogs, and we have a GSoC project this year to fix that properly, so hopefully we can kill a lot of that anyway.)
 
> To avoid this, we need a convention and awareness that these files are
> supposed to be identical in every plugin.

Surely it's better just to let the core NM code provide them. It doesn't even need to be a new shared library — these plugins we're discussing are loaded *into* a NM-owned process, so the functions just need to be available to the dynamic linker when the plugin gets loaded.