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

More granular SONAME versioning #345

Closed
kpcyrd opened this issue Sep 30, 2023 · 29 comments · Fixed by #352
Closed

More granular SONAME versioning #345

kpcyrd opened this issue Sep 30, 2023 · 29 comments · Fixed by #352

Comments

@kpcyrd
Copy link
Contributor

kpcyrd commented Sep 30, 2023

Related to rustls/rustls-ffi#345 and rustls/rustls-ffi#274.

I've tried to use cargo-c together with Arch Linux packaging tools, which is able to automatically detect and embed the SONAME into package metadata, but noticed I get this:

provides = librustls.so=0-64

After looking into how this works I noticed it's using a string embedded in the .so file:

% readelf -d /usr/lib/librustls.so | rg Lib
 0x000000000000000e (SONAME)             Library soname: [librustls.so.0]

I found the relevant part in the cargo-c codebase:

cargo-c/src/target.rs

Lines 81 to 85 in 0819def

if capi_config.library.versioning {
lines.push(format!("-Wl,-soname,lib{lib_name}.so.{major}"));
} else {
lines.push(format!("-Wl,-soname,lib{lib_name}.so"));
}

There are two things about this:

  1. if the version string is 0.11.0, I think the soname should be librustls.so.0.11
  2. even if the version string is >= 1.0.0, I think it should be possible to configure a major.minor (or even major.minor.patch) SOVER

Specifically with librustls I would like to be able to build it with -Wl,-soname,librustls.so.0.11.0 until the developers are comfortable making ABI commitments. :)

@lu-zero
Copy link
Owner

lu-zero commented Sep 30, 2023

You are right, we'll have a new cargo soon and I'd release your improvement/fix with it if is fine for you. If you prefer having it in a point release please tell me and I'll prepare it :)

@kpcyrd
Copy link
Contributor Author

kpcyrd commented Sep 30, 2023

Sounds great, thank you! :) Looking forward to the next cargo release.

@lu-zero
Copy link
Owner

lu-zero commented Oct 10, 2023

The new cargo is out, let me know if everything works as you need.

@radhermit
Copy link

radhermit commented Oct 10, 2023

This appears to break the default case where the soname version isn't specified in metadata.

For example, a library I'm building with cargo-c now has *.so.0.0 instead of *.so.0 for the SONAME and since cargo-c doesn't create the related *.so.0.0 file it breaks all the bindings that try to link to the nonexistent library.

@lu-zero
Copy link
Owner

lu-zero commented Oct 10, 2023

in your case the soversion should be lib{lib_name}.so.{major}.{minor}.{patch}, definitely more to fix.

@radhermit
Copy link

That makes sense, I didn't revert to the previous version to verify my guess.

@lu-zero
Copy link
Owner

lu-zero commented Oct 10, 2023

I'll try to refactor and cleanup a bit more since there is one more corner case:

version links soversion
0.0.n libfoo.so libfoo.so.0.0.n
0.m.n libfoo.so, libfoo.so.0.m libfoo.0.m
v.m.n libfoo.so, libfoo.so.v libfoo.v

This is what I'd expect, would it work for you?

@radhermit
Copy link

I believe that should work, thanks for looking into it.

@kpcyrd
Copy link
Contributor Author

kpcyrd commented Oct 11, 2023

Since no package in Arch Linux is using .so.0.0.X I went ahead and uploaded cargo-c 0.9.26, then did a coordinated rebuild for the changes in SOVER and moved the new packages to [extra].

rav1e now ships as librav1e.so.0.6 :)

I noticed the patch in #350 is still essential because otherwise it's still creating a librav1e.so.0 symlink instead of the required librav1e.so.0.6 symlink. The cargo-c package in Arch Linux contains this patch.

Besides that everything seems to be fine, thanks for working on this!

@jperkin
Copy link

jperkin commented Oct 12, 2023

This causes a regression in pkgsrc, at least on illumos platforms. It broke somewhere around 0.9.26, and I've just verified that it is still broken in 0.9.27.

For example, building libimagequant, I end up with the following library and symlinks:

$ ls -l libimagequant.so*
lrwxrwxrwx   1 root     root          22 Oct 11 20:18 libimagequant.so -> libimagequant.so.0.0.0
lrwxrwxrwx   1 root     root          22 Oct 11 20:18 libimagequant.so.0 -> libimagequant.so.0.0.0
-rwxr-xr-x   1 root     root     3008816 Oct 11 20:18 libimagequant.so.0.0.0

