-
Notifications
You must be signed in to change notification settings - Fork 34
Relax cc dependency requirements, bump secp256k1 to 0.15 and bump version to 0.17.0 #9
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
Conversation
Travis fails because it runs tests with rustc 1.14 which rust-secp256k1 0.13 doesn't support anymore. No idea why the appveyor/windows tests fail. |
…ed by secp256k1 0.13
Added commit to update the tests in travis and appveyor. |
We should go straight to 0.14 - rust-bitcoin/rust-secp256k1#127 |
We can't simply update to secp256k1 0.14 because
Looks like rust-bitcoinconsensus only imports rust-secp to link the C++ code against it. It would make sense to make this crate compile let its own libsecp now. |
What a mess :( |
Maybe a solution could be to make the compiling of libsecp in |
@elichai letting rust-secp users replace the C bindings without replacing the ffi module is a recipe for horrifying UB |
I hate linking problems. I don't see an easy solution to it without either side doing something weird. (adding some compilation condition to add/remove functions/libraries) |
I have an idea on how to solve this. If you think it's a good idea I can write that and open a PR here |
@elichai Good idea imo! |
Perhaps longer term it should be possible to add prefixes to libsecp symbols, similar to what boringssl does (briansmith/ring#849). |
Longer term :) Hopefully we can find a solution that does not require |
Added commit to bump secp256k1 to 0.14(.1) |
Shouldn't this be a major bump assuming these changes can break stuff if someone has an older rust-secp256k1? |
Good point @elichai. Bumped to 0.17.0 |
Looks good! any ideas why it doesn't compile on Windows?(appveyor not passing) Edit: I see now that it's broken since #7, so nvm. |
Pssh, new errors... I have no idea. |
arghh. Could we relax cc more now that upstream has CI for both MSVC and Linux for 1.16.0? (If anyone knows of a way to test the windows builds on linux I can test different versions of cc before we relax anything) |
As far as I can tell we'd have to
|
I think it's fine to support any |
This seem to fix it: rust-bitcoin/rust-secp256k1#131 Tests: https://ci.appveyor.com/project/elichai/rust-bitcoinconsensus/builds/26027109 It did also require enabling the recovery module because of missing |
@elichai great, thanks! |
checked OSX build and link with current rust-bitcoin master and secp256k1 v0.15.0 |
checked Linux build and link with current rust-bitcoin master and secp256k1 v0.15.0 |
The current latest version of rust-bitcoinconsensus (0.16.4) is not compatible with secp256k1 0.13 since the cc requirements were relaxed (rust-bitcoin/rust-secp256k1#103). The result is that when we update to secp256k1 in rust-bitcoin rust will pick bitcoinconsensus 0.16.3. The problem is that rust 1.22 has a bug where the attempt to resolve dependencies in such a way leads to an infinite loop. My hypothesis is that this PR will fix the problem and allow rust-bitcoin to update its secp256k1 version (see PR rust-bitcoin/rust-bitcoin#278).