-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 dylib linking issues within the project #3408
Conversation
While the name is equal to the root, we still should produce a dynamic one; otherwise the link will fail.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Thanks for the PR! The original intention of this logic was to pass That being said, I don't think that unconditionally applying In that sense, could you elaborate more on the use case you have here? Do you intentionally want to link to the standard library, and if so how come? |
This is the case that I'm building a plugin system; and I leave the name of both the rustc cannot handle dylibs that aren't compiled with |
Indeed rust-lang/rust#34909 is the root problem, however I believe the original addition is also for the workaround purpose. Anyway, this is a correct workaround, and probably #1501 can be reverted then too. |
Hm so in general rustc doesn't support plugins very well. If you'd like to work with a plugin system, right now the only real guaranteed way to do that is to work through a C ABI. This PR is unfortunately a breaking change to any crate using |
I believe dylib compiled without dynamic flag doesn't link to the executable at all(rust-lang/rust#34909); then how can this be a breaking change? |
Oh right yeah but there are many dylibs which aren't linked to Rust executables, but rather compiled as dylibs to be used with dlopen or linked to a C project, for example. (and that's what this'll be breaking) |
That is what cdylib is for. I feel dylib is more suitable for pure-Rust projects. |
@alexcrichton here's a ping. dylib is intended to use with Rust projects; |
@ishitatsuyuki yes nowadays |
I found this issue/PR looking for a way to make it so that if I build a library with Is that what's expected here? |
@jsgf I personally fee like that's a separate issue, mind opening a separate tracking issue for that? |
I agree with @jsgf, but I'd also like to separate the job. Anyway, I'm pretty sure that this one is safe. @alexcrichton can you give a review? This doesn't break things, since most dylib users have been doing this with RUSTFLAGS. |
@ishitatsuyuki I've given this a review in the sense that we basically cannot merge this as is. Historical crates which should use cdylib today may still be using dylib. As a result this is a breaking change, and if we were to merge it then not nearly enough preparation has been done to canvas the community. |
@alexcrichton Please give me a specific case that this could break things. Even rustc dynamically links everything including std (or it won't link at all). This PR is solving a broken behavior, not adding a feature. |
A simple hello world should break:
Before this PR there is no dynamic dependency on libstd, after there is. |
Is that a breakage? Wouldn't that still work? |
@jsgf it's not breakage in the sense that it still compiles, but it's breakage in how |
It's not always correct to block breaking changes; the current behavior is quite annoying (I have to export If you focus on the breakage, please add a crate that is utilizing this behavior. |
@ishitatsuyuki I'm sorry I'm not sure I quite follow. Do you agree that this is a breaking change? Do you think we should ignore that fact and land this anyway? I agree that the current behavior is unfortunate, but I believe this is a breaking change and we cannot simply ignore breaking changes. |
@alexcrichton - On the other hand, I can't think of a good reason to link libstd statically. It's very brittle (you either end up with multiple copies of libstd in the process at once, or you end up with linkage errors if it occurs), and... no upside? Also, I'm not sure that having separate dylib and cdylib is a very good idea in the large; it might be just better to make dylib work as both a Rust or C dependency. At the moment, if you're using dynamic linking and you have a crate that's a dependency for both Rust and C code, you'll get two instances of it: one as dylib and one as cdylib. To work around this you'd need a second crate to present a C ABI interface to the C code which then has a dependency on the Rust ABI crate, which sounds pretty awkward. (I've implemented generating cdylib crates in our build system when C has a Rust dependency, but it never seems to quite do what I expect - but I haven't really looked at it yet, since we don't have an immediate need for it.) |
cdylib sucks: it only works when you only load one type of rust library, otherwise globals are broken. The same applies to "static" dylibs, which is broken by design. There's a long road to have good dynamic loading, but we should make it at least "working". |
Ok so I think it may help to step back here a second and see what's going on. Today I believe there are three primary use cases for Rust dynamic libraries (detailed below). All of these are definitely very important use cases for Rust/Cargo to support!
So long ago Rust only had one crate type, So this went on for a time and meant that we could satisfy use cases (1) and (2) above. With (1) you just compile a dylib without Fast forward again in time and we get to a point where Ok so why does Cargo pass So that brings us to today. Cargo supports (1) with So where did case (2) and case (3) go? Case (3) has been brought up a number of time with distros, but we're not even ready as a language/compiler so I don't think now's the right time to start preparing Cargo to work. Case (2), however, has never had real support in Cargo. Cargo, in general, has never had great support for configuring how dependencies are compiled (other than compiler flags). For example, crates shouldn't be deciding what crate types they are when consumed by others! My belief is that if we want to support case (2) we need a radically different solution than in this PR. Such a solution would, for example, be global configuration to compile everything with Does that help clear things up? @jsgf does that sound like an accurate summary for (2) where I think you find yourself? @ishitatsuyuki do you have a separate case than the three above for dylibs? Or does your use case slot into one of those three and is explained above? |
Our use-case is currently (2). The goal is to speed up build+link time for the dev cycle, and mostly because its what we do for C++; I haven't measured how much difference it makes for pure Rust code. (1) is also something I hope we get to soon, initially for things like writing Python extensions in Rust. But I feel there's also an opportunity for incrementally sneaking in Rust reimplementations of pieces of code in otherwise C++ codebases - in that case the Rust is an internal implementation detail, but it may have dependencies on other Rust crates. The main concern I have is that I don't really see a difference between (1) and (2) in practice, at least for Linux/ELF. The resulting .so files should be the same in terms of what symbols they export and what their dependencies are on other .so files - the only difference might be some Rust-specific metadata/bytecode, but that shouldn't matter at runtime. Attempting to statically link all the dependencies into is both undesireable (we'd want them to be shared among multiple cdylib crates) and incorrect (linking multiple instances of a crate into the address space is not well defined). Basically I think it's a bug to build any kind of shared/dynamic crates without It gets more complex if a single crate is being used for both (1) and (2) at the same time - for example, a crate that's both a dependency for an executable via a purely Rust path, and also a dependency for a C++ library. In that case, you want a single instance of the crate to be linked in, which requires that there only be a single build model for dynamic crates. |
I'm using dylibs for plugin architecture, just like rustc. The problems with cdylib is that its global aren't shared (due to static linkage) and it cause unwanted behavior when multiple instance of Rust things are loaded. |
@jsgf note that any successful rustc compilation provides a guarantee that nothing is ever linked twice. In that sense if The bug, however, can arise if you compile two separate dylibs (for example) and then load them both as plugins into a Python process, for example. Most existing use cases today I believe only have one dylib and hence are immune to this, but if you'd like to support multiple independently-developed Rust plugins then you'll definitely need shared dependencies! Also, can you confirm/deny whether a global boolean "build everything as dylibs" would work for you? I suspect it would, but I'm not sure if you'd require further granularity of how dependencies are compiled and such. @ishitatsuyuki it sounds like you're in bucket (2) then, right? The correct course of action is to build everything as dynamic dependencies, not just the plugin itself. |
I don't think it can tell, in general. If I have a top level executable
If there's just
Yes, if you're using dlopen() to dynamically link a shared dependency, then all this gets deferred to runtime; that's why a any kind of dylib must link it dependencies dynamically. But my point is that it can happen at link time as a result of composing Rust and C++ in ways that are all completely OK in isolation, but break in composition.
It comes down to:
In general the static/dynamic choice should be global for the whole dependency graph; if it's a per-edge property then you can end up with conflicts where one edge wants a dep as static, and another as dynamic, which would have to be reconciled (or simply fail the build). I don't think there's a back-compat issue here, because anything that can tell that it's deps are statically vs dynamically linked is doing something very specialized/unusual, or is simply wrong (like having static state that they're relying on being unshared). (Also interesting: since rustc compiles with |
@jsgf oh yeah of course yeah the compiler can't see through native libraries, I meant moreso for a crate graph the compiler knows to forbid duplication which may accidentally arise via linking.
I guess one thing I'd like to agree on is that this is empirically not true of how Rust is used today. There are of course situations that this can go wrong, but this statement isn't true 100% of the time. For example there are production users that rely on the fact that their Rust plugin does not dynamically link to the standard library, and we would break code if we were to change this behavior. I definitely agree in the abstract that any given graph can go wrong if you mix static/dynamic in bad ways. In general this isn't really a solvable problem per se though, rustc just tries to provide mitigations against shooting yourself in the foot in known bad situations.
This is another point I'd like to drive home agreement on. This PR is a breaking change, full stop. The breakage is outlined in this comment, and it's something I don't think we can deny. Now that's not to say we can't do anything! It just means we need an alternate solution. For example I'd propose a global configuration option that just builds everything as dynamic libraries, which I believes solves the use cases here? |
Could you go into a bit more detail about this? No doubt a failure of imagination on my part, but I can't think of how this could arise (ie, that their code or env knows/cares about how libstd is linked). |
Sure yeah. So the piece that's changing here is whether the standard library is linked dynamically or not. Today it's not (by default with
Does that make sense? |
Hm, yes I'd forgotten about deployment issues, especially since Rust doesn't have an ABI can't cope with any version skew (even point releases?). We only ever use shared/dynamic linking for dev cycles where the artifacts are used where they're built; deployable builds are always static (aside from "platform" libraries like libc and libstdc++). Anyway; I think the immediate topic of this issue is making cargo build dynamic dependencies when building a dynamic crate; that's the actual problem I'm having, since I can only (easily) build things statically, which means the dynamic/static choice for upstream dependencies isn't even available in any form. |
@jsgf yeah sounds right to me! Want to open an issue about customizing the crate type of transitive dependencies? |
@alexcrichton - Yeah, that's what #3573 is about. |
I'm going to close this for now due to activity, but please feel free to resubmit of course! |
While the name is equal to the root, we still should produce a dynamic one; otherwise the link will fail.
Close #3406
Code sample: