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

global-context depends on rand-std #246

Merged
merged 1 commit into from Nov 11, 2020
Merged

global-context depends on rand-std #246

merged 1 commit into from Nov 11, 2020

Conversation

vorot93
Copy link
Contributor

@vorot93 vorot93 commented Nov 6, 2020

Uses thread_rng which is only available with rand/std / rand-std enabled.

@real-or-random
Copy link
Collaborator

Oh yes, it seems we missed this...

@apoelstra
Copy link
Member

Can you also add global-context to the features list of the no_std_test crate?

Unfortunate that it's such a PITA to test a feature matrix with no-std..

@vorot93
Copy link
Contributor Author

vorot93 commented Nov 8, 2020

@apoelstra done

@elichai
Copy link
Member

elichai commented Nov 9, 2020

Can you also add global-context to the features list of the no_std_test crate?

Unfortunate that it's such a PITA to test a feature matrix with no-std..

I don't get it. the global context will not work in the no-std tests, as it uses std :)

@apoelstra
Copy link
Member

Oh, derp, I was confused.

How can we test this then?

@apoelstra
Copy link
Member

To be clear, I am asking for a cargo invocation that will fail without this PR.

@elichai
Copy link
Member

elichai commented Nov 11, 2020

To be clear, I am asking for a cargo invocation that will fail without this PR.

gotcha,
so the problem here is that we have rand with all features as a dev-dependency, meaning that all tests enable the std feature of rand

rand = "0.6"

I see 2 ways to write such a test if we want:

  1. Add another crate that depends on secp and will do it.
  2. Stop using thread_rng for tests and disable default-features for rand in the dev-dependencies.
    2.a. We can replace thread_rng with a simple Pcg32 rng seeding it either deterministically with a hard coded seed or with the system time.
    2.b. We can hard code keys in tests so we don't need any randomness.

@elichai
Copy link
Member

elichai commented Nov 11, 2020

The diff for 2.a is basically: https://gist.github.com/elichai/dbbb0360dd0a6a6180a2e749588a05f9

@apoelstra
Copy link
Member

Thanks!

I think any of those options is too much work and it's fine to accept this PR as is (but @vorot93 can you drop the extra commit testing with no_std? I was confused and wrong to ask for that).

@vorot93
Copy link
Contributor Author

vorot93 commented Nov 11, 2020

@apoelstra done

@apoelstra
Copy link
Member

I just removed the commits directly (sorry if that was too invasive). Will merge after CI passes.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ack.

Merging; travis has passed but github is waiting for yet-unmerged workflows to be executed.

@apoelstra apoelstra merged commit 221254b into rust-bitcoin:master Nov 11, 2020
@vorot93 vorot93 deleted the patch-2 branch November 12, 2020 10:07
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

4 participants