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

Allow iOS dynamic linking #88130

Closed
wants to merge 1 commit into from
Closed

Allow iOS dynamic linking #88130

wants to merge 1 commit into from

Conversation

Torrekie
Copy link

@Torrekie Torrekie commented Aug 18, 2021

This reverts PR #77716.
Since Apple LLVM actually supported compiling .dylibs on/to iOS targets, there is no reason to prevent dynamic linking on a platform that supports it.
aarch64-apple-ios as a Tier 2 target that was 'promised to be compiled', should be allowed to access such a basic feature. Once dynamic library are prohibited to be built, some common libraries such as Cryptography would no longer able to run on iOS targets, it is theoretically feasible though.
I was trying to get mitmproxy working on my iPhone 12, as dynamic libraries written in rust cannot be compiled to this target, causing a lot of problems when attempting to run open source projects on an iOS device which was quite capable.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 18, 2021
@wesleywiser
Copy link
Member

Based on this comment, it seems like there are still several unresolved issues and merging this would re-introduce the issues that caused #77716 to be opened in the first place.

Is my understanding correct or have those issues been mitigated in another way and there is no longer a concern of breaking users?

Thanks!

@wesleywiser wesleywiser added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2021
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 6, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@Torrekie Can you please address the question from the reviewer?

@Torrekie
Copy link
Author

Torrekie commented Sep 7, 2021

Based on this comment, it seems like there are still several unresolved issues and merging this would re-introduce the issues that caused #77716 to be opened in the first place.

Is my understanding correct or have those issues been mitigated in another way and there is no longer a concern of breaking users?

Thanks!

'breaking users' was not caused by 'dynamic linking' but 'crate-types' are not able to override by target specific configurations. rust-lang/cargo/#4881

Users could correctly build static or dynamic libraries for iOS by setting crate type in Cargo.toml before #77716, for iOS Apps that needs to be uploaded to App Store, they can just set crate type as static. Allowing dynamic linking is not stopping people from developing iOS Apps for production use, but loosing an option to build some dylibs to iOS for tests or other use. As this comment said, he's workflow would not able to cross compile rust libraries after compiler blocked iOS dylibs (and his distro still having large ammount users in iOS jailbreak community). Aren't users who were attempting to build iOS dynamic libraries 'breaking users'?

sorry for my terrible English tho

@wesleywiser
Copy link
Member

Users could correctly build static or dynamic libraries for iOS by setting crate type in Cargo.toml before #77716, for iOS Apps that needs to be uploaded to App Store, they can just set crate type as static.

The problem as I understand it is that some projects have multiple crate types set and (prior to #73516) cdylib did not take effect for iOS targets. That's definitely not ideal but without something like rust-lang/cargo#4881 (as you mentioned), there's not really a good work around for existing users in this situation.

We discussed #77716 in the weekly compiler team triage meeting and concluded that merging it was the most appropriate action to take at that time. Since then, no new information has been discovered and the overall situation has not changed (the required features discussed at that time like rust-lang/cargo#4881 have not yet been implemented). Therefore to reverse that decision, we will need to have another conversation in the form of a Major Change Proposal that explains why that decision was incorrect, what the correct behavior should be, what work arounds are available to users in the situation described above and what the long term behavior should be taking into account proposed Cargo features.

Alternatively, I have a suggestion I did not see proposed in #77716: use a custom target JSON file which would allow you to set dynamic_linking: true for your custom target. You will also need to use the cargo build-std feature but I think that will do what you want. Does that seem like a potential path forward for you?

@Torrekie
Copy link
Author

Torrekie commented Sep 8, 2021

Currently I have a custom compiler with some modifications on my device that could let me create dylibs for Darwin targets. But bootstrapping full rust compiler to jailbroken iOS environment needs to compile rust for twice, which was spending much time: once for enabling rustc to build dylibs when iOS targets has been set, and use the compiler from first result to cross compile rust and rust toolchains to iOS (rustlibs cannot be built to iOS if rustc doesn't allow it).

But I still wonder if there is any future plans that regarding to those issues mentioned above. Allowing separate crate types for different targets would be pretty.

@bjorn3
Copy link
Member

bjorn3 commented Sep 8, 2021

You can specify a custom json target spec with dynanic linking enabled when compiling rustc. Just pass the path to the json file instead of the target name. This saves the first bootstrap.

@JohnCSimon
Copy link
Member

Ping from triage:
@Torrekie what is the status of this PR? Is it ready for review?

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@Torrekie This PR hasn't seen any movement in the last two Months - I'm closing this as inactive. Please feel free to reopen when you're ready to continue. Thanks.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Oct 31, 2021
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Oct 31, 2021
@dvc94ch
Copy link
Contributor

dvc94ch commented Apr 3, 2022

@francesca64 cargo rustc now supports the --crate-type flag. Does that fix your build so dynamic library building can be reenabled for ios?

@francesca64
Copy link
Contributor

@dvc94ch I've changed jobs, so I'm no longer involved; @ArthurKValladares is the new point of contact.

@dvc94ch
Copy link
Contributor

dvc94ch commented Apr 4, 2022

Well, that's kind of great. Now this feature will never land because one day someone's build didn't work which no longer knows if the build is broken. I guess that is why you don't make concessions for these things. Does this mean that this needs a new rust edition to be enabled? @ArthurKValladares any comments?

@ArthurKValladares
Copy link

ArthurKValladares commented Apr 4, 2022

I would hate to see improvements to the language never landing because of work that has fallen on my shoulders. I'll dedicate some time in the next few days to bringing myself up to date on this issue and figuring if the --crate-type fix solves the problem we were running into.

@dvc94ch
Copy link
Contributor

dvc94ch commented Apr 6, 2022

@ArthurKValladares I believe you're running into rust-lang/cargo#10356 can you try on nightly?

@ArthurKValladares
Copy link

ArthurKValladares commented Apr 6, 2022

@dvc94ch Yeah, this is the problem. I was under the impression that I was on nightly, but I was mistaken and everything worked after a rustup update. Sorry for deleting the old comment, I realized it was incorrect and was about to make a new comment with the updated information, but you beat me to the punch.

@dvc94ch
Copy link
Contributor

dvc94ch commented Apr 6, 2022

@ArthurKValladares thanks for looking into this

@wesleywiser is that enough to reopen and merge this PR?

@dvc94ch
Copy link
Contributor

dvc94ch commented Apr 9, 2022

Given the new evidence, I reopened the PR in #95847

@holzschu holzschu mentioned this pull request Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants