-
Notifications
You must be signed in to change notification settings - Fork 767
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
Fix SOVERSION computation logic in CMake to match libtool's #1976
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was speaking to a friend a few months back about the different DSO across autotools/cmake builds. Big thanks for taking a stab at this.
Few things worth* nothing:
- decreasing the DSO version is usually a bad idea, although
- Arch, Fedora, Gentoo, Debian - and maybe more linux distros - use the autotools build
- others would need to either rebuild or have a local symlink hack until they can rebuild
With this PR my Arch box shows identical SONAME (yay) although the filename still differs libarchive.so.13.7.3 (autotools) vs libarchive.so.13 (cmake). Can we fix that as well please?
Looking at the PR itself - please squash the commits into one.
I think last commit fixes that, but it feels a bit hacky? Not sure if there's a better way. Note this produces the same files in both platforms: Linux:
macOS:
Not sure if we want the "full soversion" in macOS too, but there's a symlink so 🤷 |
e30b612
to
b62f5f3
Compare
Confirmed that it fixes that over here.
IMHO that's fine. |
b62f5f3
to
a7b6999
Compare
Reiterating: the library version number can NEVER decrease. Making the version numbers the same is a good idea, but that must be done by increasing the lower value. |
Updating the autotools version will lead to massive rebuilds in most linux distros. They are well suited for that, although I would request that maintainers add a clear NOTE either way - cmake downrev or autotools uprev - in the release notes. Thanks in advance |
Co-authored-by: isuruf <[email protected]>
a7b6999
to
6a360ff
Compare
@theta682 the existing CMakefile already has a mix of lower/uppercase so, I'm not sure if highlighting/fixing every instance is helpful here. The indirect poke on the other hand is greatly appreciated. @kientzle @mmatuska can we help get this (or variant thereof) merged? AFAICT @kientzle's comment have been addressed a while back. |
Co-authored-by: Timothy Lyanguzov <[email protected]>
Closes #1857
It also adds
MACHO_{COMPATIBILITY,CURRENT}_VERSION
as recommended in CMake docs to match libtool's output. However, these properties require CMake >=3.17. Is it ok to bump it?Before this PR
Given libarchive 3.6.2, autotools produces:
CMake gives:
Note:
After this PR
Both systems should produce the same install name, compatibility and current versions: