After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 742993 - add 1.2 and 1.0.2 versioning stuff, belatedly fix new APIs
add 1.2 and 1.0.2 versioning stuff, belatedly fix new APIs
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: API
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2015-01-15 21:49 UTC by Dan Winship
Modified: 2015-06-26 14:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
macro to export symbols (2.71 KB, patch)
2015-01-19 14:04 UTC, Thomas Haller
none Details | Review
[patch v2] macro to export symbols (2.94 KB, patch)
2015-01-19 15:12 UTC, Thomas Haller
none Details | Review
libnm-core, libnm-util: add 1.0.2 version macros (2.69 KB, patch)
2015-01-21 17:58 UTC, Dan Winship
none Details | Review
internal: add NM_BACKPORT_SYMBOL() (1.96 KB, patch)
2015-01-21 17:58 UTC, Dan Winship
none Details | Review

Description Dan Winship 2015-01-15 21:49:55 UTC
danw/versioning-bgoXXXXXX adds NM_VERSION_1_2, NM_AVAILABLE_IN_1_2, etc.

Since it looks like jk/numeric-bond-modes is going to be backported to 1.0 (correct?) I also added corresponding macros for 1.0.2, and marked the new APIs as being first available in that. And fixed libnm.ver to correctly indicate that they didn't exist in 1.0.0.
Comment 1 Dan Winship 2015-01-15 21:51:52 UTC
(This is intended to be backported to nm-1-0 as well of course.)
Comment 2 Thomas Haller 2015-01-16 12:26:16 UTC
>> libnm-core, libnm-util: belatedly update version macros
    
I would split up this commit in first adding the new macros, and secondly updating NM_VERSION_CUR_STABLE. Because you only want to backport the new macros.


>> libnm: fix versioning on new APIs, bump soname

- * Since: 1.0.2
+ * Since: 1.2.0