However, the SONAME refers to a nonexistent link:

$ elfdump libimagequant.so.0.0.0 | grep SONAME
       [9]  SONAME            0x76a1e             libimagequant.so.0.0

This causes problems with linking against this library, as can be seen during the build of graphics/gd where we check that all library paths are valid before packaging:

https://us-central.manta.mnx.io/pkgsrc/public/reports/trunk/tools/20231012.1136/gd-2.3.3nb10/install.log

This was working fine with older releases, so if you could resolve the regression that'd be great. I'm happy to test any proposed changes. Thanks!

@jperkin
Copy link

jperkin commented Oct 12, 2023

Hmm, there's a chance that libimagequant package wasn't rebuilt correctly against the newer cargo-c release. Sorry, let me double check and I'll verify that it was built using 0.9.27.

@jperkin
Copy link

jperkin commented Oct 12, 2023

Ok, so after ensuring it was rebuilt with 0.9.27 it does appear as though the SONAME is now fixed:

$ ls -l libimagequant.so*
lrwxrwxrwx   1 root     root          22 Oct 12 12:29 libimagequant.so -> libimagequant.so.0.0.0
-rwxr-xr-x   1 root     root     3008816 Oct 12 12:29 libimagequant.so.0.0.0

$ elfdump libimagequant.so.0.0.0 | grep SONAME
       [9]  SONAME            0x76a1e             libimagequant.so.0.0.0

though there is now no longer a libimagequant.so.0 symlink, so any package using cargo-c in pkgsrc will need to be changed to remove those from the packaging list. Something to be aware of that may break other packaging systems.

Thanks though for fixing the original regression, and apologies for the incorrect original comment.

@kpcyrd
Copy link
Contributor Author

kpcyrd commented Oct 12, 2023

@jperkin I've investigated this and it's unrelated to cargo-c, I've submitted a patch to libimagequant.

@kornelski
Copy link

Version in libimagequant is intentional. I do not want to create libimagequant.so.4, because the library has the same ABI as ever, so it's compatible with previous C's libimagequant.so.0.

@lu-zero
Copy link
Owner

lu-zero commented Oct 12, 2023

Could you make it 0.1 then?

@kornelski
Copy link

I've set it to version = "0.0.4". I hope that helps.

@lu-zero
Copy link
Owner

lu-zero commented Oct 13, 2023

I'm setting the soname to fit the semver so:

version expectation links made (by cargo-c or ldconfig)
0.0.x no compatibility expected Symlinks only .so
0.x.y bumping x breaks compatibility, bumping y does not Symlinks .so and .so.0.x
x.y.z bumping x breaks compatibility, bumping the rest does not Symlinks .so and .so.x

@kyrias
Copy link

kyrias commented Nov 10, 2023

Imposing a notion of semver into soname versions is a bit strange and unexpected, as this completely violates how shared C library versioning normally works. I feel like this needs to be very clearly and obviously documented.

@kornelski The above means that making a release with the version set to 0.0.5 would be a breaking change since things linked against libimagequant will now explicitly link to libimagequant.so.0.0.4 rather than the expected libimagequant.so.0.

@kyrias
Copy link

kyrias commented Nov 10, 2023

(And for the record, it also means that it's explicitly not compatible with the earlier libimagequant.so.0, because cargo-c now only creates a single symlink, libimagequant.so -> libimagequant.so.0.0.4. Meaning that anything linked against either the C version or the Rust version built with earlier cargo-c versions won't be able to find the libimagequant.so.0 symlink they expect to exist.)

@sdroege
Copy link
Sponsor Contributor

sdroege commented Nov 10, 2023

The above means that making a release with the version set to 0.0.5 would be a breaking change since things linked against libimagequant will now explicitly link to libimagequant.so.0.0.4 rather than the expected libimagequant.so.0.

That's also the (cargo) semver meaning of the versions though. 0.0.5 is incompatible with 0.0.4.

@lu-zero
Copy link
Owner

lu-zero commented Nov 10, 2023

Imposing a notion of semver into soname versions is a bit strange and unexpected, as this completely violates how shared C library versioning normally works. I feel like this needs to be very clearly and obviously documented.

