linker: Link dylib crates by path

[petrochenkov]

Linkers seem to support linking dynamic libraries by path.
Not sure why the previous scheme with splitting the path into a directory (passed with -L) and a name (passed with -l) was used (upd: likely due to #126094 (comment)).

When we split a library path some/dir/libfoo.so into -L some/dir and -l foo we add some/dir to search directories for all libraries looked up by the linker, not just foo, and foo is also looked up in all search directories not just some/dir.
Technically we may find some unintended libraries this way.
Therefore linking dylibs via a full path is both simpler and more reliable.

It also makes the set of search directories more easily reproducible when we need to lookup some native library manually (like in #123436).

[rustbot]

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[petrochenkov]

Sigh, on Linux if a dynamic library A is linked into a binary B via path, then that entire path gets into B's DT_NEEDED.
Probably need to specify soname for dylib crates to address this.

[petrochenkov]

@bors rollup=never
@rustbot ready

[petrochenkov]

In the meantime let's test on some more targets.

[petrochenkov]

@rustbot ready

[petrochenkov]

@bors r=michaelwoerister

[bors]

📌 Commit 565ddfb has been approved by michaelwoerister

It is now in the queue for this repository.

[bors]

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

[bors]

⌛ Testing commit 565ddfb with merge 1086aff...

[bors]

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 1086aff to master...

[rust-timer]

Finished benchmarking commit (1086aff): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -3.1%, secondary -2.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.1%	[-3.1%, -3.1%]	1
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	-3.1%	[-3.1%, -3.1%]	1

Cycles

Results (secondary -2.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.4%, -2.1%]	2
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 726.158s -> 706.903s (-2.65%)
Artifact size: 327.58 MiB -> 327.56 MiB (-0.01%)

[cuviper]

Dropping a quick reference here, the new "cdylib" SONAME is causing unwanted rpm Provides:
https://bugzilla.redhat.com/show_bug.cgi?id=2314879

I will probably file an issue about this, if nobody beats me to it, although I'm not certain what the resolution should be. On the rpm side, it's also possible to filter out such provides, but it was unexpected that this might be needed.

[nirbheek]

The basic problem is that Cargo doesn't have a way to designate a target as a "module" instead of a "library". It just has "cdylib", which is supposed to be linked. Modules should not be linked (except on Android).

For example, in CMake, you have "shared library" and "module library" as separate concepts. Similarly in Meson you have shared_library() and shared_module().

There are a number of differences between the two, not just SONAME, for example undefined symbols are allowed in
modules. However, cdylib is all that was available, so it was used for modules too. We saw this in the early years of Meson as well, when everyone started using shared_library(), and we had a transition period to shared_module(), which involved people fixing their usage.

Also, answering some suggestions in that bug, I think it is a mistake to pawn-off these duties to build.rs or cargo-c:

build.rs is an anti-pattern, people should not need to use code to build code. 99% of use-cases should be covered by something declarative, and the 1% custom needs should be code. With build.rs right now if you have to interop with C, 99% of cases need code to build code.

cargo-c is a great project, and I want more people to use it, but this problem cannot be solved in cargo-c, we already tried it and the cargo-c maintainer reviewed this PR.

[petrochenkov]

Setting soname for cdylibs wasn't strictly necessary for this PR (I asked about this in #126094 (comment)).
We can set it just for Rust dylibs, that would only be a minor nuisance (passing an extra argument around).

[decathorpe]

The problem is that all uses of cdylib targets built by rustc we have in Fedora are either

• built with cargo-c or similar tools and already set soname explicitly because they're used as actual shared libraries, or
• built with maturin / setuptools_rust and expect the soname not to be set because they're "modules", not "libraries".

So the fact that this behaviour was flipped from "not set unless set explicitly" in Rust itself to "set always" breaks the expectations for the second case, and it's apparently not easy to fix "in post" because patchelf apparently doesn't support removing headers, only overriding them, so I don't even think maturin or setuptools_rust could fix this on their side.

[nirbheek]

@lu-zero what do you think about setting soname here? Should we revert that part of the change?

[lu-zero]

For maturin and other systems that generate unversioned modules probably reverting that would be the simplest solution.

Generally would be great to have a simpler mean to set/unset the value and in the long run would be even better to be able to say cdylib and cmod to Cargo.

[cuviper]

We can set it just for Rust dylibs, that would only be a minor nuisance (passing an extra argument around).

I've attempted this in #130960.