Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please do NOT bump library versions without functional changes #189

Open
mandree opened this issue Jul 22, 2021 · 24 comments
Open

Please do NOT bump library versions without functional changes #189

mandree opened this issue Jul 22, 2021 · 24 comments
Labels

Comments

@mandree
Copy link

mandree commented Jul 22, 2021

Greetings,

if there is only a patchlevel change, for instance between v3.1.0 and v3.1.1, that fixes a BUILD issue that causes either "no package" or "package built" - please DO NOT BUMP library versions. This causes needless churn in package repositories and is abusing the library versioning. There is nothing that warranted bumping from .so.29.0.0 to .so.29.1.0 between v3.1.0 and v3.1.1.

@cary-ilm
Copy link
Member

Following the libtool versioning instructions here: https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html

  1. If the library source code has changed at all since the last update, then increment revision (‘c:r:a’ becomes ‘c:r+1:a’).

The source code did change, so that's why 29.0.0 bumped to 29.1.0. Are we misinterpreting this policy?

@lgritz
Copy link
Contributor

lgritz commented Jul 22, 2021

It's easy to interpret the rule as "if you change the .cpp or .h files, bump the revision, but if you just fix the build scripts, you don't."

But an alternate interpretation might be "changes to the source code that would result in a change of the object code." I think you could also say that if it didn't build correctly on a platform, and a source change makes it build, it's not really a "change" since there was no "before."

So maybe in this case there was a little wiggle room, because the "source code change" did not change what got compiled on Intel-based platforms, and for M1 Macs it wouldn't build previously at all. So we probably didn't have to bump the number.

But this stuff is super subtle. It's pretty excusable to bump the revision on every release unless you are exceptionally sure that it's identical on all platforms on which it previously built at all.

@lgritz
Copy link
Contributor

lgritz commented Jul 22, 2021

For the record, this subtlety would not have (and did not) occur to me as we were making this release. I interpreted it as you did, Cary. It's only in retrospect, with this issue highlighted for us, that I can see that there's a level of nuance that I had overlooked.

@lgritz
Copy link
Contributor

lgritz commented Jul 22, 2021

Also for the record: this whole thing has made it very clear that I do the library numbering ENTIRELY WRONG on my other projects. On the other hand, nobody has complained in > 10 years, so I'm just not sure how big an emergency it is to drop everything to fix.

@cary-ilm
Copy link
Member

My assumption is that changing the leading "current" number (29) causes churn (which in this case would have been unnecessary), but changing the "revision" number (0) does not, so "29.1.0" should be a drop-in replacement for "29.0.0", but I'd love to better understand the implications.

@mandree
Copy link
Author

mandree commented Jul 22, 2021

I don't think there is an emergency, I was just questioning if a build fix from "build broken" to "starts to compile for macOS" really warranted a revision bump. I think the assumption around "just major number relevant for ABI" is correct and 29.1.x should be drop-in compatible with 29.0.y.

I also wonder if libtool's versioning in its own libtool world maps out portably to other worlds including the cmake build world... the run-time linker of the OS is not libtool...

@meshula
Copy link
Contributor

meshula commented Jul 23, 2021

#161

@cary-ilm
Copy link
Member

A related question we raised but didn't resolve: What's the proper setting of the version and soversion on the master branch? Building off of master produces something that is typically not equivalent to an official release version. We concluded the proper strategy is for the master branch to look forward and specify the next minor version/soversion. I'd welcome any guidance about how other projects typically handle this.

@lgritz
Copy link
Contributor

lgritz commented Jul 24, 2021

"I also wonder if libtool's versioning in its own libtool world maps out portably to other worlds including the cmake build world... the run-time linker of the OS is not libtool..."

@mandree, could you elaborate? For somebody who's been at this for so very long, I'm embarrassed by my continued confusion about which rules are OS, which are linker, which are libtool conventions (aside: I don't even know for sure if the cmake build system uses libtool underneath), which are GNU organization preferences for their own projects (some of their programming conventions I agree with, others are not to my taste at all), what is really required, and what is considered best and/or common practices for managing library versions.

@meshula
Copy link
Contributor

meshula commented Jul 25, 2021

@lgritz I am exactly in the same boat. I think we should have an issue whose title is exactly your statement!!!

Edit: @mandree, I see your thumbs down emoji there :) To be clear, what I'm trying to express is that it seems we can't yet articulate on behalf of the project what the correct policy is, and so I think we need to discuss and document it. To my mind, that would be the proper resolution of this issue.

