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

Tune platform-specific crate dependencies #724

Merged
merged 4 commits into from
Feb 6, 2019
Merged

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Feb 4, 2019

Fixes #723 and tidies up a couple of details.

@@ -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
Copy link
Collaborator

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.

Copy link
Member Author

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.

Copy link
Collaborator

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.

@vks
Copy link
Collaborator

vks commented Feb 4, 2019

Looks good to me, besides my minor concern.

@dhardy
Copy link
Member Author

dhardy commented Feb 6, 2019

Whoops, I pushed my fix for #725 to the same PR! @fizyk20 @vks this is a breaking change for anyone who implements Distribution and overrides this function (probably never necessary and I haven't seen any evidence of this). Because of this I'm less comfortable committing this now, though it's probably okay. Thoughts?

@vks
Copy link
Collaborator

vks commented Feb 6, 2019

Because of this I'm less comfortable committing this now, though it's probably okay. Thoughts?

I think we should consider it a breaking change then and only include it for 0.7.

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.

None yet

2 participants