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

rustc-link-arg does not propagate transitively #9554

Closed
tronical opened this issue Jun 7, 2021 · 13 comments
Closed

rustc-link-arg does not propagate transitively #9554

tronical opened this issue Jun 7, 2021 · 13 comments
Assignees
Labels
A-build-scripts Area: build.rs scripts A-linkage Area: linker issues, dylib, cdylib, shared libraries, so C-bug Category: bug disposition-close finished-final-comment-period FCP complete T-cargo Team: Cargo to-announce

Comments

@tronical
Copy link

tronical commented Jun 7, 2021

Problem
With #9523 the following use-case does not compile anymore with nightly. It works with current stable.

  1. A -sys crate links against an external library using cargo:rustc-link-lib/cargo:rustc-link-lib-framework in its build.rs. The crate is to be consumed by a cdylib crate. To make consumption easy, build.rs of the -sys crate also prints out cargo:rustc-cdylib-link-arg=-Wl,-rpath,/some/path.
  2. The cdylib crate depends on the -sys crate and the cdylib is linked in a way that the external library becomes a dependency for the dynamic linker and the rpath is set due to the propagation of rustc-cdylib-link-arg from the -sys crate to the final cdylib crate.

With stable rust this works as described, with nightly this now produces a build error when compiling the -sys crate due to the use of rustc-cdylib-link-arg in the non-cdylib -sys crate.

Concrete example:

Building the qttypes crate from https://github.com/woboq/qmetaobject-rs/tree/master/qttypes produces the error that #9523 introduced:

$ cargo +nightly build
   Compiling qttypes v0.2.1 (/Users/simon/src/qmetaobject-rs/qttypes)
error: invalid instruction `cargo:rustc-cdylib-link-arg` from build script of `qttypes v0.2.1 (/Users/simon/src/qmetaobject-rs/qttypes)`
The package qttypes v0.2.1 (/Users/simon/src/qmetaobject-rs/qttypes) does not have a cdylib target.

Possible Solution(s)

Allow rustc-cdylib-link-arg again in non-cdylib crates to avoid breaking their build with the next stable release.

Notes

Output of cargo version:

cargo 1.54.0-nightly (0cecbd6 2021-06-01)
cargo 1.52.0 (6976741 2021-04-21)

@tronical tronical added the C-bug Category: bug label Jun 7, 2021
@joshtriplett
Copy link
Member

@tronical We discussed this in the @rust-lang/cargo meeting today.

The behavior on stable wasn't intended to pass the flags to any package other than the current one. Up till 1.50, it was silently ignored. From 1.50 to 1.52, it was incorrectly propagating to the final linking package. On nightly, an error was added to indicate that the package doesn't have a cdylib. The behavior on stable wasn't known at the time.

A crate shouldn't be able to silently affect the link arguments of the crate depending on it; the crate depending on it should have to opt into that by calling a dependency-provided function from its own build script or similar. (Such a function could reference metadata provided by the dependency, just as it could currently reference such metadata to find things like C include paths.)

In general, rpath is something that a dependency can't safely set for a crate that depends on it; that might work for a specific cooperating set of crates, but not for an arbitrary dependency. Different crates may want to set rpath in different ways. (And, for that matter, different builders/distributors of executables may want to set it differently; for instance, Linux distributions typically do not want rpath set.)

So, rustc-cdylib-link-arg shouldn't affect the linker invocations of depending crates. To avoid breaking builds, we will make this a warning rather than an error for now, though we may make it an error in the future. We suggest altering your build scripts to only pass rustc-cdylib-link-arg within the package that contains the cdylib.

@ehuss ehuss self-assigned this Jun 9, 2021
@ehuss ehuss added A-linkage Area: linker issues, dylib, cdylib, shared libraries, so A-build-scripts Area: build.rs scripts labels Jun 9, 2021
@ogoffart
Copy link

ogoffart commented Jun 9, 2021

A crate shouldn't be able to silently affect the link arguments of the crate depending on it;

Why not?
For example, in C++ with cmake, this is quite typical to do target_link_options with INTERFACE and the link flags are propagated to all the dependees.
I would say that this is a valid use case to add some flags from the -sys crate, so that everything is setup for any crate using it.
I wouldn't call this behavior "silent".

the crate depending on it should have to opt into that by calling a dependency-provided function from its own build script or similar.

That's quite an overkill workaround. I've seen crates doing that (e.g, neon) but i guess it's only because it did not work with previous versions.
Asking every crate at the bottom of the tree to use a build script and depending on a build crate because one crate deep in the hierarchy might need a specific flag is quite not convenient when one could just tell cargo to do the right thing with something like rustc-cdylib-link-arg

