I think we all agree that what happened with the msgb_wrap_with_TL() is a prime example about how absolutely *not* to do things. This makes me want to rewrite libosmocore history.
After all the naming / API duplication mistakes around msgb_wrap_with_TL(), the final state I am trying to finish my day to is that the osmo-msc and openbsc master builds remain broken, while patches to fix that are idling on gerrit with a commit log summary that mentions neither that they fix the build nor that they are related to msgb_wrap_with_TL().
All this happens with a new release tag on libosmocore, which was tagged to include a commit that is supposed to fix things instead of break them...
Let's try to avoid this kind of series of events in the future.
* Separate function definitions must not have identical names. * This applies both to the master HEADs as well as throughout entire API history. * Especially functions moved to another source tree *must* change their name.
* Such API changes in libosmocore should be tested with "all" depending programs.
* API fixes in libosmocore should ideally build with both past and future versions of depending source trees, i.e. should not need a matching patch in the other source trees.
* When a commit breaks the master build in depending programs, * clearly mark that so in the commit log and/or gerrit comments, so it is not merged on its own by accident. * the author should not go away until either none or both of them are merged.
* Similarly obvious, avoid pouring water on burning oil.
Sure, we all make mistakes every now and then. In this case we made a few more :P
I spent time now to merge fixes to the master builds instead of going to sleep... (I hope I'm not adding mistakes to the minefield, though.)
Not sure what to do about osmocom-bb. It has lots of implicit function definition warnings, only one of them is: gsm480_ss.c:441:3: warning: implicit declaration of function ‘msgb_wrap_with_TL’
~N
It looks like we need a 1.3.1 tag in osmo-msc, osmo-msc 1.3.0 won't build with libosmocore 1.0.1 :/
~N
Hi Neels,
On Tue, Jan 22, 2019 at 03:00:31AM +0100, Neels Hofmeyr wrote:
I think we all agree that what happened with the msgb_wrap_with_TL() is a prime example about how absolutely *not* to do things. This makes me want to rewrite libosmocore history.
I had the same feeling yesterday. To clarify "rewrite libosmocore git history" is probably what you hinted here :)
Let's try to avoid this kind of series of events in the future.
- Separate function definitions must not have identical names.
- This applies both to the master HEADs as well as throughout entire API history.
- Especially functions moved to another source tree *must* change their name.
This is generally what we do. And "generally", we prefix them with osmo_ in the libraries, and don't permit the osmo_ prefix for symbols in applications. However, the msgb_ code pre-dates the osmo_ prefix - it even predates the name Osmocom as the project name.
So we have some legacy prefixes that are "reserved" for use in libosmo*. Those are
bitvec_ gsmtap_ log_ msgb_ rate_ctr_ abis_nm_ gprs_cipher_ gsm0341_ gsm0480_ gsm0502_ gsm0503_ gsm0808_ gsm29118_ gsm0858_ gsm340_ gsm411_ gsm414_ gsm48_ gsm610_ gsm620_ gsm690_ gsm_7bit_ lapd_ lapdm_ milenage_ rsl_ rxlev_ tlv_ ipa_ccm bssgp_ gprs_ns_ gprs_nsvc_ btsctx_ osim_ vty_ vector_ telnet_ config_ cmd_ buffer_
Unfortunately a lot of them are rather generic, so it's hard to avoid, at least in absence of any automatic tests for it. It's a separate discussion whether we should e.g. stop to export los of libosmovty internals (buffer, vector, cmd) - and if we should prefix all symbols in libosmo* with osmo_* providing the older names only as backwards compatibility layer with possibly weak symbols to be able to migrate to a cleaner namespace at least at some point in the future.
Furthermore, there is one factors that made this particular instance even more problematic:
The respective functions were inline functions. Otherwise we could have simply turned the library symbol into a weak symbol, making any application implementation supersede the library one.
- Such API changes in libosmocore should be tested with "all" depending programs.
ACK. So what's needed is some kind of build job that continuously builds old applications against modern libosmo*. This could be run at least once per day/night, so we get notified if we introduce any such breakage and can fix libosmo* shortly to keep the incompatiblity something that appeared for a few days in the history, at maximum.
- API fixes in libosmocore should ideally build with both past and future versions of depending source trees, i.e. should not need a matching patch in the other source trees.
That goes without saying. It just wasn't possible in this case.
Regards, Harald
On Tue, Jan 22, 2019 at 04:36:19PM +0100, Harald Welte wrote:
So we have some legacy prefixes that are "reserved" for use in libosmo*. Those are
bitvec_ gsmtap_ log_ msgb_ rate_ctr_ abis_nm_ gprs_cipher_ gsm0341_ gsm0480_ gsm0502_ gsm0503_ gsm0808_ gsm29118_ gsm0858_ gsm340_ gsm411_ gsm414_ gsm48_ gsm610_ gsm620_ gsm690_ gsm_7bit_ lapd_ lapdm_ milenage_ rsl_ rxlev_ tlv_ ipa_ccm bssgp_ gprs_ns_ gprs_nsvc_ btsctx_ osim_ vty_ vector_ telnet_ config_ cmd_ buffer_
yikes, quite a few more than I would have thought!
BTW, in osmo-msc we still have a number of gsm48_ and other "reserved" prefixes as well, for example gsm48_rx_mm_serv_req(). As I'm refactoring for the inter-MSC split I should be able to get rid of them.
~N