Re: [PATCH 1/4] libnm-core: add wake-on-wlan configuration items

[Simon Fels <simon fels canonical com>]

On 18.01.2017 15:17, Thomas Haller wrote:
> On Tue, 2017-01-17 at 15:38 +0100, Simon Fels wrote:
> > ---
> >  libnm-core/nm-core-enum-types.c  | 27 +++++++++++++
> >  libnm-core/nm-core-enum-types.h  |  2 +
> >  libnm-core/nm-setting-wireless.c | 85
> > ++++++++++++++++++++++++++++++++++++++++
> >  libnm-core/nm-setting-wireless.h | 47 ++++++++++++++++++++++
> >  libnm/libnm.ver                  |  3 ++
> >  libnm/nm-setting-docs.xml        |  2 +
> >  6 files changed, 166 insertions(+)
>
> Hi Simon,
>
> Thanks for the patches.
>
> the patchset is for 1.2 branch (but doesn't apply there because of 
> libnm-core/nm-core-enum-types.h, which is a generated file).
>
> We always apply patches first on master, and backport from there to old
> branches. Never the other way around.
>
> This is especially the case when adding new public API. You cannot ever
> add new API to a stable branch that is not already in master.
>
> Would you be willing to rebase it to master?

Sure, stated that already in another mail I've sent directly after
sending out the patches and noticing that I've sent the wrong ones.

Just saw, that mail only went back to myself and not to the mailing
list... sorry for that :-)

> > diff --git a/libnm-core/nm-core-enum-types.c b/libnm-core/nm-core-
> > enum-types.c
> > index 83f5e88..58581bb 100644
> > --- a/libnm-core/nm-core-enum-types.c
> > +++ b/libnm-core/nm-core-enum-types.c
>
> these files are generated, they shall not be part of the patchset.
>
>
> > +/**
> > + * nm_setting_wireless_get_wake_on_wlan:
> > + * @setting: the #NMSettingWireless
> > + *
> > + * Returns the Wake-on-WLAN options enabled for the connection
> > + *
> > + * Returns: the Wake-on-WLAN options
> > + *
> > + * Since: 1.2
> > + */
>
> you cannot add new API after 1.2.0 is released, and claim it is 1.2 API
> (it is not).
>
> What you can do, is backport API from newer versions to minor stable
> releases, like for example like when we backported
> nm_setting_wired_get_wake_on_lan from 1.2, to 1.0.6:
>
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=d449d823046ac02f19d086575a19be7f40278bd0
>
> this also needs to add the backported symbols to all newer versions to
> allow the upgrade path:
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=0bc335cfbee754b472686cbad96cb6ad6d9c6a0a
>
> But doing so is a major pain, so usually it's just simpler not to
> extend already released stable API.
>
> Personally, I think it is also a mistake to add API downstream first
> (or downstream only). Adding API can only be done first in upstream
> master, then backported to the corresponding stable-branch, and then
> rebase the downstream package to a new stable-release with the new API.
> Doing it any other way, prevents a user to use an upstream version of
> libnm, or upgrade to a newer version.

For our use case its good enough to do it this way as we have a version
of network-manager we use in an environment where only NetworkManager
itself uses libnm and nothing else and we're stuck for now to 1.2.2 and
as we're not upstream we can't easily say we release a new minor version.

However you're right and I will correct these things with the next
review round to get everything correct for inclusion in master.

regards,
Simon