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 general mechanism of setting RUSTFLAGS in Cargo for the root crate only #3310

Closed

Conversation

ridwanabdillahi
Copy link
Contributor

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

@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2022

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.

@ehuss ehuss added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Sep 3, 2022
text/0000-rustflags.md Outdated Show resolved Hide resolved
@joshtriplett
Copy link
Member

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.

@thomcc
Copy link
Member

thomcc commented Sep 9, 2022

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.

@ridwanabdillahi
Copy link
Contributor Author

ridwanabdillahi commented Sep 9, 2022

@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.

@nrc
Copy link
Member

nrc commented Sep 14, 2022

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.

@thomcc
Copy link
Member

thomcc commented Sep 14, 2022

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.

@joshtriplett
Copy link
Member

@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".

@ridwanabdillahi
Copy link
Contributor Author

@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?

@jschwe
Copy link

jschwe commented Jan 13, 2023

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.

I agree. All the other options which have rustflags in the name modify the rustflags for the crate and all dependencies.
I have two different suggestions:

a) rename the flag to --local-rustflags or similar to make it clear that it doesn't affect everything
b) adjust cargo rustc to optionally take a positional parameter build | test | bench | .... which extends cargo rustc to also support the other cargo commands. If the new positional parameter is missing, then cargo rustc behaves as it currently does, which implicitly is the same as the new cargo rustc build. To pass parameters to e.g. the test binary, this would work the same way as proposed in the current version of this RFC, i.e. cargo rustc test <params> -- <rustflags> ; -- <flags to the test binary>. I'm not a huge fan of the semicolon though (in both approaches).

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".

To allow approach b) to support this I guess we could add another parameter to cargo rustc like --downstream-rustflags=<crate_name_regex>=<rustflags_for_selected_crates>

In general, I think none of this is impossible today - One could write a RUSTC_WRAPPER script that modifies rustflags on certain crates. It's just quite a lot of effort.

@joshtriplett
Copy link
Member

I don't have a strong preference on the bikeshed color for this. Anything like root-rustflags or top-rustflags that gives an indication of this only applying to one crate seems fine.

Comment on lines +127 to +134
## --rustflags for a workspace

Cargo supports two types of [workspaces](https://doc.rust-lang.org/cargo/reference/workspaces.html), a workspace with a root package and
a workspace with a virtual manifest, meaning a "primary" package does not exist.

For workspaces that contain a root package, any `--rustflags <RUSTFLAGS>` options set will be passed to the invocation of the Rust
compiler for the root crate only. If the `workspace.default-members` manifest key has been set, then only the members listed as a
default member will have the `--rustflags` values passed to the invocation of rustc.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like tying the definition of "root package" to the primary package or the default-members doesn't seem like the right way to go.

Unfortunately, I'm not sure of anything else. Maybe "whatever is 'selected' on the command-line" which will be inferred from the primary package or default-members but allows a user finer control (cargo build --package foo would make foo a root for that invocation)

Comment on lines +132 to +133
For workspaces that contain a root package, any `--rustflags <RUSTFLAGS>` options set will be passed to the invocation of the Rust
compiler for the root crate only. If the `workspace.default-members` manifest key has been set, then only the members listed as a
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some terminology is being mixed here. A "package" corresponds to a Cargo.toml file while a "crate" is one build target within a package, whether that is the bin, the lib, an example, or one of the 20 integration test bins.

Which does raise the question of how this might want to be applied for different build target types.

@epage
Copy link
Contributor

epage commented Jun 11, 2024

@rfcbot fcp postpone

Based on our discussion in today's Cargo team meeting, I'm proposed we postpone this RFC until someone can pick it up and drive it to completion.

In particular, areas that need addressing

  • Naming
  • Which build units this gets applied to
  • Documenting this within the general precedence of RUSTFLAGS
  • Documenting how this interacts with cargo rustc and the relationship between this flag and that command
  • More clearly specifying the CLI flag and the trade offs for that design
    • e.g. why value terminator over a shlex::joined string or other designs
    • Can you repeat the flag?

This runs into a bit of the same problem as --artifact-dir in trying to guess users intent. This is less of an issue with profile.rustflags and at one point in the conversation, there was interest in stabilizing profile-rustflags first to see how much of a use case is left. However, one blocker for profile-rustflags is we are uncomfortable putting rustflags in Cargo.toml, wanting to leave that to the CLI and .cargo/config.toml.

There is some debate over whether a CLI flag or a config is better for this. The CLI flag might be easier to design. In the past though, there has been concern over the growth of Cargo's CLI and how that makes it less approachable.

@rfcbot
Copy link
Collaborator

rfcbot commented Jun 11, 2024

Team member @epage has proposed to postpone 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-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels Jun 11, 2024
@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 Jun 18, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 18, 2024

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

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

rfcbot commented Jun 28, 2024

The final comment period, with a disposition to postpone, 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 is now postponed.

@rfcbot rfcbot added postponed RFCs that have been postponed and may be revisited at a later time. and removed disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels Jun 28, 2024
@rfcbot rfcbot closed this Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
finished-final-comment-period The final comment period is finished for this RFC. postponed RFCs that have been postponed and may be revisited at a later time. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. to-announce
Projects
Archived in project
Status: Unreviewed
Development

Successfully merging this pull request may close these issues.

10 participants