rust-bitcoin / rust-secp256k1

Rust language bindings for Bitcoin secp256k1 library.
Creative Commons Zero v1.0 Universal
341 stars 262 forks source link

Backport of #735 to secp256k1-sys 0.10 #736

Closed apoelstra closed 3 weeks ago

apoelstra commented 3 weeks ago

This backports #735. I am PR'ing to the secp256k1-0.29.x branch because this will work and because it feels like unnecessary complication to try to create a secp256k1-sys-0.10.x branch, which might be "more correct" but would make our git tree harder to understand and maintain.

As in #735, this just deletes some dead code from secp256k1-sys/depend/secp256k1.

Kixunil commented 3 weeks ago

Do we want to ignore CI?

apoelstra commented 3 weeks ago

Yes. We only just started making an effort to fix CI to pin tooling last week. We don't want to try to backport all those changes.

apoelstra commented 3 weeks ago

Forgot to update the lockfiles (which in 0.29.x exist but are not checked in CI). Will get my local CI working on this before merging..

apoelstra commented 3 weeks ago

Note to self: needed to add

SECP_VENDOR_VERSION_CODE=0_10_0 \

to my local CI script to get it to pass, since by default it wants the symbols in secp-sys to match the version in Cargo.toml exactly. (But doing this would turn a non-breaking change into a breaking one, which is why I didn't.)

Kixunil commented 3 weeks ago

@apoelstra your CI fail made me realize we should do the same thing here as we do in bitcoinconsensus.

apoelstra commented 3 weeks ago

What in particular?

Kixunil commented 3 weeks ago

Version the API, not the library and put the upstream version into metadata instead.

apoelstra commented 3 weeks ago

Agreed. Though for purposes of this PR, we can't make such a change (as it'd be a breaking change to change our version format at all).

Kixunil commented 3 weeks ago

Well, we already are breaking our format here, but obviously I'm not NACKing this.

apoelstra commented 3 weeks ago

obviously I'm not NACKing this

Will you ACK it? :}

Fine if you don't want to, but there aren't a lot of maintainers on this repo and you're the one who appears to be active right now.

Kixunil commented 3 weeks ago

I already did and since GH didn't dismiss my review I haven't noticed the force push.

apoelstra commented 3 weeks ago

Tagged and published.