- libnm_1_0_2 {
+ libnm_1_2_0 {


I think master is decoupled from stable releases and should not reference to them at all (only old branches can refer to master)

New API must first be added to master. There it should be under "libnm_1_2_0" (NEXT_STABLE). If we backport it to nm-1-0, there it must be under libm_1_2_0 too (otherwise users cannot seamlessly update from 1.0.2 to 1.2.0).

Adding it new API in master as libnm_1_0_2 already implies that you planned to backport it immediately. But that is not certainly the case. We might add a function now, and only backport it to nm-1-0 later for 1.0.4.

Also, with "Since: 1.0.2", I would expect that any "1.1.0" has it. Which is not the case.

Case in point: pre-1.0 we added new API as "Since: 1.0". We didn't add it as "Since: 0.9.10.2".
Comment 3 Dan Winship 2015-01-16 13:26:00 UTC
(In reply to comment #2)
> New API must first be added to master. There it should be under "libnm_1_2_0"
> (NEXT_STABLE). If we backport it to nm-1-0, there it must be under libm_1_2_0
> too

Adding a libnm_1_2_0 section to nm-1-0 would completely break symbol versioning. If NetworkManager-libnm-1.0.2.rpm provided "libnm.so.0(libnm_1_2_0)", then RPM would consider it to satisfy the dependencies of packages built against NM 1.2 (ie, it would assume that it implements the *entire* libnm_1_2_0 API).

> Adding it new API in master as libnm_1_0_2 already implies that you planned to
> backport it immediately. But that is not certainly the case. We might add a
> function now, and only backport it to nm-1-0 later for 1.0.4.

In general, you're not supposed to add new API in point releases (and this is one of the reasons). But anyway, we *do* plan to backport this immediately. (It's going into 7.1.).

If we do end up adding something to libnm_1_2_0 and then later needing to backport it, I think it's possible to have a symbol be exported from two different versions.

> Also, with "Since: 1.0.2", I would expect that any "1.1.0" has it. Which is not
> the case.

Unstable releases are unstable.

> Case in point: pre-1.0 we added new API as "Since: 1.0". We didn't add it as
> "Since: 0.9.10.2".

For the most part we didn't plan to backport any of that new API.

And in the absence of symbol versioning, it's not necessary to get it right the first time anyway. Changing "NM_AVAILBLE_IN_1_0" to "NM_AVAILABLE_IN_0_9_10_2" isn't an API or ABI break.
Comment 4 Dan Winship 2015-01-16 13:30:13 UTC
(In reply to comment #2)
> >> libnm-core, libnm-util: belatedly update version macros
> 
> I would split up this commit in first adding the new macros, and secondly
> updating NM_VERSION_CUR_STABLE. Because you only want to backport the new
> macros.

oops, I was about to do this, but then I realized that that's wrong; we should have changed it when we released NM 1.0.0, so it's currently wrong on nm-1-0, so we do want to backport it.
Comment 5 Thomas Haller 2015-01-16 15:18:35 UTC
(In reply to comment #3)
> (In reply to comment #2)

> > Case in point: pre-1.0 we added new API as "Since: 1.0". We didn't add it as
> > "Since: 0.9.10.2".
> 
> For the most part we didn't plan to backport any of that new API.

How about:

New API on master should always be added as libnl_1_2_0 (NEXT_MAJOR).

Whenever backporting anything, it should be backport as (e.g.) libnl_1_0_2, and also add the same section libnl_1_0_2 to master -- so that master satisfies 1.0.2 too. But until backporting, the new API is added to libnl_1_2_0 only.

(this would work in both cases, whether you already know that a new API will be backported, or whether the requirement comes up later)


> > Also, with "Since: 1.0.2", I would expect that any "1.1.0" has it. Which is 
> > not the case.

> Unstable releases are unstable.

Note, we might need to backport stuff to nm-1-0 even after nm-1-2 released (for rhel-7). Then the example applies.
Then we would add new API to libnl_1_4_0, backport to both nm-1-0 (libnm_1_0_12) and nm-1-2 (libnm_1_2_4). With nm-1-2 and master providing also libnm_1_0_12, and with master also providing libnm_1_2_4.
Comment 6 Dan Williams 2015-01-16 16:55:23 UTC
(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #2)
> 
> > > Case in point: pre-1.0 we added new API as "Since: 1.0". We didn't add it as
> > > "Since: 0.9.10.2".
> > 
> > For the most part we didn't plan to backport any of that new API.
> 
> How about:
> 
> New API on master should always be added as libnl_1_2_0 (NEXT_MAJOR).
> 
> Whenever backporting anything, it should be backport as (e.g.) libnl_1_0_2, and
> also add the same section libnl_1_0_2 to master -- so that master satisfies
> 1.0.2 too. But until backporting, the new API is added to libnl_1_2_0 only.
> 
> (this would work in both cases, whether you already know that a new API will be
> backported, or whether the requirement comes up later)
> 
> 
> > > Also, with "Since: 1.0.2", I would expect that any "1.1.0" has it. Which is 
> > > not the case.
> 
> > Unstable releases are unstable.
> 
> Note, we might need to backport stuff to nm-1-0 even after nm-1-2 released (for
> rhel-7). Then the example applies.
> Then we would add new API to libnl_1_4_0, backport to both nm-1-0
> (libnm_1_0_12) and nm-1-2 (libnm_1_2_4). With nm-1-2 and master providing also
> libnm_1_0_12, and with master also providing libnm_1_2_4.

Until somebody tells me I'm wrong, this all sounds right to me :)
Comment 7 Dan Winship 2015-01-16 17:13:57 UTC
OK, it looks like if we want a symbol to appear in multiple versions, we have to actually define a second function and then use gnu-specific magic to rename it. So if we had nm_utils_bond_mode_int_to_string() in libnm_1_2_0, and we wanted to add it to libnm_1_0_2 too, then we'd need to do:

>+const char * nm_utils_bond_mode_int_to_string_1_0_2 (int mode);
>+__asm__(".symver nm_utils_bond_mode_int_to_string_1_0_2,nm_utils_bond_mode_int_to_string@libnm_1_0_2");
>+
>+const char *
>+nm_utils_bond_mode_int_to_string_1_0_2 (int mode)
>+{
>+       return nm_utils_bond_mode_int_to_string (mode);
>+}

Yuck. So, we *can* do this when we need to, but I think in cases where we know ahead of time that we're going to backport it, we should probably just "cheat" and put it in the right version from the start.
Comment 8 Thomas Haller 2015-01-16 19:17:28 UTC
AFAIS, you don't need a different function and no need to redeclare the prototyp. But you would have to state again the 1_2_0 version. Seems the entry form the version script gets ignored:

__asm__(".symver nn_foo,nm_foo@libnm_1_2_0");
__asm__(".symver nn_foo,nm_foo@libnm_1_0_2");
const char * nm_foo (int mode)
{
   /*implementaiton*/
}



Acceptable IMO
Comment 9 Dan Winship 2015-01-16 19:48:46 UTC
That's what I tried first. It doesn't work:

  CC       nm-utils.lo
/tmp/cclEYgFc.s: Assembler messages:
/tmp/cclEYgFc.s:9965: Error: multiple versions [`nm_utils_bond_mode_int_to_string@libnm_1_2_0'|`nm_utils_bond_mode_int_to_string@libnm_1_0_2'] for symbol `nm_utils_bond_mode_int_to_string'
Comment 10 Thomas Haller 2015-01-19 14:04:29 UTC
Created attachment 294889 [details] [review]
macro to export symbols

Using a macro, the extra effort is IMO acceptable (codewise).
Comment 11 Thomas Haller 2015-01-19 15:12:01 UTC
Created attachment 294893 [details] [review]
[patch v2] macro to export symbols 

Update. Mark the exported symbols also in libnm.ver and check them with check-exports.sh
Comment 12 Dan Winship 2015-01-21 15:45:25 UTC
OK, pushed an updated danw/versioning-bgo742993 and a new danw/nm-1-0 showing the backporting

The new versioning-bgo742993 branch adds NM_BACKPORT_SYMBOL(), based on Thomas's macro, but taking advantage of the fact that gcc lets you have a void function do "return (VOID EXPRESSION)", so we don't actually need a separate _VOID version of the macro. And then the new functions are originally added to 1.2, but then backported to 1.0.2.

danw/nm-1-0 backports everything to nm-1-0, including actually backporting the bond-mode stuff (which hadn't been backported yet, although it needs to be per rh1171009). I also realized that NM_VERSION_NEXT_STABLE should be NM_VERSION_1_0 in nm-1-0, not NM_VERSION_1_2 (because NEXT_STABLE is there for NM and nma to use during unstable branches, but if they're actually compiling against a stable branch, they don't really want to be using the API from the *NEXT* stable branch.)
Comment 13 Dan Williams 2015-01-21 16:05:54 UTC
Both look fine to me.
Comment 14 Thomas Haller 2015-01-21 16:17:34 UTC
 #define _NM_BACKPORT_SYMBOL_IMPL(VERSION, RETURN_TYPE, ORIG_FUNC, VERSIONED_FUNC, ARGS_TYPED, ARGS) \
+RETURN_TYPE ORIG_FUNC ARGS_TYPED; \
 RETURN_TYPE VERSIONED_FUNC ARGS_TYPED; \
 RETURN_TYPE VERSIONED_FUNC ARGS_TYPED \


>> libnm: backport nm_utils_bond_mode_int_to_string() and _string_to_int()

- * Since: 1.2
+ * Since: 1.0.2
 
-NM_AVAILABLE_IN_1_2
+NM_AVAILABLE_IN_1_0_2
 int nm_utils_bond_mode_string_to_int (const char *mode);

I think it is wrong to make these changes on master. Even when backporting API, the functions appear first on 1.2.0.

I agree, in this example it makes arguably some sense.

But what if we backport API from 1.3-dev to 1.0.42 ?
Would we change:
  -Since: 1.4
  +Since: 1.0.42
? Which would be wrong, because 1.2 does not have the API, although 1.2.0 > 1.0.42.
IMO the correct answer is, that 1.0.x must not be mentioned on master.


Similarly, I don't think "libnm-core, libnm-util: add 1.0.2 version macros" belongs to master at all.



nm-1-0 branch:

>> libnm-core, libnm-util: belatedly update version macros
    
nm-1-0 branch doesn't need any NM_VERSION_1_2, NM_AVAILABLE_IN_1_2, etc. macros. As before, 1.2.x should not be mentioned on nm-1-0.

this is IMO correct,
-#define NM_VERSION_CUR_STABLE  NM_VERSION_0_9_10
+#define NM_VERSION_CUR_STABLE  NM_VERSION_1_0

but NEXT_STABLE on nm-1-0 should be NM_VERSION_1_0_2




Otherwise I like the approach.



Note some commit messages reference commit-ids (cherry-picked from), that needs updating after rebasing on master.
Comment 15 Dan Winship 2015-01-21 17:07:23 UTC
(In reply to comment #14)
>  #define _NM_BACKPORT_SYMBOL_IMPL(VERSION, RETURN_TYPE, ORIG_FUNC,
> VERSIONED_FUNC, ARGS_TYPED, ARGS) \
> +RETURN_TYPE ORIG_FUNC ARGS_TYPED; \

well, that's only needed if you want to put the backport before the actual function, which seems like a weird way to do it to me.

> I think it is wrong to make these changes on master. Even when backporting API,
> the functions appear first on 1.2.0.
> 
> I agree, in this example it makes arguably some sense.
> 
> But what if we backport API from 1.3-dev to 1.0.42 ?

Well, all of this highlights the fact that backporting API onto a stable branch is a bad idea.

And actually, we don't even need to expose the bond_mode methods as public API in nm-1-0. We can put them into nm-core-internal so ifcfg-rh, etc, can use them to implement the bugfixes, but as public API, they'd only be available in master.

> >> libnm-core, libnm-util: belatedly update version macros
> 
> nm-1-0 branch doesn't need any NM_VERSION_1_2, NM_AVAILABLE_IN_1_2, etc.
> macros. As before, 1.2.x should not be mentioned on nm-1-0.

Yeah, I think this is right... (You can still say "-DNM_VERSION_MAX_ALLOWED=NM_VERSION_1_2" even if NM_VERSION_1_2 isn't defined, because the macros are defined/used in such a way that any unrecognized symbol there is treated as meaning "newer than CUR_STABLE".)
Comment 16 Dan Winship 2015-01-21 17:20:30 UTC
repushed danw/nm-1-0 and danw/versioning-bgo742993.
Comment 17 Thomas Haller 2015-01-21 17:22:18 UTC
(In reply to comment #15)
> (In reply to comment #14)
> >  #define _NM_BACKPORT_SYMBOL_IMPL(VERSION, RETURN_TYPE, ORIG_FUNC,
> > VERSIONED_FUNC, ARGS_TYPED, ARGS) \
> > +RETURN_TYPE ORIG_FUNC ARGS_TYPED; \
> 
> well, that's only needed if you want to put the backport before the actual
> function, which seems like a weird way to do it to me.

The idea is to get a compile error if the signature differs (wrong ARGS_TYPED). But the line should be after defining VERSIONED_FUNC, so you also get a compile error if you didn't include the header file first.
Comment 18 Thomas Haller 2015-01-21 17:37:31 UTC
(In reply to comment #15)

> > I think it is wrong to make these changes on master. Even when backporting API,
> > the functions appear first on 1.2.0.
> > 
> > I agree, in this example it makes arguably some sense.
> > 
> > But what if we backport API from 1.3-dev to 1.0.42 ?
> 
> Well, all of this highlights the fact that backporting API onto a stable branch
> is a bad idea.

agree, but sometimes not avoidable when supporting a stable branch for a long time


> And actually, we don't even need to expose the bond_mode methods as public API
> in nm-1-0. We can put them into nm-core-internal so ifcfg-rh, etc, can use them
> to implement the bugfixes, but as public API, they'd only be available in
> master.
> 
> > >> libnm-core, libnm-util: belatedly update version macros
> > 
> > nm-1-0 branch doesn't need any NM_VERSION_1_2, NM_AVAILABLE_IN_1_2, etc.
> > macros. As before, 1.2.x should not be mentioned on nm-1-0.
> 
> Yeah, I think this is right... (You can still say
> "-DNM_VERSION_MAX_ALLOWED=NM_VERSION_1_2" even if NM_VERSION_1_2 isn't defined,
> because the macros are defined/used in such a way that any unrecognized symbol
> there is treated as meaning "newer than CUR_STABLE".)

what about:

-#define NM_VERSION_CUR_STABLE  NM_VERSION_0_9_10
+#define NM_VERSION_CUR_STABLE  NM_VERSION_1_0
-#define NM_VERSION_NEXT_STABLE NM_VERSION_1_0
+#define NM_VERSION_NEXT_STABLE NM_VERSION_1_0_2



After all this discussion, I think we found a consensus how to backport (even if we rather not).
So some of the bits you just dropped was good stuff:
  - on master: NM_BACKPORT_SYMBOL
  - on nm-1-0: NM_VERSION_1_0_2, NM_AVAILABLE_IN_1_0_2
Comment 19 Dan Winship 2015-01-21 17:40:34 UTC
if we decide we need to backport something later, we can add that stuff at the point when we need it
Comment 20 Dan Winship 2015-01-21 17:58:12 UTC
Created attachment 295121 [details] [review]
libnm-core, libnm-util: add 1.0.2 version macros

Add version macros for NM 1.0.2, since we will be backporting some new
1.2 API to nm-1-0.
Comment 21 Dan Winship 2015-01-21 17:58:32 UTC
Created attachment 295122 [details] [review]
internal: add NM_BACKPORT_SYMBOL()

Add a macro to insert the necessary compiler/linker magic to add a
copy of an existing symbol to an older version. Also, update
check-exports.sh to be able to check for such symbols by listed them
commented-out in the appropriate section.

(Based on a patch from Thomas Haller.)

====

note; this doesn't include the fix from comment 17
Comment 22 Dan Winship 2015-01-21 17:59:25 UTC
merged versioning-bgo742993, and pushed updated nm-1-0 branch.

The unused patches are attached here for future reference
Comment 23 Thomas Haller 2015-01-22 15:17:56 UTC
(In reply to comment #20)
> Created an attachment (id=295121) [details] [review]
> libnm-core, libnm-util: add 1.0.2 version macros
> 
> Add version macros for NM 1.0.2, since we will be backporting some new
> 1.2 API to nm-1-0.

Reminder: adding API for 1.0.2 should also include the change:

-#define NM_VERSION_NEXT_STABLE NM_VERSION_1_0
+#define NM_VERSION_NEXT_STABLE NM_VERSION_1_0_2
Comment 24 Thomas Haller 2015-06-26 14:36:58 UTC
the (In reply to Dan Winship from comment #21)
> Created attachment 295122 [details] [review] [review]
> internal: add NM_BACKPORT_SYMBOL()
> 

this is now merged to master:
http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=64ff214bb84a576c9490b73fc373e31d2c15fe53