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

Add a --crate-type override to build and check. #8789

Closed
wants to merge 2 commits into from

Conversation

cutsoy
Copy link

@cutsoy cutsoy commented Oct 16, 2020

When writing a single library in Rust to target multiple platforms (e.g. iOS, Android, WASM), it's common to want to set a different crate type for each target. For example, on iOS we might want a staticlib, on Android and WASM a cdylib, and for Rust itself we might want to keep the default rlib. For use cases, see rust-lang/rust#77716, #4881, #6160 and more generally cargo-crate-type.

Unfortunately, that's currently not easy to do, due to:

  1. Crate-type cannot be set depending on target. Ability to set crate-type depending on target #4881
  2. Crate-type cannot be overridden with --crate-type. --crate-type does not override lib properties on manifest #6160
  3. Using multiple Cargo.*.toml files is currently rejected. --manifest-path path/to/NonCargo.toml fails #6690
  4. #![crate_type = "*"] does not work with Cargo.

I implemented a fix that allows --crate-type to be passed directly to Cargo build and check.

It's quite simple but still effective: if you run cargo build, it will build according to your Cargo.toml. If you run cargo build --crate-type cdylib, it will build a shared library (e.g. for Android). If you run cargo build --crate-type staticlib, it will build a static library (e.g. for iOS). In both cases, the provided crate type will override the crate type of all library targets in de "root" manifest (i.e. it doesn't modify the manifests of dependencies), and the modification is not persisted to disk (it is applied "ad-hoc").

This would allow the on-disk Cargo.toml to keep the default rlib crate-type, and at the same time accommodate "end users" who want to build the library into a static or shared library without modifying the source code.

@rust-highfive
Copy link

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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 16, 2020
@bjorn3
Copy link
Member

bjorn3 commented Oct 17, 2020

This could be useful for cg_clif's JIT mode, which requires all crates to be available through one or more dylibs. Making all crates dylibs would trivially satisfy this requirement.

@@ -152,6 +152,12 @@ pub trait AppExt: Sized {
self._arg(opt("manifest-path", "Path to Cargo.toml").value_name("PATH"))
}

