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

[New custom build] Implement platform-specific dependencies #735

Closed
wants to merge 3 commits into from

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Oct 21, 2014

cc #610

@@ -23,7 +23,8 @@ pub enum ResolveMethod<'a> {
ResolveEverything,
ResolveRequired(/* dev_deps = */ bool,
/* features = */ &'a [String],
/* uses_default_features = */ bool),
/* uses_default_features = */ bool,
/* target_platform = */ Option<String>),
Copy link
Member

Choose a reason for hiding this comment

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

This is actually called pretty frequently due to the recursive nature of resolve, and I think this could get away with the more cheap Option<&'a str>, would that be possible?

@alexcrichton
Copy link
Member

Looks fantastic, thanks so much @tomaka! As mentioned on IRC I think I'll wait to merge this until the RFC gets merged, but hopefully that won't take too long!

@tomaka
Copy link
Contributor Author

tomaka commented Oct 21, 2014

Updated

@tomaka
Copy link
Contributor Author

tomaka commented Oct 21, 2014

Oops, I forgot to remove the call to rustc_version in cargo_rustc::Context

@tomaka
Copy link
Contributor Author

tomaka commented Oct 21, 2014

Updated

let platform = match method {
ResolveRequired(_, _, _, ref platform) => {
platform.clone()
},
Copy link
Member

Choose a reason for hiding this comment

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

It's probably ok to omit the ref + clone() here (as well as below)

@alexcrichton
Copy link
Member

Looks fantastic, thanks @tomaka! I'll hold off on this pending the resolution of rust-lang/rfcs#403.

@tomaka tomaka force-pushed the platform-specific-deps branch 2 times, most recently from 77ebc37 to a3c5b06 Compare October 23, 2014 09:54
@tomaka tomaka changed the title Implement platform-specific dependencies [New custom build] Implement platform-specific dependencies Oct 23, 2014
@alexcrichton
Copy link
Member

BTW: I added a location in which we'll need to upload this information to the registry, you'll just need to fill it out: https://github.com/rust-lang/cargo/blob/master/src/cargo/ops/registry.rs#L75

@tomaka
Copy link
Contributor Author

tomaka commented Oct 28, 2014

BTW: I added a location in which we'll need to upload this information to the registry, you'll just need to fill it out: https://github.com/rust-lang/cargo/blob/master/src/cargo/ops/registry.rs#L75

Done

@alexcrichton
Copy link
Member

Ah I knew I forgot one other location! https://github.com/rust-lang/cargo/blob/master/src/cargo/sources/registry.rs#L420 (I can fix this up after merging as well, which can hopefully happen today!)

@tomaka
Copy link
Contributor Author

tomaka commented Oct 28, 2014

I fixed it too

@alexcrichton
Copy link
Member

Thanks @tomaka!

@alexcrichton
Copy link
Member

RFC was approved this afternoon, let's see what bors thinks of this!

@bors bors closed this Oct 30, 2014
@tomaka tomaka deleted the platform-specific-deps branch October 31, 2014 06:00
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.

3 participants