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

The dependencies of the bigint feature on rand and rustc_serialize should be optional. #151

Closed
briansmith opened this issue Jan 6, 2016 · 9 comments

Comments

@briansmith
Copy link
Contributor

My builds are failing on Nightly because I depend on num and the rand crate fails to build on Nightly. My crate only depends on rand because it depends on BigInt. I tried to build without the rand dependency but that made BigInt unavailable.

Instead all of BigInt being dependent on the rand feature, only the random-related parts of Bigint should depend on the rand feature. And, in particular, when building num without the rand feature, the rest of BigInt should work.

@briansmith
Copy link
Contributor Author

See rust-lang/rust#30713.

@briansmith
Copy link
Contributor Author

Oops, here's the real issue: If you build with the bigint feature (which is the default), then there's a non-optional dependency on rand and rustc_serialize. There should be some way to make that dependency optional and for users of num to build without those dependencies.

@briansmith briansmith changed the title Building without the rand feature completely disables BigInt The dependencies of the bigint feature on rand and rustc_serialize should be optional. Jan 6, 2016
@cuviper
Copy link
Member

cuviper commented Jan 6, 2016

Sure, making these fully optional would be welcome.

It used to be mandatory for bigint to have rust-serialize for ToHex, but I nixed that. The remaining derives could be configured like they are in complex and rational.

So these two would be dropped from bigint dependencies, then added to default feature list instead.

@briansmith
Copy link
Contributor Author

Just to be clear, you would accept this?:

  • Add two new features: serialize and rand.
  • Add those two features to the default feature set.
  • Make the FromHex/ToHex parts of bigint conditional on the serialize feature.
  • Make the rand-dependent parts of bigint conditional on the rand feature.
  • Remove the dependencies on rustc_serialize and rand from the bigint feature.

@briansmith
Copy link
Contributor Author

My pull request #154 fixes this.

Add two new features: serialize and rand.

Optional dependencies are handled like features, so I didn't need to do this.

Add those two features to the default feature set.

I did this.

Make the FromHex/ToHex parts of bigint conditional on the serialize feature.

I made it conditional on the "rustc_serialize" feature.

Make the rand-dependent parts of bigint conditional on the rand feature.

I did this, but I also had to enable the rand functionality for all test builds because some of the tests are randomized using the rand functionality.

Remove the dependencies on rustc_serialize and rand from the bigint feature.

I did this.

@cuviper
Copy link
Member

cuviper commented Jan 8, 2016

#154 is merged.

@cuviper cuviper closed this as completed Jan 8, 2016
@briansmith
Copy link
Contributor Author

Thanks for the quick turnaround. Is there going to be a release to crates.io soon that will include this change? Thanks!

@cuviper
Copy link
Member

cuviper commented Jan 8, 2016

I suppose we should. I don't like publishing a release with dirty CI, but this change in particular will help others get around the rand issues.

@cuviper
Copy link
Member

cuviper commented Jan 8, 2016

num 0.1.30 is now published.

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

No branches or pull requests

2 participants