SONAME not changed despite incompatible changes

[andreasbombe]

In addition to the separate issue where the library SONAME is not actually set (and for which I have a pull request #335) the SONAME has not changed in version 1.6.0 from the one used in 1.5.0 despite incompatible changes requiring a new SONAME.

There is a minor one in liquid_print_crc_schemes() now returning int rather than void, but the major problems are missing symbols:

framesync64_execute_rxpayload
framesync64_execute_rxpreamble
framesync64_execute_seekpn
framesync64_step
msequence_default
ofdmframesync_estimate_eqgain
qnsearch_normalize_gradient
qnsearch_run

If my pull request is merged, it would be sufficient to simply change the SOVERSION variable in makefile.in to 2 (and continue to increment that in future releases whenever the API is changed in a way that could prevent a program compiled for the older version to start and work correctly).

[jgaeddert]

With regard to the missing symbols, most of those are internal methods for which I wouldn't expect a need to retain older versions of them. For example, the framesync64_seekpn has been supplanted with the use of qdetector_cccf object which handles the initial synchronization.

Many of the objects now return int rather than void to support error handling. Is that going to require a major version change?

Thanks for bringing this to my attention!

[andreasbombe]

Indeed the missing symbols are internal only and ideally would not be exported. For some of those it looks like they are internal to one file only and declaring them static would be sufficient. The others, those that have prototypes in liquid.internal.h, are harder. To control their visibility would require either libtool's -export-symbol option or ld version scripts.

It is up to you, but it is probably more effort than it's worth for a library that doesn't release very often. For the Debian packaging it means I can't use the symbols file to have programs compiling against it choose the lowest version providing the symbols as its dependency. Again, for a library that doesn't release very often it's not really a big deal.

Regarding the change from void to int return values it should be safe for all architectures on C as far as I can see for a program compiled against the void API (not expecting return values) running against the error-int-returning API. I was a bit concerned about having a newer program running against an older library with the same SONAME, but I realize now that is not necessarily expected to work anyway.

In conclusion, you are likely right that it should be okay to keep the SONAME the same. I will close this issue then.

[szpajder]

msequence_create() in version 1.6.0 is not backwards compatible. It requires different arguments than in prior versions, hence any app using msequence will stop working after liquid-dsp is upgraded (it will run but it won't work correctly because msequence_advance() will return wrong results). SOVERSION change shall be mandatory in such cases.

[seonlee2013]

好啊，邮件收到啦！ Hi, your email is arrived safely

[jgaeddert]

Oof. I'm so sorry @szpajder. This is correct: I did change the interface to msequence. As is probably evident, I'm making a lot of interface changes to support new functionality, but the thing I'm struggling with is version naming. Interface changes should require a major version change (e.g. from 1.6.0 to 2.0.0), but the changes aren't significant enough in my opinion to merit that. And while technically the soname version doesn't need to match the major/minor/patch release version, but it makes things easier to manage that way.

I realize I messed that up for 1.6.0, but moving forward, what is the best approach development this way? Obviously changing interfaces without a major version bump (e.g. soname change) is bad practice and can lead to a lot of user frustration.

[jgaeddert]

The letter of the law is, of course, if a new library breaks ABI functionality, its soname must change. That was on me.

[szpajder]

I realize I messed that up for 1.6.0, but moving forward, what is the best approach development this way? Obviously changing interfaces without a major version bump (e.g. soname change) is bad practice and can lead to a lot of user frustration.

The main principles are summarized in this article.

If you are about to change the interface, please consider adding a new interface first, rather than modifying the existing one. Taking msequence as an example - rather than changing how msequence_create works internally, please add msequence_create_v2 which works the new way. Optionally turn msequence_create into a wrapper for msequence_create_v2 and mark it as deprecated, so that application developers get a proper warning during compilation of their apps and are aware of the imminent API change. This may seem ugly, but it doesn't matter, because it's just an interim solution which allows existing apps to work as before.

Once you accumulate enough significant changes and/or interim API hacks, you make a new major release with a new major version and soname, remove deprecated functions and interim API hacks and document all incompatible changes in the changelog, so that we know exactly which parts of the interface no longer work as before and what we need to do to switch to the new interface. Especially this final part was missing when msequence change happened, my HFDL decoder would still run, but was unable to decode anything. I suspected that this has something to do with liquid-dsp version, but how do I know whether this is a feature or a bug? I ended up git-bisecting liquid-dsp source tree to find the relevant commit, but still had to figure out how shall I convert the old parameters into new ones, because neither the commit log nor the changelog were clear about how to do this.

[jgaeddert]

I'm so sorry. I know this caused you quite a bit of frustration, and while I'm glad you were able to find the issue, I'm embarrassed that it was me who caused it. I will heed your advice and ensure API changes are clearly documented, don't break backwards compatibility, and produce a future deprecation compiler warning.