Semver is fairly clear on what to expect, so if you do otherwise you aren't following semver.

(And for the record, it also means that it's explicitly not compatible with the earlier libimagequant.so.0, because cargo-c now only creates a single symlink, libimagequant.so -> libimagequant.so.0.0.4. Meaning that anything linked against either the C version or the Rust version built with earlier cargo-c versions won't be able to find the libimagequant.so.0 symlink they expect to exist.)

I asked for a version 0.1 so you can have point releases that do not have breaking changes exactly for that reason.

@kyrias
Copy link

kyrias commented Nov 10, 2023

@sdroege:
That's also the (cargo) semver meaning of the versions though. 0.0.5 is incompatible with 0.0.4.

Sure, but it's strange and unusual to impose this onto the concept of ABI soname versions, which are intentionally wholly separate from crate versions, without documenting this.

@lu-zero:

Imposing a notion of semver into soname versions is a bit strange and unexpected, as this completely violates how shared C library versioning normally works. I feel like this needs to be very clearly and obviously documented.

Semver is fairly clear on what to expect, so if you do otherwise you aren't following semver.

<snip>

I asked for a version 0.1 so you can have point releases that do not have breaking changes exactly for that reason.

  1. I'm saying that the fact that the concept of SemVer being enforced on the ABI version needs to be documented, not that you specifically need to document what SemVer implies, because using SemVer like this on soname versions is wholly foreign to how C-ABI shared libraries normally work, and the way cargo-c is currently branded it appears to be meant to provide you with a way of creating standard C-ABI shared libraries, and thus this behavior is unexpected.

  2. This notion of 0.1.0 being compatible with 0.1.1 is a Cargo-thing and you won't find it in the SemVer spec at all, and so by choosing this behavior you have in fact decided to not follow SemVer proper.

  3. While using 0.1 would have solved the immediate issue of the missing .so symlink, that would still lead to a breaking .so change for the library in question! Software built against the new version would not run with the older ABI-compatible version installed due to being linked against libimagequant.so.0.1 rather than libimagequant.so.0.

@kpcyrd
Copy link
Contributor Author

kpcyrd commented Nov 10, 2023

Assuming this is about https://bugs.archlinux.org/task/80209, this package was initially missed in https://archlinux.org/todo/cargo-c-sover-rebuild/ because it's >= 1.0.0 and assumed to be unaffected by this change (just like libdovi). By the time I noticed imagequant-sys/Cargo.toml is overwriting the cargo-c specific version with 0.0.0 it didn't make much sense to include it in the rebuild because the SONAME would change from libimagequant.so.0.0.0 to libimagequant.so.0.0.4 in the next release anyway (which the dynamic linker considers a breaking change).

The concept of .so.0 doesn't make much sense in the Rust world which uses semver to indicate breaking changes, instead of using an incompatibility-counter. This concept is well understood among Rust devs. The dynamic linker doesn't care about this at all, it considers SONAME an opaque value that either matches or doesn't match. Whether you use libfoo.so.5 or libfoo.so.0.5 or libfoo.so.twelve does not make any difference. The old .so.0 behavior was arguably broken because it looks like it understands SONAME versioning, while it actually blindly throws everything < 1.0.0 in the same .so.0 bucket and blanket-assumes ABI compatible between them (even if the developer is attempting to indicate incompatibility through Rust-native versioning), possibly resulting in memory corruption issues.

There are no concerns about libimagequant.so.0.0.4 and the absence of a symlink because a change to libimagequant.so.0.0.5 would be intentional by upstream to indicate ABI incompatibility. A symlink is only needed if the .so contains the full release version, instead 4.2.2 and 4.2.3 would both contain a libimagequant.so.0.0.4 file instead, and the ELF header records NEEDED libimagequant.so.0.0.4 instead of relying on a symlink.

@kyrias
Copy link

kyrias commented Nov 10, 2023

The concept of .so.0 doesn't make much sense in the Rust world which uses semver to indicate breaking changes, instead of using an incompatibility-counter. This concept is well understood among Rust devs.

It's indeed a well-understood concept among Rust devs for Rust crates.

