8000 Make secp256k1/rand a dev-dependency by elichai · Pull Request #315 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Make secp256k1/rand a dev-dependency #315

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.

Sign up for GitHub

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
Merged
merged 1 commit into from
Aug 16, 2019
Merged

Make secp256k1/rand a dev-dependency #315

merged 1 commit into from
Aug 16, 2019

Conversation

elichai
Copy link
Member
@elichai elichai commented Aug 16, 2019

Right now we automatically compile rust-secp with the rand feature for no reason.
We need rand only in tests.

@apoelstra
Copy link
Member

Does this actually work? rust-lang/cargo#1796

@codecov-io
Copy link

Codecov Report

Merging #315 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #315   +/-   ##
=======================================
  Coverage   81.85%   81.85%           
=======================================
  Files          38       38           
  Lines        6951     6951           
=======================================
  Hits         5690     5690           
  Misses       1261     1261

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 166e2bd...3bab3e7. Read the comment docs.

@elichai
Copy link
Member Author
elichai commented Aug 16, 2019

I did not know about this bug.
Looking at the Cargo.lock it doesn't work.
But if I use secp without rand and import rand separately in dev-deps it seems to work

@tarcieri
Copy link
tarcieri commented Aug 16, 2019 •
edited
Loading

rust-lang/cargo#1796 is a slightly different issue, where if you have crate foobar in [dependencies], and crate baz in [dev-dependencies], and baz activates feature quux of foobar, then feature quux will be activated in release builds of the parent package/crate.

It does not impact this case, where you as the parent package/crate are activating different features of the same crate in the [dependencies] vs [dev-dependencies]

@elichai
Copy link
Member Author
elichai commented Aug 16, 2019 •
edited
Loading

@tarcieri so why running cargo build on this branch (without running tests) shows rand in the secp256k1 section of Cargo.lock?

(i'll try to see if rand gated features in secp are available here or not)

@tarcieri
Copy link
tarcieri commented Aug 16, 2019 •
edited
Loading

@elichai because of rust-lang/cargo#1796

However, if you were to include the bitcoin crate as a dependency of another crate, that would not be the case.

@elichai
Copy link
Member Author
8000 elichai commented Aug 16, 2019

oh ok. but still #1796 makes this impossible.

@tarcieri
Copy link
tarcieri commented Aug 16, 2019 •
edited
Loading

If this PR were merged, rust-lang/cargo#1796 is irrelevant to activation of secp256k1/rand in the case that a crate is depending on the bitcoin crate.

Before this change, crates depending on bitcoin will see the secp256k1/rand feature activated.

After this change, crates depending on bitcoin will NOT see secp256k1/rand activated.

An example of where rust-lang/cargo#1796 would come into play is if a hypothetical crate had this in their Cargo.toml (prior to this PR):

[dependencies]
secp256k1 = "*"

[dev-dependencies]
bitcoin = "*"

Such a crate would see secp256k1/rand activated in release builds, because bitcoin is presently (without this PR) activating it.

@apoelstra
Copy link
Member

Ah! Makes sense, thanks Tony!

@stevenroose stevenroose merged commit 6651cf9 into rust-bitcoin:master Aug 16, 2019
@elichai
Copy link
Member Author
elichai commented Aug 16, 2019

Nice :)
I can confirm that I locally created a separate crate and imported this and rand wasn't part of the dependency graph:
https://gist.github.com/elichai/d7c9da99cd69c06daf788f5ffd70e0f3

@elichai elichai deleted the 2019-08-deps branch August 16, 2019 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevenroose stevenroose stevenroose approved these changes

@apoelstra apoelstra apoelstra 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.
5 participants
@elichai @apoelstra @codecov-io @tarcieri @stevenroose
0