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

Jump to bottom

Fix no_std compatibility issues #1173

Merged
merged 5 commits into from Sep 10, 2021
Merged

Fix no_std compatibility issues #1173

merged 5 commits into from Sep 10, 2021

Conversation

bhgomes
Copy link
Contributor

@bhgomes bhgomes commented Sep 4, 2021

  • Using features = ["min_const_gen"] on no_std fails to compile because of a bad import.
  • The sample_efraimidis_spirakis algorithm only requires alloc but it was marked with a std requirement.
  • Replaced std with core in places where the standard library uses core.

@bhgomes
fix no_std/alloc/std compatibility issues
1544c72 
- using min_const_gen on no_std fails to compile because of a bad import
- sample_efraimidis_spirakis only requires alloc but it was marked with
  a std requirement
dhardy
dhardy reviewed Sep 5, 2021
View reviewed changes
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Thanks. Any tests you'd recommend adding (.github/workflows/test.yml)?

src/distributions/uniform.rs Outdated Show resolved Hide resolved
src/distributions/uniform.rs Outdated Show resolved Hide resolved
@bhgomes
Copy link
Contributor Author

bhgomes commented Sep 6, 2021

I added min_const_gen to the tests on the workflow. It's hard to ensure consistency with core with just a test since this package has so many features and we don't already test every combination of features. I'd say its just better to always import from core and then if they don't have it, use alloc, then std.

@bhgomes
Copy link
Contributor Author

bhgomes commented Sep 6, 2021

I also had to revert sample_efraimidis_spirakis back since I hadn't noticed f64::powf was not stable yet.

@vks
Copy link
Collaborator

vks commented Sep 6, 2021

I also had to revert sample_efraimidis_spirakis back since I hadn't noticed f64::powf was not stable yet.

We could use our libm fallback for this, then it should work with alloc.

vks
vks reviewed Sep 7, 2021
View reviewed changes
.github/workflows/test.yml Outdated
cargo build --target ${{ matrix.target }} --no-default-features --features alloc,getrandom,small_rng,min_const_gen
cargo test --target ${{ matrix.target }} --lib --tests --no-default-features --features=alloc,getrandom,small_rng,min_const_gen
cargo build --target ${{ matrix.target }} --no-default-features --features alloc,getrandom,small_rng
cargo test --target ${{ matrix.target }} --lib --tests --no-default-features --features=alloc,getrandom,small_rng
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't min_const_gen work on stable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it would, but I was getting errors in the CI, but perhaps they were other kinds of errors. I don't have a good way to test the CI, so maybe one of the maintainers should put the flag in the right place.

@bhgomes
Copy link
Contributor Author

bhgomes commented Sep 7, 2021

We could use our libm fallback for this, then it should work with alloc.

I'm not sure how to invoke the libm fallback. Perhaps one of you could help me with that part.

@vks
Copy link
Collaborator

vks commented Sep 7, 2021 •
edited

@bhgomes

I'm not sure how to invoke the libm fallback. Perhaps one of you could help me with that part.

It's not something we are currently using in this crate, but rather in rand_distr. The idea is to use num_traits::Float instead of std, because the former supports no_std via the libm crate. However, I think it might be better to do this in a follow-up PR, so we can fix the compatibility issue first.

Furthermore, we need to decide whether we are actucally ok with introducing an additional dependency into rand, because many users are sensitive about this.

@bhgomes
Copy link
Contributor Author

bhgomes commented Sep 7, 2021

Ok, makes sense. The min_const_gen fix is really the priority of this PR anyway.

vks
vks requested changes Sep 9, 2021
View reviewed changes
Copy link
Collaborator

@vks vks left a comment

Choose a reason for hiding this comment

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

I think this looks good now, except that min_const_gen is no longer tested on stable. This needs to be fixed before merging.

@connec connec mentioned this pull request Sep 10, 2021
Merged
@connec
Copy link
Contributor

connec commented Sep 10, 2021

The nightly test issue looks to be related to rust-lang/packed_simd#329. That's been fixed with a point release, so I think the tests would pass if they could be re-run?

vks
vks approved these changes Sep 10, 2021
View reviewed changes
Copy link
Collaborator

@vks vks left a comment

Choose a reason for hiding this comment

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

This should be fine now with the changes from #1175.

@vks vks merged commit fcc5baf into rust-random:master Sep 10, 2021
@vks
Copy link
Collaborator

vks commented Sep 10, 2021

Thanks for the contribution, @bhgomes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhardy dhardy dhardy left review comments

@vks vks vks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bhgomes @vks @connec @dhardy