search

google / s2geometry

Computational geometry and spatial indexing on the sphere
http://s2geometry.io/
Apache License 2.0
2.29k stars 302 forks source link

ABI compatability and SOVERSION #278

Closed spaetz closed 1 year ago

spaetz commented 1 year ago

Hello, I am about to finish the Debian packaging of s2geometry, so the lib will likely make it into the Debian repository soon. One question on your ABI stability promises:

CMakeLists.txt contains SOVERSION ${PROJECT_VERSION_MAJOR}, which a Debian developer has called "optimistic". Are you planning to remain ABI compatible over all of ${PROJECT_VERSION_MAJOR}? If yes, great, if no, we can still change that :)

Thanks for s2geometry

spaetz commented 1 year ago

P.S. Just FYI: s2geometry is about to find its way into main Debian right now: https://ftp-master.debian.org/new/s2geometry_0.10.0-2.html

jmr commented 1 year ago

Are you planning to remain ABI compatible over all of ${PROJECT_VERSION_MAJOR}?

I was planning to, once it reached version 1, but we haven't promised anything. Definitely no promises for version 0.

CMakeLists.txt contains SOVERSION ${PROJECT_VERSION_MAJOR}, which a Debian developer has called "optimistic".

Yeah, this would probably mean that nearly every release is a new major version.

Do you have any suggestions of best practices? Should we remove SOVERSION?

spaetz commented 1 year ago

Right, now you need to bump the major version on all ABI breaks. You can just do that and keep things that way, but how about simply manually setting SOVERSION 0 and bumping it manually when you break the ABI? Or remove SOVERSION (at least while you are in 0.x area), and don't promise anything at all....

Both options are possible, I guess.

jmr commented 1 year ago

how about simply manually setting SOVERSION 0 and bumping it manually when you break the ABI?

We'll update the project version appropriately.

Or remove SOVERSION

Looks like SOVERSION was added in #161, so some people might not like that.

According to SemVer, version 0 promises nothing. Is that ok for you?

spaetz commented 1 year ago

Yes, that would be fine with us.

jmr commented 1 year ago

In addition to the general "version 0 promises nothing" warning, I'd like to call out that we want to change the python bindings significantly (like every function name changes) soon.

https://ftp-master.debian.org/new/s2geometry_0.10.0-2.html isn't there anymore, but it may be better to hold off on packaging the python parts so we don't get a bunch of new users who are then soon broken. @spaetz What do you think?

spaetz commented 1 year ago

Too late, it is in Debian sid now :-). https://packages.debian.org/sid/python3-pywraps2

Needed to call the package this name as this is the module name that needs to be imported (debian policy).

Just change stuff, in the worst case, we will have to bump the Debian epoch. So no need to hold yourself back.

Perhaps while you are it, you can rename the module from pywraps2 to s2 or so? pywraps2 feels a bit awkward. -- Sent from mobile device.