In general, rpath is something that a dependency can't safely set for a crate that depends on it [...]. Different crates may want to set rpath in different ways.

This is off topic, but i was under the impression rpath are additive, so that different crates just append all the rpath they need.

(And, for that matter, different builders/distributors of executables may want to set it differently; for instance, Linux distributions typically do not want rpath set.)

That's true, but on the other hand, we added the rpath on our project because people filling github issues about our crate "not working" as it couldn't load shared libraries. We thought rpath was a good way to fix this problem to get a better out-of-the-box experience. Even of packagers, rpath doesn't hurt, does it?

@joshtriplett
Copy link
Member

I would say that this is a valid use case to add some flags from the -sys crate, so that everything is setup for any crate using it.

Linker options can conflict or create various issues, and may break if the depending crate is doing something non-trivial with linking, or if they have different needs than one of their dependencies might expect.

And there's no way for the depending crate to control or filter out those options, if they're propagated from a dependency. In many cases, the depending crate may not even see those options; nothing will even display them unless you verbosely display your link line or the output of dependency build scripts.

Also, where to load shared libraries from can be a very project-specific or distribution-specific problem of release engineering. Binaries may be built on one system, packaged up, and deployed to other systems, with shared libraries in a different place than they were on the build system. In general, the -sys crate can't know the right solution for many use cases.

If we were going to support this, I could imagine doing so by allowing crates to declaratively (without using a build.rs script) state that they want to use link flags from a specific dependency's metadata (DEP_THEDEPNAME_LINK_FLAGS, for instance).

But beyond that, rpath is one of many things that we should really have a standard solution to. People should be able to make a single top-level decision like "I want rpath set in this common way" or "I want rpath set in this uncommon way" or "I don't want rpath set at all", and then crates supplying shared libraries should be able to give cargo enough information that it could set that consistently with the user's preferences and those of the top-level crate.

This is off topic, but i was under the impression rpath are additive, so that different crates just append all the rpath they need.
Even of packagers, rpath doesn't hurt, does it?

It can hurt, for various reasons. https://wiki.debian.org/RpathIssue gives one explanation. An rpath may also cause libraries to be loaded from an unexpected path; that may have security implications, or may break if there are other libraries in that path. It's important for people building crates to be able to control where they load libraries from.

@tronical
Copy link
Author

Perhaps the -sys crate can't know the right solution for many use-cases, but I believe that it should be in a position to make some decisions and it can take "user input" into account. If the consumers of the crate have to express that they want to use link flags from a dependency's meta-data and it also needs to be specified by the consumers of the consuming crate, then it becomes hard to use. We want the crate that we are developing to be easy to use. It's okay if the build fails, but it's bad if everything compiles but the program can't be started because the dynamic linker can't find a library that the regular linker clearly found at build time.

The feature that build systems like CMake offer is the encapsulation, that a CMake target can be used without having to know what other things it depends on. If I create a library with CMake and say that it needs this particular linker flag, then I can be sure that in a dynamic build the library will be linked with the flag and in a static build that flag will be forwarded until a shared library (or regular binary) is created. The only equivalent that I can think of from before that were lib tool archives.

I agree that built-in support for rpath in Cargo would be awesome, combined with a single top-level decision as you pointed out. It would be great to have a solution in the meantime, which is what rustc-cdylib-link-arg seems to offer - at the risk of being imperfect.

In any case, thanks for considering this issue and our input :-). It's much appreciated that you plan on making this a warning again, instead of an error.

@ehuss ehuss changed the title rustc-link-arg validation breaks link-arg propagation from -sys crates rustc-link-arg does not propagate transitively Jul 2, 2021
@ivmarkov
Copy link

ivmarkov commented Jul 4, 2021

Given that this issue was recently closed as a duplicate of this one, let me state that the scope of the problem is not only about passing an rpath from a -SYS crate down to a 'cdylib' crate.

We would like to pass - transitively of course - ALL sorts of linker args from a -SYS crate all the way down to a binary (as opposed to cdylib) crate.

The above is very, very useful in the embedded world when the SYS crate is modelled to represent the unsafe bindings to the Vendor-provided SDK for the MCU, as well as the vendor-specific linker args (including custom linker scripts and whatnot) that have to be passed through to the binary crate.

@kaspar030
Copy link

Just to chime in: This also broke our build, in embedded context. In RIOT-rs, one crate compiles and links a static library (from C code). that crate also needs to link in newlib (-lc_nano). Previously, with "-Zextra-link-args", that worked fine. Not anymore.

