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

Support cargo:rustc-cfg in build.rs #4296

Merged
merged 3 commits into from May 7, 2020

Conversation

robojumper
Copy link
Contributor

Fixes #4238.

@@ -83,6 +83,7 @@ pub struct PackageData {
pub dependencies: Vec<PackageDependency>,
pub edition: Edition,
pub features: Vec<String>,
pub cfgs: Vec<PathBuf>,
Copy link
Member

Choose a reason for hiding this comment

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

TIL BuildScript::cfgs in cargo_metadata crate is Vec<PathBuf> !

But why ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might well be a copy-paste error in cargo_metadata; the cargo documentation says:

single identifier, or any arbitrary key/value pair [...] key should be a Rust identifier, the value should be a string

which should well be covered by String rather than PathBuf/OsString. Should I send an issue/PR to cargo_metadata?

Copy link
Member

Choose a reason for hiding this comment

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

Sound reasonable !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oli-obk/cargo_metadata#112 has been merged and I changed the code to minimize the needed changes once that hits crates.io.

pub fn insert_cfgs(&mut self, iter: impl IntoIterator<Item = SmolStr>) {
iter.into_iter().for_each(|cfg| match cfg.find('=') {
Some(split) => self
.insert_key_value(cfg[0..split].into(), cfg[split + 1..].trim_matches('"').into()),
Copy link
Member

Choose a reason for hiding this comment

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

In theory, we should unescape the RHS here, but in practice, I don't think it is relevant here, right?

@edwin0cheng
Copy link
Member

LGTM !

@@ -53,4 +53,13 @@ impl CfgOptions {
pub fn insert_features(&mut self, iter: impl IntoIterator<Item = SmolStr>) {
iter.into_iter().for_each(|feat| self.insert_key_value("feature".into(), feat));
}

/// Shortcut to set cfgs
pub fn insert_cfgs(&mut self, iter: impl IntoIterator<Item = SmolStr>) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this does not belong here (and the above inseret_features probably as well) -- CfgOptions should a be pretty abstract thing, exactly what rustc sees.

Lowering various command-line args to this data structure should happen where we get the args.

@matklad
Copy link
Member

matklad commented May 7, 2020

Merging this as is, I'll fix up this myself (want to keep the number of open PR lower)

bors r+

@bors
Copy link
Contributor

bors bot commented May 7, 2020

@bors bors bot merged commit 1b136aa into rust-lang:master May 7, 2020
@robojumper robojumper deleted the 4238-build-rs-cfgs branch May 31, 2020 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Types from scrap crate don't resolve
3 participants