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
Tune platform-specific crate dependencies #724
Conversation
Both usages have moved to sub-crates now
@@ -18,7 +18,9 @@ rand_core = { path = "../rand_core", version = "0.4" } | |||
log = { version = "0.4", optional = true } | |||
|
|||
[target.'cfg(any(target_os = "macos", target_os = "ios"))'.dependencies] | |||
libc = "0.2" | |||
# We don't need the 'use_std' feature and depending on it causes | |||
# issues due to: https://github.com/rust-lang/cargo/issues/1197 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this statement is slightly misleading, the problem is that we use the use_std
feature even for no_std
builds. Would it be better if we tie libc's use_std
feature to our std
feature? I'm not sure how cargo works, but I imagine it might avoid recompiling the libc crate, when it is already required elsewhere with the use_std
feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I believe features are unified before the build takes place, so if another crate depends on libc
with use_std
it will just get built once with the feature enabled. We don't actually need use_std
so there's no reason to depend on it.
The problem pointed out in #723 is that rand_jitter
depending on libc
with default features for a different target causes libc
to be built with default features when depended on without by other crates in the build — obviously unintended functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, then we should not use use_std
in any case.
Looks good to me, besides my minor concern. |
Whoops, I pushed my fix for #725 to the same PR! @fizyk20 @vks this is a breaking change for anyone who implements |
I think we should consider it a breaking change then and only include it for 0.7. |
Fixes #723 and tidies up a couple of details.