@patlefort
Copy link

If a change only affect building, then those affected by the fix can always pull the specific changeset and apply it on their build to build successfully, without bumping the version.

@meshula
Copy link
Contributor

meshula commented Sep 3, 2021

I propose that we rename this issue to "Establish library version change policy". @mandree Your original request is well taken, and this issue is changing into a centralized working discussion to investigate and establish what the policy should be. Does anyone have any objection to renaming the issue?

@mandree
Copy link
Author

mandree commented Sep 4, 2021

Renaming is fine for me.

I have been looking for documents in the meanwhile, but most I could find is GNU-libtool-centric which is adding an additional policy layer and that I do not want to endorse because libtool brings its own can of worms and is not used with cmake. All the other things essentially boil down to looking at the major version as part of the SONAME of the provided library (Imath, or openexr) and it landing in the NEEDED tags of the libraries and executables that link against it.

@hobbes1069
Copy link
Contributor

I just ran into this with the 3.1.8 release, even if nothing API/ABI changed from 3.1.7, I still have to announce the rebuilds ahead of time on the Fedora development mailing list and then rebuild all the following packages:

Field3D
ImageMagick
OpenColorIO
OpenImageIO
OpenSceneGraph
alembic
blender
darktable
freeimage
gdal
gimp
gmic
gstreamer1-plugins-bad-free
hugin
kf5-kimageformats
krita
luxcorerender
openexr
openshadinglanguage
openvdb
povray
prusa-slicer
synfigstudio
usd
vigra
vips

@lgritz
Copy link
Contributor

lgritz commented May 29, 2023

Oh no, we messed this up again, didn't we?
We bumped the "current" number, but it should never be changed for these patch releases. Patch releases should only change "revision", right?

Perhaps we should un-release and try again with the right so numbering? This mistaken change is too disruptive.

@lgritz
Copy link
Contributor

lgritz commented May 29, 2023

Asides for @hobbes1069:

  1. Mistakes notwithstanding (sorry), due to simple project coordination and limited resources, we have the habit of releasing a new imath, and then several days later, the corresponding new version of openexr. There's never any point in rebuilding things for a new imath until that openexr comes out, because most of the same packages will need to be rebuilt then as well.

  2. Is anything still using Field3D? I think we should consider that a dead project. It's not being maintained in any way. If you can't identify any other important package that's using it as a required dependency, I recommend dropping it from your list.

@cary-ilm
Copy link
Member

This was my mistake, my apologies. We'll discuss this again internally and come up with a better process. I admit I'm unclear on the precise versioning procedure for a library that is predominantly inline/templates.

The headers did change, although I believe not in ways that make a functional difference.

@hobbes1069
Copy link
Contributor

You don't necessarily need to follow this guide, but some kind of ABI comparison for ABI compatibility may be good.

https://sourceware.org/libabigail/manual/abipkgdiff.html

I run a wrapper, fedabipkgdiff, which allows me to compare against existing RPMs to help determine if there has been an API/ABI change when I'm evaluating updating a library package.

Perhaps there's a way to incorporate it (or something like it) into the CI infrastructure?

@lgritz
Copy link
Contributor

lgritz commented May 30, 2023

@cary-ilm I think the only mistake was in the very last commit that staged the release for 3.1.8, which included this change in CMakeLists.txt:

-set(IMATH_LIBTOOL_CURRENT 30)
+set(IMATH_LIBTOOL_CURRENT 31)

That first number should just never be bumped for a patch release, in which by definition and policy we never intend to create a backwards compatibility break. That is, we all agree that 3.1.8 should just be swappable in for 3.1.7.

I'm sure it was just a mistake on your part, and then the rest of us missed it when reviewing the code.

I don't think we currently have a "RELEASING" document that gives the procedures and checklist for how we release. We should make one and then just add to the checklist that we should ensure we haven't bumped the wrong numbers for patch releases.

I can't deploy 3.1.8 at my facility for basically the same reason -- it will break all the execution environments that assume 3.1.x+1 can be substituted for 3.1.x without rebuilding things, but I fear that the dynamic liker will reject the new name. I really need this new release for the DWA fix in the core library, so I'm in a bind.