fn arg_crate_type(self) -> Self {
self._arg(
opt("crate-type", "Override crate type for all libraries").value_name("CRATE-TYPE"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
opt("crate-type", "Override crate type for all libraries").value_name("CRATE-TYPE"),
opt("libs-crate-type", "Override crate type for all libraries").value_name("CRATE-TYPE"),

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? Since crate-type always applies to libraries and examples. I think there are two options:

  1. Call it --crate-type and apply the modification to both the library and all example targets. (This PR currently only applies it to libraries, not examples).
  2. Call it --libs-crate-type and apply the modification only to the library target.

@cutsoy
Copy link
Author

cutsoy commented Oct 17, 2020

This could be useful for cg_clif's JIT mode, which requires all crates to be available through one or more dylibs. Making all crates dylibs would trivially satisfy this requirement.

Just to make sure we're on the same page: this PR does not modify the crate-types of dependencies directly, but I think the compiler will attempt to propagate the crate-type to upstream dependencies if necessary. Correct? Would this still be useful to you?

@bjorn3
Copy link
Member

bjorn3 commented Oct 17, 2020

I thought it would also change the crate type for dependencies. Because it doesn't, this means this PR is not useful for me. I guess I will have to implement real support for the JIT mode in Cargo.

@cutsoy
Copy link
Author

cutsoy commented Oct 17, 2020

Hm, bummer! I don't think it would be quite as trivial to override the crate types of (transitive) dependencies, because the fix in this PR is applied immediately after the manifest is read from disk and before dependencies are resolved.

I haven't used the JIT mode yet (but I will!) and I have no knowledge about this whatsoever but wouldn't it be possible to strip the metadata from an rlib to obtain a staticlib, and then convert that into a dylib (as long as the original code was built as position independent, which fortunately is the default)?

@bjorn3
Copy link
Member

bjorn3 commented Oct 17, 2020

How and when do I build a dylib when --crate-type is not dylib? A for how taking the individual rlibs and passing them to a linker doesn't work, as that misses all necessary linker arguments that are only calculated when linking a dylib the regular way. As for when if the linking were to happen when running cg_clif in JIT mode, that would require it to be linked again every time you run in JIT mode. This would be a significant slowdown.

@cutsoy
Copy link
Author

cutsoy commented Oct 19, 2020

I understand ... Given that your use case doesn't benefit from this, what does it mean for this PR?

I think there are still some other use cases that are worthwhile to support:

For use cases, see rust-lang/rust#77716, #4881, #6160 and more generally cargo-crate-type.

@francesca64
Copy link

I tested this out, and it works great! Thanks so much for implementing this.

@alexcrichton
Copy link
Member

Thanks for the PR here! This is definitely a longstanding pain point in Cargo and one I've always liked to see fixed!

I think though it may be best to discuss possible solutions first before implementing and/or accepting one. For example this solves the final crate tweaking its crate type but I think there's also a case to be made for intermediate crates to get overridden as well. Effectively I think that we may want a broader solution than simply changing the top-level crate (although I realize that's sufficient for your purposes).

A more general system would also help configure multiple crates in a workspace, for example, or allow you to build multiple crates at once and configure separately each crate type.

@cutsoy
Copy link
Author

cutsoy commented Oct 20, 2020

Hmm. That wouldn't actually work for my use case. Currently, all (transitive) dependencies are regular rlibs. The final crate is a staticlib or cdylib depending on whether it's built for Android or iOS. If the --crate-type override would apply to all (transitive) dependencies, the dependencies would become cdylibs too, which doesn't work:

warning: The package b provides no linkable target. The compiler might raise an error while compiling a. Consider adding 'dylib' or 'rlib' to key crate-type in b's Cargo.toml. This warning might turn into a hard error in the future.

On the other hand, I don't want --crate-type dylib either because then it compiles a separate .so for each crate and I would have to ship hundreds of .so files in the APK. One of the simplest projects already has 173 transitive dependencies ...

Alternatively, (I don't know much about the rlib format, but) is it somehow possible to convert an .rlib to a .a or .dylib file after it's been built?

@cutsoy
Copy link
Author

cutsoy commented Oct 21, 2020

I tried the alternative (compiling to rlib and converting to a staticlib / cdylib afterwards) without any luck ... I tried to use cargo_metadata to parse the JSON compiler messages and figure out which libraries to link to, but that's not currently possible because the CompilerArtifact message doesn't contain the target architecture that it is compiled for and includes some of the artifacts that are built for the host architecture. 😞

Needless to say, I would really like to see this feature added to Cargo, if possible. Its implementation is simple and its use-case is clear: compiling a single crate to a user-specified type of library without changing its dependencies.

There are also many parallels with existing options for cargo build that also do not "bleed" through to the dependencies or other workspace members. For example, bin, example, test, bench, features all don't apply to dependencies. That would defeat the purpose.

Also, I think the use case where one would want to modify the crate type of all dependencies ad-hoc is relatively limited (basically only tooling like debuggers or JITs) in comparison to the use case where one would only want to change the crate type of the final crate (cross-platform integration, e.g. using 1 library for WASM, iOS, Android and Rust).

I understand that this is something that needs to be discussed properly before being added since it implicitly adds a long-term commitment to maintain it. But on the other hand, a recent change in rustc makes this nearly impossible to work around. Would it make sense to start with an undocumented prefixed option instead? For example cargo build --experimental-crate-type staticlib. The prefix would indicate that there is no commitment to backwards compatibility in future versions of Cargo.

@alexcrichton
Copy link
Member

Sorry to clarify the feature I'm talking about isn't configuring all crate types at once, just having the ability to configure crate types for all crates. I also agree that this is a small feature, but as maintainers of Cargo it's our job to figure out how it fits into the big picture (or rather make sure it does). If Cargo was just a bunch of tiny additions over time it would not be a pleasant tool to use.

I think whatever we do in this arena will be unstable at first. There are a number of various mechanisms Cargo has to control the usage of unstable features, but the main one is -Zunstable-options for this probably.

@bors
Copy link
Contributor

bors commented Jan 5, 2021

☔ The latest upstream changes (presumably #8997) made this pull request unmergeable. Please resolve the merge conflicts.

@Absolucy
Copy link

Absolucy commented Jan 9, 2021

@cutsoy i could try to re-do this PR if you don't have time?

@cutsoy
Copy link
Author

cutsoy commented Jan 10, 2021

@cutsoy i could try to re-do this PR if you don't have time?

Still very interested in this: it's a major showstopper for me. So I definitely want to invest some time. I will fix the conflict later today.

But more generally, what's the path forward? As is, I don't think the PR was accepted (regardless of the merge conflict). What steps do we need to take to get this merged?

@alexcrichton
Copy link
Member

I mentioned this above, but put plainly this is a feature proposal which has not been discussed prior to being proposed. We have documentation on adding features to Cargo and features like this are often hard to distinguish between small/large due to how the affect multiple parts of Cargo.

Unfortunately there's no simple answer to "what's next to merge this?". We as Cargo maintainers don't always have tons of time to help design and implement new features. We can try to offer ideas and small bits of review where possible, but that's unfortunately not always possible or easy to incorporate. For example I mentioned above a more general form of this feature which I think we'll want in the future at least, and in the meantime I personally fear that this feature is too focused that I'm not sure if it would be good to land in Cargo.

I realize this isn't a great answer because there's not clear "do this and we'll merge" step here, but the reality is that the Cargo team is trying to maintain a stable build tool for the Rust ecosystem and we don't have all the time in the world to focus on new features and everyone's use cases.

@bors
Copy link
Contributor

bors commented Feb 23, 2021

☔ The latest upstream changes (presumably #9184) made this pull request unmergeable. Please resolve the merge conflicts.

@Absolucy
Copy link

Absolucy commented Mar 8, 2021

I guess some people are going to need to use patched rustc for cdylib support :/

@ehuss
Copy link
Contributor

ehuss commented Aug 6, 2021

I'm gonna close this due to inactivity. As discussed above, new features usually have some level of discussion before proceeding to a PR. Thanks!

@ehuss ehuss closed this Aug 6, 2021
@seanmonstar
Copy link

I've opened an RFC to propose this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants