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

allow Cargo.toml pkg metadata as config source #657

Closed
Emilgardis opened this issue Mar 17, 2022 · 7 comments
Closed

allow Cargo.toml pkg metadata as config source #657

Emilgardis opened this issue Mar 17, 2022 · 7 comments
Labels
A-config Area: cross-toml config enhancement

Comments

@Emilgardis
Copy link
Member

Ideally I would like to get rid of Cross.toml eventually and use package.metadata.cross in Cargo.toml to store cross configuration.

Originally posted by @reitermarkus in #624 (comment)

@Emilgardis
Copy link
Member Author

This would be a good opportunity to switch to a serde deserialize implementation of getting configs

@reitermarkus
Copy link
Member

To get the config, we can use https://docs.rs/cargo/0.60.0/cargo/util/config/struct.Config.html.

We'll just need to parse some more arguments to get all information for calling https://docs.rs/cargo/0.60.0/cargo/util/config/struct.Config.html#method.configure.

@Emilgardis
Copy link
Member Author

the cargo lib is quite heavy, not sure it's worth it

@mntns
Copy link
Contributor

mntns commented Mar 22, 2022

I just submitted a PR for using serde to deserialize the Cross.toml and would be happy to integrate the Cargo.toml as config source if it gets merged.

bors bot added a commit that referenced this issue Mar 22, 2022
670: Implement `Cross.toml` deserialization using `serde` r=Emilgardis a=mntns

This PR implements the deserialization of the `Cross.toml` config using `serde`. It can therefore be seen as some preliminary work on the issues #657, #664, and is related to #532 on #624.

I added some basic documentation as module-level documentation, although I'm not sure whether this is the best place for that. Ideally it should be documentated in the `README.md` at some point. Furthermore, `CrossToml` lives in its own module, but a more suitable place might be the `config` module.

Co-authored-by: Niklas Kunz <git@monoton.space>
@Emilgardis
Copy link
Member Author

Should it be valid to have configuration in both Cross.toml and Cargo.toml? I think not.

If we do allow it, should we panic when a value exists twice?

@mntns
Copy link
Contributor

mntns commented Jun 4, 2022

I just submitted a PR for that feature. For now, a warning will be shown if there are a Cross.toml and configuration values in the Cargo.toml at the same time, making the two configuration sources mutually exclusive.

Should the Cargo.toml method be the primary form of configuration? That could then be reflected more explicitly in the documentation.

@Alexhuszagh
Copy link
Contributor

Should the Cargo.toml method be the primary form of configuration? That could then be reflected more explicitly in the documentation.

I think so, our goal was to phase out Cross.toml I believe (which wouldn't be anytime soon), so first having Cargo.toml be the primary form of configiration, then warning about Cross.tomlif it exists a few releases later would be good.

bors bot added a commit that referenced this issue Jun 23, 2022
842: Allow `Cargo.toml` as configuration source r=Emilgardis a=mntns

Due to a GitHub mishap in #754, this is a new PR containing the same commits (rebased by `@Emilgardis).` Just for reference, it still addresses #657.

Co-authored-by: Niklas Kunz <git@monoton.space>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-config Area: cross-toml config enhancement
Projects
None yet
Development

No branches or pull requests

4 participants