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

Fix compilation of benchmarks #505

Merged
merged 1 commit into from
Jun 4, 2020
Merged

Fix compilation of benchmarks #505

merged 1 commit into from
Jun 4, 2020

Conversation

nstoddard
Copy link
Contributor

This adds the rand_isaac dependency since that's no longer in the rand crate, but in the future we should consider switching to an RNG in rand so we don't need another dependency.

Cargo.toml Outdated Show resolved Hide resolved
@nstoddard nstoddard requested a review from kvark June 4, 2020 01:10
Copy link
Collaborator

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you!

@kvark
Copy link
Collaborator

kvark commented Jun 4, 2020

Note: there is another error on nightly that's still happening:
https://travis-ci.org/github/rustgd/cgmath/jobs/694474980#L222

Could this PR address this one too? Looks related

@nstoddard
Copy link
Contributor Author

It looks like that error is due to the use of an old version of rust nightly (which is needed for SIMD support) which rand_core doesn't support. Optional dev-dependencies aren't supported, so one way to fix this would be to move rand_isaac back to a regular dependency so it can be enabled for benchmarks but not tests, but that's not ideal since it makes it possible for other crates to enable that dependency.

Another way would be to switch to an RNG that doesn't require another dependency. Are any in rand suitable for benchmarks?

@kvark
Copy link
Collaborator

kvark commented Jun 4, 2020

Did you consider using https://docs.rs/rand/0.7.3/rand/rngs/struct.SmallRng.html for this?

@nstoddard
Copy link
Contributor Author

SmallRng requires a feature to be enabled, which would then also be enabled for any crates that enable cgmath's rand feature; is that okay? If not, would StdRng work?

@kvark
Copy link
Collaborator

kvark commented Jun 4, 2020

I think we can enable it for rand, yes. Presumably, other crates enabling rand would be interested in using this.

This switches to `SmallRng` since that doesn't require adding another dependency.
@nstoddard
Copy link
Contributor Author

Done.

@kvark
Copy link
Collaborator

kvark commented Jun 4, 2020

Thank you! let's just see what CI thinks :)

@kvark
Copy link
Collaborator

kvark commented Jun 4, 2020

@kvark kvark merged commit 2c7ee50 into rustgd:master Jun 4, 2020
@nstoddard
Copy link
Contributor Author

That's a different one than the one that was broken by this PR (current nightly rather than nightly-2019-01-01), and it won't work until #490 is implemented.

@kvark
Copy link
Collaborator

kvark commented Jun 4, 2020

Yeah, I figured, hence this PR is merged :)

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.

2 participants