Major API/ABI break - soname change required or revert

Question

Major API/ABI break - soname change required or revert

remicollet opened this issue 4 years ago · 19 comments

remicollet commented 4 years ago

Looks like version 8.1.0 have major API changes, so soname MUST be change to 2

But why breaking everything ?

Answer 1 · 2020-09-24T07:24:49.000Z

please consider reverting everything in e024e70 excepted what is really related to the memory leak.

Answer 2 · 2020-09-24T07:40:59.000Z

please consider reverting everything in e024e70 excepted what is really related to the memory leak.

Any other issues in e024e70? Some struct and function are having performance issue and has been replaced. The public interface is remaining the same.

Answer 3 · 2020-09-24T07:53:52.000Z

The public interface is remaining the same.

Obviously no,
See chrislim2888/IP2Location-PECL-Extension#8

Answer 4 · 2020-09-24T08:00:01.000Z

Sorry, I didn't know that the PECL-Extension is using the IP2Loc_DBInterface. Let me check.

Answer 5 · 2020-09-24T08:09:47.000Z

Also see abi-compliance-checker report

Answer 6 · 2020-09-24T12:13:40.000Z

So what is your plan ?

At least a quick 8.1.1 expected (for the netspeed thing)

And if you plan to keep this new API, please bump the soname

diff --git a/libIP2Location/Makefile.am b/libIP2Location/Makefile.am
index e00de6c..f17ca23 100644
--- a/libIP2Location/Makefile.am
+++ b/libIP2Location/Makefile.am
@@ -7,4 +7,4 @@ lib_LTLIBRARIES = libIP2Location.la
 include_HEADERS = IP2Location.h
 
 libIP2Location_la_SOURCES = IP2Location.c                                                                      
-libIP2Location_la_LDFLAGS = -no-undefined -version-info 1:0:0
+libIP2Location_la_LDFLAGS = -no-undefined -version-info 2:0:0

Answer 7 · 2020-09-24T14:14:10.000Z

Please also note that Developers_Guide.txt is outdated.

Answer 8 · 2020-09-24T23:06:11.000Z

Thanks for your advice, we decided to keep the changes by a bump to the soname.

Answer 9 · 2020-09-30T04:58:34.000Z

Look like the 8.1.4 revert most of the changes...

Sorry, but this project seems terribly managed

Answer 10 · 2020-09-30T05:08:10.000Z

Yes indeed. We are not familiar with C and hope everything is back on track now.

Answer 11 · 2020-09-30T05:13:28.000Z

I don't know what is your expectation, but 8.1.4 break API again
https://rpms.remirepo.net/compat_reports/libip2location/8.1.3_to_8.1.4/compat_report.html

And still not equivalent to 8.0.x

Answer 12 · 2020-09-30T05:30:55.000Z

Yes indeed. We are not familiar with C and hope everything is back on track now.

omg...you want to maintain a C-library and nobody in R&D is familiar with C? Please talk to your management...if IP2Location management want to have a C-library available (to be able to sell their database for C-programs which want to take use of it), in worst case they have to hire some part-time developers to get this proper implemented.

Otherwise I would strongly recommend to drop the C-Library support completly (incl. an announcement) and then the rest of the world C-programmers will get notified and can drop IP2Location support at all (and then switch or only use maxminddb support - here at least 2 databases GeoIP and DBIP.com are already available).

BTW: potentially also an option for IP2Location: switch to the very flexible maxminddb implementation and drop the selfmade BIN database.

Answer 13 · 2020-09-30T15:05:48.000Z

Notice: some tools exist to help you, I will recommend running https://lvc.github.io/abi-compliance-checker/ before each release

You can also trust some users / contributors to check if everything seems OK before a new release

And of course, read the fucking manuals, we never read it enough ;)

Answer 14 · 2020-09-30T22:34:02.000Z

Thanks for the advice.

Working on version 8.2.0 to restore all the variables and functions from the version 8.0.9 now.

Answer 15 · 2020-10-01T02:55:05.000Z

I have reverted the variables and functions.

Already verified with ABI.

Please have a look before releasing a new version.

Thanks!

Answer 16 · 2020-10-01T05:14:12.000Z

Again, I don't know what are your expectations, please explain.

BTW, 8.2.0 breaks API again
https://rpms.remirepo.net/compat_reports/libip2location/8.1.4_to_8.2.0/compat_report.html
Why removing symbols introduced in 8.1 ?

And still not equivalent to 8.0.9
https://rpms.remirepo.net/compat_reports/libip2location/8.0.9_to_8.2.0/compat_report.html

Answer 17 · 2020-10-01T05:40:26.000Z

For now you have -version-info 2:0:0

If you fix bug => compatible change
If you add symbol => enhancement, and compatible
If you remove symbol of break 2 API in any other way => (new ABI) all app using this library will need to be rebuild (and probably adapted)

Read https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html

Answer 18 · 2020-10-01T07:43:31.000Z

The 8.2.0 should compatible with 8.1.4 now.
The symbols are removed as I try to match with 8.0.9.

It's not possible to compatible with 8.0.9 now as previously we union two int to store IPv6. The new method is using in6_addr to store IPv6 in IP2Location_readIPv6Address() and ipv6_compare().

Answer 19 · 2020-10-01T11:54:38.000Z

Looks like API is compatible (sources), not ABI (binaries)
so if you release 8.2.0 that way, you have to bump again the soname.