-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 member
table to cargo config
#8797
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (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. |
6bd30fd
to
18200dd
Compare
The member table right now only contains the `target` member. It defines the build target for a single workspace member overriding both the `build.target` config value and the '--target' switch. Fixes rust-lang#7004
18200dd
to
508aad9
Compare
Thanks for the PR! Almost all major changes to Cargo typically go through the RFC process, though sometimes we allow nightly-only changes if the design is well understood. I think in this case there is a bit of design space to explore. Generally we haven't allowed workspace-specific settings in config files. Config files are intended more for local-system settings (although admittedly that doesn't always follow in practice). I think for specifying things like this, it would probably want to lean more towards placing settings in These may or may not have some interaction, but I'm not clear on how they might. |
☔ The latest upstream changes (presumably #8799) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
I really like this idea and I think that it would fix most issues I have with |
A default-target field would be nicer than this. One of the reasons I did this in config in the first place was that I was certain changes to Cargo.toml would need to go through the RFC process while I thought the config was treated as an implementation detail. I guess this was not correct. |
@Richard-W I just created a related proposal to replicate all package-specific |
Closing because there are better approaches than this one. |
This PR attempts to fix #7004. While I intend to get this merged eventually I am aware that this implementation is somewhat ugly right now since I avoided major refactorings. Any feedback is appreciated. I have discussed the implementation options here.
This introduces
[member.'name']
tables into.cargo/config
. The tables right now have the single membertarget
which defines the target of a member crate if it is set. For the member crate this key takes precedence over thebuild.target
value and the--target
CLI argument. This is useful for crates where differing targets do not really make sense such as for WASM or UEFI binaries.I am not sure if this needs an RFC. IIRC past RFCs mainly concern themselves with the manifest format so I assume
.cargo/config.toml
format is not covered by the RFC process. Is this correct?The new functionality is marked unstable (
-Z member-configs
).