But we're not talking about Rust crate ABIs here, we're talking about C-ABI shared libraries, and if the intention is to build libraries to be used from languages other than Rust then this line of reasoning doesn't make any sense because it's just not how C-ABI shared libraries normally work.

There are no concerns about libimagequant.so.0.0.4 and the absence of a symlink because a change to libimagequant.so.0.0.5 would be intentional by upstream to indicate ABI incompatibility.

Not if we're talking about C-ABI shared libraries, which appears to be the claimed purpose of cargo-c.

@lu-zero
Copy link
Owner

lu-zero commented Nov 10, 2023

When you break the API you always break also the ABI, I'm not sure what's the problem that you are arguing about so vehemently.

@kyrias
Copy link

kyrias commented Nov 10, 2023

I'm saying that for C-ABI shared libraries the switch from 0.0.4 to 0.0.5 IS NOT breaking either the API or the ABI.

Hard-coding the current policy that it is a breaking change is imposing a notion that does not exist in the C-ABI shared library ecosystem, which makes it difficult to use cargo-c for projects that actually care about the C-ABI shared library ecosystem to express the versioning policy that they need to, such as libimagequant above which now cannot express the version they need to retain the compatibility that they're trying to guarantee.

@lu-zero
Copy link
Owner

lu-zero commented Nov 10, 2023

For any software that follows semver, it means exactly that.
I'm not sure why somebody wants to opt out semver, but if you want to add an override option I welcome the patch.

(but I would be quite displeased as consumer of such theoretical library)

@sdroege
Copy link
Sponsor Contributor

sdroege commented Nov 10, 2023

I'm saying that for C-ABI shared libraries the switch from 0.0.4 to 0.0.5 IS NOT breaking either the API or the ABI.

For C shared libraries, versions have no meaning at all in general and every library does its own thing. But this here is about mapping a cargo crate version to a soname, and cargo crate versions have a very specific meaning.

@kpcyrd
Copy link
Contributor Author

kpcyrd commented Nov 10, 2023

@kyrias I think you're mixing up the C ABI (a calling convention for assembly) with dynamic linking (load a shared object by name). Both are not unique to C and can be implemented/integrated in most languages (like cargo-c did). Neither system has a concept of "you MUST use .so.0, .so.1, .so.2", this is simply what most C projects ended up doing. This SONAME is controlled by upstream (both in C and Rust world) and cargo-c provides two interfaces to configure this, either by auto-detecting it from the crate version [package.version] or by overwriting it through [package.metadata.capi.library.version]. Every consuming ecosystem (Arch Linux, pkg-config, the dynamic linker) understands the convention for compatibility is "if the SONAME changes, you need to rebuild". If you are an upstream developer using cargo-c you are expected to understand how the cargo-c interface for SONAME works.

What is a valid concern though, I think, if I have a C project that currently uses .so.0 and I want to migrate to cargo-c, 100% reimplement the current C binary interface in Rust and indicate a .so.0 SONAME since the ABI is identical. As-of today it's possible to reimplement the ABI but not to configure .so.0 natively in cargo-c. I think it's a valid feature request to support this, but there's nothing inherently wrong with using libfoo.so.0.17 instead of libfoo.so.17. Ideally the crate is strictly used for cargo-c instead of doubling as a Rust library, so you would simply publish 0.17.1 for ABI non-breaking changes and 0.18.0 for ABI breaking changes. I think this is fairly straight forward and easy to understand. If the crate also needs to be a Rust crate you could maintain a second version string like:

[package.metadata.capi.library]
version = "17.0.0"

To get .so.17 or .so.1 or .so.2 with the only exception it's currently not possible to specify .so.0.

When planning this feature we should also take into account there's also the librustls use-case of "I don't want to commit to any ABI and every new version should be considered ABI breaking (for safety reasons)".

To support both, there could be a setting like:

  • None: default/auto-detect, current behavior
  • Some(1): always use .so.{major}
  • Some(2): always use .so.{major}.{minor}
  • Some(3): always use .so.{major}.{minor}.{patch}

This setting should be in [package.metadata.capi.library].

I think this is more of an oversight instead of "cargo-c not caring about ABI compatibility", and also doesn't have much to do with the Arch Linux bugreport, except it would've allowed the libimagequant project to specify in their Cargo.toml that they want to keep using libimagequant.so.0 (until they decide they want something else).

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.

7 participants