I think we should re-release 3.1.8 (or else bump to 3.1.9 with a fixed library version, and deprecate 3.1.8 with prejudice, it will only break things).

@cary-ilm
Copy link
Member

cary-ilm commented May 31, 2023

I can make a 3.1.9 that corrects the so number, but it looks like the previous release is similarly broken, so presumably the situation we're in now shouldn't be all that different from where we've been since v3.1.7 in May. The .so versions for the last few releases:

Imath-3.1.5 - libImath-3_1.so.29.4.0
Imath-3.1.6 - libImath-3_1.so.29.5.0
Imath-3.1.7 - libImath-3_1.so.30.0.1
Imath-3.1.8 - libImath-3_1.so.31.0.0

In retrospect, my mistake was inadvertently applying the rule that says "If any interfaces have been added, removed, or changed since the last update, increment current, and set revision to 0" to the API rather than the ABI. 3.1.7 did introduce a new method, Matrix44<T>::trace(), although it's inline so the ABI is unaffected so the .so version should have changed only in the revision.

@kdt3rd contributed an ABI checker script to OpenEXR based on https://github.com/lvc, although abipkgdiff looks even simpler, thanks for suggesting that. There is a "Creating a Release" section of CONTRIBUTING.md, but it doesn't describe the versioning process. I'll update it to be more explicit and say, "Don't attempt to determine the .so version by human analysis. Run the ABI checker to confirm whether the release qualifies as a patch." Or better yet, we should set up a CI check to reject any commit to the release branch that changes the ABI.

abipkgdiff does report an ABI change between v3.1.7 and v3.1.8 in libPyImath_Python3_10-3_1.so (2 functions removed, 18 added, all boost_python specializations), which is odd because the source didn't change. I tried dropping the v3.1.8 library into a v3.1.7 build, and the tests succeed, so I'm willing to ignore it.

Imath v3.1.7 should have been libImath-3_1.29.6.0, and Imath v3.1.8 should have been libImath-3_1.29.7.0.

I'm not entirely sure the best fix from here, but I propose releasing a v3.1.9 with the so version libImath-3_1.29.8 (skipping 6 and 7 and keeping the revision in line with the release number), and marking the v3.1.7 and 3.1.8 releases with a warning not to use them. Feel free to suggest an alternative fix.

@lgritz
Copy link
Contributor

lgritz commented May 31, 2023

Oh boy. Well, for my part, slothy Larry hasn't upgraded internally at Imageworks since 3.1.5, so my vote would be to have 3.1.9 go back to .so.29.x series and declare both 3.1.7 and 3.1.8 to be mistakes that we wish into the cornfield.

But I don't know if Richard (if he can even still read this thread with his eyes bleeding) will have a different opinion on the least disruptive way forward.

@hobbes1069
Copy link
Contributor

It would be good to get input from other distro package maintainers, but as Fedora is on 29.5, 29.8 wouldn't be disruptive.

$ rpm -ql imath | grep libImath
/usr/lib64/libImath-3_1.so.29
/usr/lib64/libImath-3_1.so.29.5.0

@mandree
Copy link
Author

mandree commented May 31, 2023

FreeBSD has packaged the patchlevel releases, so we need to clean up anyways, which I will do. It would be beneficial to have clean-up Imath and OpenEXR releases in the same vein in the first half of June so we can properly clean up fallout before we branch our quarterly aka stable 2023Q3 packaging branch July 1st. (We maintain a latest and a no-surprises three-monthly branch for third-party packages.)

Other than that, @cary-ilm 's most recent comment and the with-hindsight policies would be much appreciated as future direction. Takeaway, do not bump without ABI change. It is the point I have apparently explained in a way so that it took some time to be understood. Thanks, Cary, for spelling my idea out in other words.

(FreeBSD rebuilds direct dependencies on incompatible changes by way of what we call PORTREVISION bumps on packages consuming a changed requisite. It is a FreeBSD version add-on on top of the upstream version counter, the latter we consider the DISTVERSION).

@kmilos
Copy link

kmilos commented May 31, 2023

