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 member table to cargo config #8797

Closed

Conversation

Richard-W
Copy link

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 member target which defines the target of a member crate if it is set. For the member crate this key takes precedence over the build.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).

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2020
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
@ehuss
Copy link
Contributor

ehuss commented Oct 22, 2020

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 Cargo.toml, such as in the [package] table, maybe as a default-target key (or default-targets now that Cargo supports multiple targets?). Additionally, there's some uncertainty about how more sophisticated target settings might be expressed. For example, there is a desire to be able to specify the supported targets of a package or cargo-target (#6179).

These may or may not have some interaction, but I'm not clear on how they might.

@bors
Copy link
Collaborator

bors commented Oct 27, 2020

☔ 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:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@phil-opp
Copy link
Contributor

@ehuss

I think for specifying things like this, it would probably want to lean more towards placing settings in Cargo.toml, such as in the [package] table, maybe as a default-target key (or default-targets now that Cargo supports multiple targets?).

I really like this idea and I think that it would fix most issues I have with .cargo/config files today. What do you think would be the process to implement something like this? Would it require an RFC directly or would it be ok to implement it as an unstable feature for experimentation first (to find out which .cargo/toml keys are useful in Cargo.toml)?

@Richard-W
Copy link
Author

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.

@phil-opp
Copy link
Contributor

phil-opp commented Nov 4, 2020

@Richard-W I just created a related proposal to replicate all package-specific .cargo/config settings in Cargo.toml files: https://internals.rust-lang.org/t/proposal-move-some-cargo-config-settings-to-cargo-toml/13336.

@Richard-W
Copy link
Author

Closing because there are better approaches than this one.

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.

Per crate build target in workspace
5 participants