@joshtriplett joshtriplett added the T-cargo Team: Cargo label Jul 26, 2021
@joshtriplett
Copy link
Member

Opening a poll to check consensus among the rest of the team.

Should we confirm that the current behavior is correct, and dependencies shouldn't be able to affect the linker command-line of the depending crate without some kind of explicit opt-in from the depending crate?

@rfcbot close

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 26, 2021

Team member @joshtriplett has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-close final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Jul 26, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Jul 26, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@Lokathor
Copy link

My final comment is that this is poor for the embedded space where the API crate for a particular device, which includes the necessary linker script for said device, can't send the linker script along to binaries built for the device that depend on the API crate.

@ivmarkov
Copy link

ivmarkov commented Jul 28, 2021

I do agree with @Lokathor that not having any form of transitive arguments passing from the API/SYS crate to the binary one is very suboptimal. At the same time, I also agree that such a scheme would ideally require an explicit opt-in from the binary crate.

My only concern is that if the current restricted scheme is stabilized in its current form, future transitive + explicit opt-in extensions might take a lot of time to stabilize or not receive enough attention for that to happen at all in the foreseeable future.

@joshtriplett
Copy link
Member

joshtriplett commented Jul 28, 2021

To be clear, I'm quite amenable to proposals for opt-in linker arguments; I just wouldn't want them to happen implicitly without the explicit opt-in from the top-level crate.

We also already have the DEP_ mechanism, which is enough to support manually propagating these arguments via build.rs. Any further mechanism would be to make that more automatic and declarative without requiring a build.rs in the top-level crate.

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 5, 2021

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@rfcbot rfcbot added finished-final-comment-period FCP complete and removed final-comment-period FCP — a period for last comments before action is taken labels Aug 5, 2021
branchseer added a commit to branchseer/rust-nodejs that referenced this issue Oct 29, 2021
@ehuss ehuss closed this as completed Jan 15, 2022
Ectras added a commit to Ectras/rust-hptt that referenced this issue Jan 25, 2024
linker-args do not propagate transitively
(rust-lang/cargo#9554). Hence, -fopenmp would
need to be set in every crate that uses this crate.
The openmp-sys crate instead searches for the libraries and adds them
as link-lib, which is picked up by crates using this crate.
jacob-hughes added a commit to jacob-hughes/alloy that referenced this issue Mar 4, 2024
This is useful for debugging, where profilers intercept and override
dynamically linked symbols (e.g. `malloc` and `free`).

As of this commit, there is no way to build a custom BDWGC fork and have
that dynamically link to a program compiled with Alloy automatically.
This is because `cargo` does not support bubbling up linker args to the
final linked binary, so there is no way to programmatically set the
rpath for BDWGC to the OUT directory in `library/bdwgc` [1].

Alloy programs must therefore set the `LD_PRELOAD` environment variable
to prevent it from linking against the system libgc.

[1]: rust-lang/cargo#9554
jacob-hughes added a commit to jacob-hughes/alloy that referenced this issue Mar 4, 2024
This is useful for debugging, where profilers intercept and override
dynamically linked symbols (e.g. `malloc` and `free`).

As of this commit, there is no way to build a custom BDWGC fork and have
that dynamically link to a program compiled with Alloy automatically.
This is because `cargo` does not support bubbling up linker args to the
final linked binary, so there is no way to programmatically set the
rpath for BDWGC to the OUT directory in `library/bdwgc` [1].

Alloy programs must therefore set the `LD_PRELOAD` environment variable
to prevent it from linking against the system libgc.

[1]: rust-lang/cargo#9554
jacob-hughes added a commit to jacob-hughes/alloy that referenced this issue Apr 23, 2024
This is useful for debugging, where profilers intercept and override
dynamically linked symbols (e.g. `malloc` and `free`).

As of this commit, there is no way to build a custom BDWGC fork and have
that dynamically link to a program compiled with Alloy automatically.
This is because `cargo` does not support bubbling up linker args to the
final linked binary, so there is no way to programmatically set the
rpath for BDWGC to the OUT directory in `library/bdwgc` [1].

Alloy programs must therefore set the `LD_PRELOAD` environment variable
to prevent it from linking against the system libgc.

[1]: rust-lang/cargo#9554
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts A-linkage Area: linker issues, dylib, cdylib, shared libraries, so C-bug Category: bug disposition-close finished-final-comment-period FCP complete T-cargo Team: Cargo to-announce
Projects
None yet
Development

No branches or pull requests

8 participants