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

Binary-versioning regression #992

Closed
dmacks opened this issue Oct 16, 2023 · 10 comments · Fixed by #997
Closed

Binary-versioning regression #992

dmacks opened this issue Oct 16, 2023 · 10 comments · Fixed by #997

Comments

@dmacks
Copy link
Contributor

dmacks commented Oct 16, 2023

The change from autotools (libtool in particular) in libheif-1.15.1 to cmake retained the same SONAME of the library (we call it "install_name" on OS X), which seems correct as it appears only ABIs were added rather than removed or changed. But the compatibility_version got decremented (was 17.0.0 now 1.0.0 when I upgrade libheif to 1.17.0). It is not allowed for compatibility_version to be decreased for any specific install_name, or it will cause a runtime failure for any program that was linked against that install_name that had had a higher compatibility_version.

Libtool handles all these library versioning details automatically, via -version-info, but cmake does not know anything about this. Assuming I am reading the public APIs correctly, that symbols were added but no symbols were removed or changed, it's correct that install_name stays the same, but compatibility_version should be incremented.

@dmacks
Copy link
Contributor Author

dmacks commented Oct 16, 2023

In libheif/CMakeLists.txt:

if (APPLE)
    set_target_properties(heif
            PROPERTIES
	    LINK_FLAGS "-Wl,-compatibility_version,18.0.0")
endif ()

@bradh
Copy link
Contributor

bradh commented Oct 17, 2023

You can probably submit that as a pull request.

@farindk
Copy link
Contributor

farindk commented Oct 17, 2023

If I read the Apple docs and this comment, it appears to me that to correct values (ignoring anything that happened in the past) would be to set

  • current_version = 1.17.0
  • compatibility_version = 1.0.0
    Since we are still backwards compatible to the very first version.

What is current_version set to by default?
Doesn't it introduce problems if we have a current_version=1.17.0 and higher compatibility_version=17.0.0 ?

Couldn't we do a hard cut and set these values properly instead of shifting the issue further into the future?

It also appears that there is now cmake support for this.

@dmacks
Copy link
Contributor Author

dmacks commented Oct 17, 2023

compat=1.0.0 current=1.17.0 would be reasonable if starting from scratch, but compat must be monotonic for any given SONAME. So libheif seems stuck building on the libtool legacy until it jumps to the next major-version. The basis is libheif-1.15.1 giving compat=17.0.0 current=17.1.0.

My patch gives compat=18.0.0 current=1.17.0. That's a good point about "current < compat" being weird, but current doesn't seem to have a visible runtime effect in most cases. Neither compile-time linking (when building libheif-1.17.0) nor dyld (when running heif-info) gives any warning or error. Easy enough to upgrade the patch to give 17.0.0/17.2.0 or 17.0.0/17.17.0 or 18.0.0/18.17.0

I do not know much about cmake's features, so I'm glad there is a "cmake way" to do it instead of just forcing a linker flag.

@kmilos
Copy link
Contributor

kmilos commented Oct 17, 2023

My patch gives compat=18.0.0 current=1.17.0. That's a good point about "current < compat" being weird, but current doesn't seem to have a visible runtime effect in most cases. Neither compile-time linking (when building libheif-1.17.0) nor dyld (when running heif-info) gives any warning or error. Easy enough to upgrade the patch to give 17.0.0/17.2.0 or 17.0.0/17.17.0 or 18.0.0/18.17.0

The big issue w/ this is that one always has to remember the relationship and manually bump compat and curret together, or code an automated way in CMake to do this somehow. Maintainers are only human (for now), and they will forget to keep these in sync.

@dmacks
Copy link
Contributor Author

dmacks commented Oct 17, 2023

I agree that it's not great to set it manually. cmake is defaulting to compat=1.0.0 current=1.17.0, so should it be calculated to "+17"?

@farindk
Copy link
Contributor

farindk commented Oct 17, 2023

Probably this has to be set manually because cmake cannot know when there is an incompatible change. In practice, there will only be incompatible changes with the next major version, so this will essentially be a constant that is incremented at every major version. If this is commented in the CMakeLists.txt file right below the version number, this should be doable.

@farindk farindk reopened this Oct 17, 2023
@farindk
Copy link
Contributor

farindk commented Oct 17, 2023

I changed it to use "17.0.0". As far as I understood, it's no problem to keep it at the same number.

Do we also have to set current_version or is that automatic?

@farindk
Copy link
Contributor

farindk commented Oct 17, 2023

Libtool handles all these library versioning details automatically, via -version-info, but cmake does not know anything about this.

I checked this and apparently, cmake does indeed handle current_version and compatible_version automatically. Just in a different way than Autotools. If the target properties VERSION and SOVERSION are set correctly, cmake will take the information from there.
In our case, we have to set it manually because we have to differ from the cmake naming scheme.

@farindk farindk closed this as completed Oct 18, 2023
@kmilos
Copy link
Contributor

kmilos commented Oct 19, 2023

Just for my curiosity (and feel free to ignore and tell me to dig through the docs): how did the libtool scheme in 1.15.2 end up setting this to "17.0.0"?

In 1.15.2, there was

https://github.com/strukturag/libheif/blob/v1.15.2/configure.ac#L22-L24

which, through -version-info 16:2:15 results in the expected libheif.so.1 and libheif.so.1.15.2 artifacts on *nix (soversion of 1, which is current-age). I guess current_version on macOS matches this, but what did libtool on macOS do for compatible_version then to end up w/ 17.0.0 from -version-info 16:2:15?!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants