-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Revert "Allow dynamic linking for iOS/tvOS targets." #77716
Conversation
This reverts commit 56e115a.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (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. |
I'd rather this be a rustc flag rather than being completely disabled. |
Hmm, I think |
I'm probably not the best person to review this, but also don't know who would be. @francesca64 Can you check if your build setup passes |
@jonas-schievink mm, actually, I know what's happening now. In my Cargo.toml, I have: [lib]
crate-type = ["staticlib", "cdylib", "rlib"]
|
Ah, I see, that explains why this is causing problem now.
|
@francesca64 couldn't something like this be used? #[cfg(target_os = "android")]
#![crate_type = "cdylib"]
#[cfg(target_os = "ios")]
#![crate_type = "staticlib"]
#[cfg(not(any(target_os = "android", target_os = "ios")))]
#![crate_type = "rlib"] |
You'd have to use |
|
I still don't feel "my project doesn't work as a cdylib" is a valid reason to re-add a meaningless limitation |
I don't think "adding a meaningless limitation" is the end-goal here. It's simply the fastest fix for a number of issues that are more difficult to address:
|
Temporary fixes are sadly rarely temporary. And this "temporary fix" would impede actual functionality. |
If we don't find a workaround for those Cargo bugs that you can use, I think we should go ahead with the revert, since this is a stable regression affecting production users. |
If there is any interest, I can file a PR that fixes rust-lang/cargo#6690. This commit cutsoy/cargo@ebfc65c would allow multiple Alternatively, rust-lang/cargo#6160, is also relatively easy to fix with an implementation that looks at the Although admittedly, neither fix would be as elegant as fixing rust-lang/cargo#4881 (which would allow Any thoughts? (Alternatively, there's also cargo-crate-type which automates replacing the crate-type between builds, which might provide a work-around for others.) |
@cutsoy An actual fix would be much better than this PR, yes, and would be appreciated very much. |
@aspenluxxxy
I know this situation must be frustrating, but I believe your PR was merged on the basis that it wasn't known to have any downsides: #73516 (comment) Thanks for doing such a heroic amount of legwork! And I'm always happy to see someone else using Rust for mobile development. I'm sure people would appreciate a fix for rust-lang/cargo#6690, though I wonder if it'd be contentious to be less opinionated about the file name than that. I don't have much of a hat in this particular issue either way, since for my use case I could just generate each Cargo.toml in a different temp dir (my build process is abstracted away from end-users, so I can get away with doing arbitrary work behind the scenes). A fix for rust-lang/cargo#6160 would make me ecstatic! As I mentioned above, I'm hiding all the build complexity anyway, so just being able to override the crate-type via a flag would be super elegant for me. rust-lang/cargo#4881 definitely seems like the hardest one... it'd actually be less elegant for my use case anyway, but I imagine people who are building things more manually would prefer that. |
I would like to note that the merging of this pull request into upstream has been more than an extreme inconvenience for me, as I maintain a distribution of *nix tools for iOS. Imagine my surprise updating rust and seeing I can no longer build dynamic libraries. The merging of this improper solution has all but destroyed my workflow when compiling dynamic libraries using the latest Rust. |
Sorry for the inconvenience, but this should be expected when relying on tier 2 targets that don't see much testing. |
No, it shouldn't. It's lazy. T2 targets are guaranteed to build. They're not expected to thrash around because some blessed runtime workflow broke. That seems quite opposite of the build guarantee. |
"guaranteed to build" means that the target is guaranteed to successfully build the contained rustup components, not that it'll be able to build your application |
we should revert this pr since t2 targets were guaranteed to be successfully built, this was definitely not a proper solution for what francesca64 mentioned. |
#3 is hardly "difficult to address". It's literally just removing three lines of code that cause yet another pointless limitation. I don't know how you came to the conclusion that reverting was the best course of action here when people even provided patches that would fix OP's issue without breaking it for others who do need dylibs. |
This reverted pull request got mentioned in #21727 I've got a growing number of Rust libraries that I build for Windows, macOS, Linux, Android and iOS as shared libraries packaged in nuget packages with C# bindings on top. I am blocked on this for iOS support and just build static libraries for now to ensure the code builds, but I discard the output. I have a line in my build scripts to edit the Cargo.toml and change the crate type to staticlib just for iOS, as I think cargo still doesn't have a way to specify platform-specific crate types. Here's my question: I see there's been a lot of back and forth on this, and my understanding is that the fix is fairly minimal, as it consists of removing the block for dynamic linking on iOS. How do we get this issue moved forward from here? I don't expect all crates to build as cdylib, but then it's not any worse than the current state where cdylib is hard blocked anyway. Once the block is removed, we can start just fixing build issues on crates that need them. Did I miss anything? |
If you specify multiple crate types cargo will drop all unsupported types with a warning. Only linking and/or assembling a staticlib happens multiple times. The rest of the compilation process doesn't get duplicated when using multiple crate types. |
Is that the reason why some people report that simply allowing dynamic linking would break stuff for them? I assume it's because it would actually try building cdylib crate types when it would just skip over it with a warning before? What if your only crate type is cdylib, doesn't it fail the whole build? I'm not sure it's such a good idea to "warn" when what you've requested to build, can't be built at all. |
while in most cases it shouldn't cause breakages it is conceivable that there are some cases where breakage occurs. one example is if a static lib and a dynamic lib can't both be built due to name conflicts. since you can't choose the crate type per platform, either the crate is split into two crates or change the build to only use dylibs. In general the rust team is pedantic about not breaking things. I guess it's a good stance overall. This means that we get stuck with some edge cases like this one. I think another example of this would be Default can't be implemented with const generics because there is some weirdness around empty arrays which I forgot. |
@dvc94ch I'm trying to figure out what the best approach to finding an acceptable solution is, because right now, the least painful approach would likely be to patch Rust and roll my own custom distribution, just to change a bool to true. It's an insane amount of work just for something that at a bare minimum could be overridden with an environment variable, or command-line argument to cargo? This issue has been opened since 2015 - I suggest that if it's not possible to find a default solution that would satisfy everyone, then we should look into make it possible to override the default behavior, such that we could at least unblock those of us who really need to build shared libraries on iOS, without causing breaking changes for everybody else. |
It has been almost two years so my memory is a bit foggy on this, but I think this comment by someone else on another issue explains why the revert was necessary: rust-lang/cargo#4881 (comment).
Before the original PR was merged, Cargo/Rustc would ignore the After that PR was merged (and before it was reverted), it would always try to build the I think this was the issue, but please correct me if I'm wrong. 😁 Edit: note that this only applies if you're using the same library for multiple targets. Otherwise, you could of course just remove the |
well, I'm sure that we can find a solution thats ok for everyone. should be a non-issue for a new edition (but still requires someone to actually bring it up when the next edition occurs), and if someone on the rust team had some interest in ios I'm sure they could get it done before that. if your company is interested in this @awakecoding you could ask on the zulip chat if someone on the core team willing to push this through.
I believe the naming conflict example is slightly easier to understand. |
I agree that it's easier to understand but I don't think that was the issue. On other platforms (e.g. macOS) it has always been possible to compile both Having said that, I see that this got merged recently: rust-lang/cargo#10083. The So summing up, I think this PR can be reverted in 1.64. 🤞 |
and I'm pretty sure that if they were linking native libraries, they added the correct dylibs to the search path. actually agree that is even easier to understand. I'm pretty sure I've seen warnings and errors about naming conflicts before from cargo, so it's not unheard of. |
Revert "Revert "Allow dynamic linking for iOS/tvOS targets."" This reverts commit 16e10bf (PR rust-lang#77716). The original original PR enabled `cdylib` builds for iOS. However this caused problems because: > This new feature in Rust 1.46 added a lot of headache for iOS builds with cdylib targets. cdylib target is near impossible to build if you are using any crate with native dependencies (ex. openssl, libsodium, zmq). You can't just find .so files for all architectures to perform correct linking. Usual workflow is the following: > > 1. You build staticlib and rely that native dependencies will be linked as frameworks later > 2. You setup right cocoapods in ObjectiveC/Swift wrapper. > > As cargo doesn't support platform-dependent crate types rust-lang#4881 as a result a lot of projects now broken on Rust 1.46 However, this will be soon a thing of the past since 1.64 brings us the long awaited much anticipated `--crate-type` flag. > I see that this got merged recently: rust-lang/cargo#10083. The --crate-type flag will get stabilized in 1.64. In 1.64, you could still get a successful iOS staticlib with cargo build --crate-type=statclib even if the crate has cdylib targets too. If I'm not mistaken, this solves the problem too so this PR could be reverted in 1.64 with relatively little headache. So summing up, I think this PR can be reverted in 1.64. 🤞
Revert "Revert "Allow dynamic linking for iOS/tvOS targets."" This reverts commit 16e10bf (PR rust-lang#77716). The original original PR enabled `cdylib` builds for iOS. However this caused problems because: > This new feature in Rust 1.46 added a lot of headache for iOS builds with cdylib targets. cdylib target is near impossible to build if you are using any crate with native dependencies (ex. openssl, libsodium, zmq). You can't just find .so files for all architectures to perform correct linking. Usual workflow is the following: > > 1. You build staticlib and rely that native dependencies will be linked as frameworks later > 2. You setup right cocoapods in ObjectiveC/Swift wrapper. > > As cargo doesn't support platform-dependent crate types rust-lang#4881 as a result a lot of projects now broken on Rust 1.46 However, this will be soon a thing of the past since 1.64 brings us the long awaited much anticipated `--crate-type` flag. > I see that this got merged recently: rust-lang/cargo#10083. The --crate-type flag will get stabilized in 1.64. In 1.64, you could still get a successful iOS staticlib with cargo build --crate-type=statclib even if the crate has cdylib targets too. If I'm not mistaken, this solves the problem too so this PR could be reverted in 1.64 with relatively little headache. So summing up, I think this PR can be reverted in 1.64. 🤞
Revert "Revert "Allow dynamic linking for iOS/tvOS targets."" This reverts commit 16e10bf (PR rust-lang#77716). The original original PR enabled `cdylib` builds for iOS. However this caused problems because: > This new feature in Rust 1.46 added a lot of headache for iOS builds with cdylib targets. cdylib target is near impossible to build if you are using any crate with native dependencies (ex. openssl, libsodium, zmq). You can't just find .so files for all architectures to perform correct linking. Usual workflow is the following: > > 1. You build staticlib and rely that native dependencies will be linked as frameworks later > 2. You setup right cocoapods in ObjectiveC/Swift wrapper. > > As cargo doesn't support platform-dependent crate types rust-lang#4881 as a result a lot of projects now broken on Rust 1.46 However, this will be soon a thing of the past since 1.64 brings us the long awaited much anticipated `--crate-type` flag. > I see that this got merged recently: rust-lang/cargo#10083. The --crate-type flag will get stabilized in 1.64. In 1.64, you could still get a successful iOS staticlib with cargo build --crate-type=statclib even if the crate has cdylib targets too. If I'm not mistaken, this solves the problem too so this PR could be reverted in 1.64 with relatively little headache. So summing up, I think this PR can be reverted in 1.64. 🤞
Revert "Revert "Allow dynamic linking for iOS/tvOS targets."" This reverts commit 16e10bf (PR rust-lang#77716). The original original PR enabled `cdylib` builds for iOS. However this caused problems because: > This new feature in Rust 1.46 added a lot of headache for iOS builds with cdylib targets. cdylib target is near impossible to build if you are using any crate with native dependencies (ex. openssl, libsodium, zmq). You can't just find .so files for all architectures to perform correct linking. Usual workflow is the following: > > 1. You build staticlib and rely that native dependencies will be linked as frameworks later > 2. You setup right cocoapods in ObjectiveC/Swift wrapper. > > As cargo doesn't support platform-dependent crate types rust-lang#4881 as a result a lot of projects now broken on Rust 1.46 However, this will be soon a thing of the past since 1.64 brings us the long awaited much anticipated `--crate-type` flag. > I see that this got merged recently: rust-lang/cargo#10083. The --crate-type flag will get stabilized in 1.64. In 1.64, you could still get a successful iOS staticlib with cargo build --crate-type=statclib even if the crate has cdylib targets too. If I'm not mistaken, this solves the problem too so this PR could be reverted in 1.64 with relatively little headache. So summing up, I think this PR can be reverted in 1.64. 🤞
The problem is in cargo-mobile not passing the correct linking information (and generally cargo knowing nothing about how to make platform-correct dylibs), you may copy the logic that I put in cargo-c to avoid this issue. |
For anybody still hard-blocked on this, I have rolled my own prebuilt Rust distribution with a patch to enable iOS shared libraries. The ideal solution is an upstream fix, but this will do until the matter is finally resolved. I detailed my instructions here: #21727 (comment) |
This reverts PR #73516.
On macOS I compile static libs for iOS, automated using cargo-mobile, which has worked smoothly for the past 2 years. However, upon updating to Rust 1.46.0, I was no longer able to use Rust on iOS. I've bisected this to the PR referenced above.
For most projects tested, apps now immediately crash with a message like this:
This can be reproduced by using cargo-mobile to generate a winit example project, and then attempting to run it on an iOS device (
cargo mobile init && cargo apple open
).In our projects that depend on DisplayLink, the build instead fails with a linker error:
After reverting the change to enable dynamic linking on iOS, everything works the same as it did on Rust 1.45.2 for me.
In the future, would it be possible for me to be pinged when iOS-related PRs are made? I work for a company that intends on using Rust on iOS in production, so I'd gladly provide testing.
cc @aspenluxxxy