In retrospect, my mistake was inadvertently applying the rule that says "If any interfaces have been added, removed, or changed since the last update, increment current, and set revision to 0" to the API rather than the ABI. 3.1.7 did introduce a new method, Matrix44<T>::trace(), although it's inline so the ABI is unaffected so the .so version should have changed only in the revision.

Not really a mistake there - what you seem to have omitted was the mandatory execution of all the steps instead of stopping where you think you hit your condition:

  1. If any interfaces have been added since the last public release, then increment age.
  2. If any interfaces have been removed or changed since the last public release, then set age to 0.

That way (step 5 is also true in addition to your step 4, but not 6), the ABI version/SOVERSION (which libtool calculates as current - age) would've stayed the same!

Programs using the previous version may use the new version as drop-in replacement, but programs using the new version may use APIs not present in the previous one. In other words, a program linking against the new version may fail with “unresolved symbols” if linking against the old version at runtime: set revision to 0, bump current and age.

See also e.g. https://github.com/pvanhoof/dir-examples and https://dcreager.net/shared-library-versions/#cmake

The real problem is that the Imath versioning policy outlined here does not match nor does it implement libtool's really but seems to be some weird cross w/ semver that is evidently not working out:

Imath/CMakeLists.txt

Lines 27 to 45 in e95e049

# Library API version (CMake's library VERSION attribute) is of the
# form CURRENT.REVISION.AGE; the CMake SOVERSION attribute corresonds
# to just CURRENT. These produce a .so and a symlink symlink, e.g.:
# libImath-3_1.so.29 -> libImath-3_1.so.29.0.0
# ^ ^ ^ ^
# | | | |
# CURRENT | | AGE
# | REVISION
# CURRENT
# When updating:
# 1. no API change: CURRENT.REVISION+1.AGE
# 2. API added: CURRENT+1.0.AGE+1
# 3. API changed: CURRENT+1.0.0
#
set(IMATH_LIBTOOL_CURRENT 29)
set(IMATH_LIBTOOL_REVISION 0)
set(IMATH_LIBTOOL_AGE 0)
set(IMATH_LIB_VERSION "${IMATH_LIBTOOL_CURRENT}.${IMATH_LIBTOOL_REVISION}.${IMATH_LIBTOOL_AGE}")
set(IMATH_LIB_SOVERSION ${IMATH_LIBTOOL_CURRENT})

For other ideas see e.g. libtiff (replicates libtool versioning exactly), libavif (does its own policy, but correctly), libjxl (semver policy)...

So, you either need to implement all the libtool mechanisms correctly, or go with your own policy but then not let the libtool one confuse you. 😉

waebbl added a commit to waebbl/gentoo that referenced this issue May 31, 2023
waebbl added a commit to waebbl/gentoo that referenced this issue May 31, 2023
Mask dev-libs/imath-3.1.7 due to upstream soversion issue.

Bug: AcademySoftwareFoundation/Imath#189 (comment)
Signed-off-by: Bernd Waibel <[email protected]>
waebbl added a commit to waebbl/gentoo that referenced this issue May 31, 2023
waebbl added a commit to waebbl/gentoo that referenced this issue May 31, 2023
Mask dev-libs/imath-3.1.7 due to upstream soversion issue.

Bug: AcademySoftwareFoundation/Imath#189 (comment)
Signed-off-by: Bernd Waibel <[email protected]>
waebbl added a commit to waebbl/gentoo that referenced this issue May 31, 2023
waebbl added a commit to waebbl/gentoo that referenced this issue May 31, 2023
Mask dev-libs/imath-3.1.7 due to upstream soversion issue.

Bug: AcademySoftwareFoundation/Imath#189 (comment)
Signed-off-by: Bernd Waibel <[email protected]>
freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this issue Jun 2, 2023
Note this reverts the .so file major version from 31 to 29,
but after some discussion, the AcademySoftwareFoundation people
seem to have revised the versioning policies and also did not
intentionally make breaking changes.
<AcademySoftwareFoundation/Imath#189>

It looks as though future releases should be less painful
WRT versioning.

Unfortunately that means we need to bump PORTREVISION and rebuild ports
again, and it's getting messy with respect to 2023Q2, which has
3.1.7 and major version 30...

Changelog:
https://github.com/AcademySoftwareFoundation/Imath/blob/v3.1.9/CHANGES.md#version-319-May-31-2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants