rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

Add a general mechanism of setting RUSTFLAGS in Cargo for the root crate only

Open ridwanabdillahi opened this issue 1 year ago • 6 comments

This RFC adds support In Cargo for setting RUSTFLAGS via a new command line options, --rustflags <RUSTFLAGS> that only applies to the current crate being built.

Rendered

ridwanabdillahi avatar Sep 02 '22 20:09 ridwanabdillahi

Error: This repository is not enabled to use triagebot. Add a triagebot.toml in the root of the master branch to enable it.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

rustbot avatar Sep 02 '22 20:09 rustbot

I do think we should have a mechanism for this, but I think this option name would be confusing; it doesn't give any indication that it applies just to the root crate, or that it's different than RUSTFLAGS in any way.

joshtriplett avatar Sep 09 '22 16:09 joshtriplett

We have rustflags that impact ABI, and that would be unsound to only compile one crate with. For example, -Csoft-float on some targets, -Ctarget-feature (and -Ctarget-cpu by extension), and likely others (several -Z flags are this way, for sure).

I think this RFC should have some consideration of this, which I don't see.

thomcc avatar Sep 09 '22 18:09 thomcc

@joshtriplett

Thanks for taking a look, I understand the concern and I'm fine to update the name of the option to make it clearer. Any thoughts on a suitable replacement? root-crate-rustflags, current-crate-rustflags?

@thomcc

Thanks for the comment, this issue exists today in Cargo via the cargo rustc subcommand which passes rustflags to rustc only for the root crate being compiled. This potential issue is not mentioned in the RFC and definitely should be. I'll update the Drawbacks section to explicitly mention this scenario and the harm that can be done by certain rustflags. Thank you for pointing this out.

ridwanabdillahi avatar Sep 09 '22 18:09 ridwanabdillahi

We have rustflags that impact ABI, and that would be unsound to only compile one crate with. For example, -Csoft-float on some targets, -Ctarget-feature (and -Ctarget-cpu by extension), and likely others (several -Z flags are this way, for sure).

It's worth mentioning, but I wouldn't be too concerned about it - both setting rust_flags and setting ABI flags are advanced features and naive users are unlikely to stumble into an error here, so I'm fine with this being an 'at your own risk' kind of feature. The other side of the trade-off is that any mechanism for checking this sort of thing would need a list of 'risky' flags which would need to be kept in sync between rustc and Cargo, which is intrinsically fragile. More philosophically, the less Cargo knows about rustc, the better - separation of concerns and all that.

nrc avatar Sep 14 '22 16:09 nrc

In principal I agree, but it is worth noting -Ctarget-cpu=native is among the most common uses of RUSTFLAGS in my experience, even among new users -- it's even in many guides to place in RUSTFLAGS (usually when installing their crate). Most are not aware it has impact on ABI.

This isn't to say that cargo should know about it, but just that it's not as much of an advanced-user feature as you suggest.

thomcc avatar Sep 14 '22 16:09 thomcc

@ridwanabdillahi At the risk of making this slightly more complex: we have various things in Cargo for matching crates and setting profile information on them, and I'm wondering if we could use similar logic, so that it's possible to say "use these rustflags for this crate, and these rustflags for the top-level crate".

joshtriplett avatar Sep 27 '22 17:09 joshtriplett

@joshtriplett thanks for your response!

I'm wondering if we could use similar logic, so that it's possible to say "use these rustflags for this crate, and these rustflags for the top-level crate"

I believe this is possible via the profile-rustflags unstable Cargo toml feature that exists today. This allows a user to specify which rustflags are enabled for a specific profile and can also specify the crates that should have these rustflags set.

The point of this RFC was to remove the need to specify a given profile when setting the rustflags that should be enabled for a given crate. This way any Rust developer that would like to set rustflags can regardless of the profile being used when running a build/test.

I understand this RFC adds to the complexity of setting rustflags by adding yet another supported way that is drastically different from the existing scenarios, i.e. cargo cli option. There is also an alternative listed in the RFC of expanding on the already supported build.rustflags config section to allow for specifying crates to match against in the same manner as the profile.<profile>.package.<crate> manifest key. Thoughts on this?

ridwanabdillahi avatar Sep 30 '22 17:09 ridwanabdillahi