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

Cargo --crate-type CLI Argument #3180

Merged
merged 4 commits into from
Nov 14, 2021

Conversation

seanmonstar
Copy link
Contributor

@seanmonstar seanmonstar commented Oct 7, 2021

Add the ability to provide --crate-type <crate-type> as an argument to cargo build. This would have the same affect of adding crate-type in the Cargo.toml, while taking higher priority than any value specified there.

Rendered

@ehuss ehuss added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Oct 7, 2021
@stevelr
Copy link

stevelr commented Oct 9, 2021

Please document the commands line syntax to specify multiple values (a subset of the list values in Cargo.toml)

Is it
--crate-type cdylib,rlib
or
--crate-type cdylib --crate-type rlib
?

@stevelr
Copy link

stevelr commented Oct 9, 2021

Wondering out loud: this might be the hook needed for 'cargo test' to be able to run #tests in source files in a wasm32 project.

The developer might need to add --target in addition to --crate-type to build the tests for the local platform, but it would be nice to be able to use 'cargo test' in wasm projects.

@joshtriplett
Copy link
Member

I think this should work for cargo install as well: it makes sense to do cargo install --crate-type staticlib --root somedir to install a .a file and subsequently link to it.

@joshtriplett
Copy link
Member

We talked about this in today's @rust-lang/cargo meeting, and we can understand where this is coming from, but we'd like to understand why it wouldn't suffice to simply have a separate crate exporting the cdylib/staticlib interface. If that's possible, we would prefer that approach. If that wouldn't suffice for your use case, we'd like to understand that aspect of your use case in more detail.

@joshtriplett
Copy link
Member

One further thought from the meeting: it may make sense to restrict this only to cargo rustc, which already has the ability to apply to only one crate (e.g. if there's a lib and multiple bins in the same crate).

@guilhermewerner
Copy link

guilhermewerner commented Oct 19, 2021

We talked about this in today's @rust-lang/cargo meeting, and we can understand where this is coming from, but we'd like to understand why it wouldn't suffice to simply have a separate crate exporting the cdylib/staticlib interface. If that's possible, we would prefer that approach. If that wouldn't suffice for your use case, we'd like to understand that aspect of your use case in more detail.

I believe that having to create two packages just to change the crate type is not very ergonomic.

For example, I work on a mobile project where a lib in rust is compiled for android and ios, ie a cdylib for android and a staticlib for ios. What we do is declare the lib as crate-type = ["cdylib", "staticlib"], however this is not ideal, as it generates a .a that will not be used on android, and generates a warning for ios, as that cdylibs are not supported in the platform.

@guilhermewerner
Copy link

One further thought from the meeting: it may make sense to restrict this only to cargo rustc, which already has the ability to apply to only one crate (e.g. if there's a lib and multiple bins in the same crate).

Currently passing --crate-type to cargo rustc doesn't overwrite what's in Cargo.toml, it just adds one more crate-type to the rustc invocation.

It would be nice if the behavior was changed to overwrite what is in Cargo.toml

@seanmonstar
Copy link
Contributor Author

we'd like to understand why it wouldn't suffice to simply have a separate crate exporting the cdylib/staticlib interface.

So, in hyper's case (just 1 of the cases in the motivation), the exposed C API uses private implementation details. Shifting it to a separate crate would mean parts of the API could only depend on public details (for example, currently it can check the internal error kind, but publicly I suppose it would need to depend on the unstable Display format?). There are also a couple features exposed in the C API that we figured out at a low level, but haven't settled on how to expose them in idiomatic Rust.

But even if hyper could solve all of those problems on its own, the other motivating reason would still exist: cross-compiling.

@CryZe
Copy link

CryZe commented Oct 21, 2021

we'd like to understand why it wouldn't suffice to simply have a separate crate exporting the cdylib/staticlib interface.

There's a big long standing bug that causes all sorts of problems around reexports and explicit cdylib / staticlib top level crates. If you don't use LTO you have to export all of the publicly visible symbols from the top level crate directly, reexporting doesn't work.

rust-lang/rust#50007

@joshtriplett
Copy link
Member

@seanmonstar Would cargo rustc --crate-type alone solve your use case?

@seanmonstar
Copy link
Contributor Author

Currently, cargo rustc --crate-type cdylib fails with an error, argument '--crate-type' which wasn't expected.

If I use cargo rustc -- --crate-type cdylib, it compiles all the dependencies, and then errors with crate 'http' required to be available in rlib format, but was not found in this form, once for each dependency.

Do you mean would it be fine to make that work? Probably. I'm not stuck on it being cargo build. It's just that currently nothing at the command-line works.

@ehuss
Copy link
Contributor

ehuss commented Nov 2, 2021

I think there may have been some confusion in some of the comments above. We are suggesting to reword the RFC so that it only adds --crate-type to cargo rustc (cargo rustc --crate-type cdylib) since the cargo rustc command is designed for building a specific target, and is also intended as a lower-level interface. That would circumvent some of the concerns around cargo build as it can be somewhat general purpose, and builds multiple targets. I think we would be willing to approve the RFC now if it switched from cargo build to cargo rustc.

@seanmonstar
Copy link
Contributor Author

Oh ok, sure! I indeed was a little confused, but that would be perfectly fine. I think. I must admit, I don't know all the finer details that makes them different. It feels like cargo rustc mostly just allows passing more arguments to rustc, but otherwise feels very similar to cargo build.

@ehuss
Copy link
Contributor

ehuss commented Nov 4, 2021

Thanks! This looks like a good start to me.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 4, 2021

Team member @ehuss has proposed to merge 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 Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Nov 4, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 4, 2021

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

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Nov 4, 2021
@Eh2406
Copy link
Contributor

Eh2406 commented Nov 4, 2021

As artifact dependencies get stabilized, I would love for normal dependencies to know that they only need the rlib. And then for the unit graph to figure out what types are needed. So if a library has crate-type = ["lib", "staticlib", "cdylib"] that means that a cargo build in the library will make all 3, but does not imply that a project that depends on the library needs to build all 3. Such a change, if it worked out as well as I think it will, would reduce the motivation for this RFC. But this RFC would still be useful even then so +1.

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this RFC. label Nov 14, 2021
@rfcbot rfcbot added to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Nov 14, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 14, 2021

The final comment period, with a disposition to merge, 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.

This will be merged soon.

@ehuss
Copy link
Contributor

ehuss commented Nov 14, 2021

Huzzah! The @rust-lang/cargo team has decided to accept this RFC.

To track further discussion, subscribe to the tracking issue here:
rust-lang/cargo#10083

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. to-announce
Projects
No open projects
Status: Done (Stabilized)
Development

Successfully merging this pull request may close these issues.

8 participants