Commit "minimal" and "latest" lockfiles #591
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `cbor` crate has been unmaintained for several years, and depends on the ancient `rustc_serialize` crate which (a) doesn't build on WASM, and (b) doesn't build when we use a minimal-dep Cargo.lock. (The latter is because cbor specifies rustc_serialize 0.3.0 when it should specify 0.3.1, but there is nothing we can do to fix that when cbor is unmaintained.) This changes a hardcoded value in a regression test, but it's because we're replacing the serialization engine rather than changing our code, so this is not actually a change.
tcharding
previously approved these changes
Mar 19, 2023
|
The idea is then that we would each publish crev reviews for all the crates (and versions) in both lock files, right? |
Kixunil
previously approved these changes
Mar 19, 2023
| // Secret key is 32 bytes. CBOR adds a byte of metadata for 20 of these bytes, | ||
| // (Apparently, any byte whose value is <24 gets an extra byte.) | ||
| // It also adds a 1-byte length prefix and a byte of metadata for the whole vector. | ||
| assert_eq!(e.len(), 54); |
There was a problem hiding this comment.
It's really weird that different engines encode it differently but I don't care much; it's not our fault.
|
Great, thanks both of you! We need a rust-secp maintainer to ACK before I can merge but it sounds like we're agreed on strategy here. |
This is only needed for the serde test, so don't bother putting it in the README. Downstream users won't encounter this dependency and don't need to care about it.
|
@elichai @sanket1729 @jonasnick could one of you take a look and ACK this? |
sanket1729
approved these changes
Mar 30, 2023
There was a problem hiding this comment.
ACK bd9d3c9. Verified the lockfiles. The latest one has a couple lines diff, but that is expected :) . minimal one is the same
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a proposed strategy for maintaining tested lockfiles in the rust-bitcoin ecosystem. The idea is that we would have both a "minimal" and a "latest" lockfile, and in both cases we have trusted crev reviews for all dependencies (this is not implemented here, I'd like to start by committing the lockfiles so we can agree what to review).
Periodically we can update the "latest" one to reflect new versions of deps that we've gotten around to reviewing.
I have local nix build scripts that are able to test every commit of proposed PRs against both lockfiles. In CI it is probably reasonable to at least do
cargo test --locked --all-featureswith both of them, to make sure that the tests at least pass on each PR with them.Thoughts?