Closed
Conversation
We forgot to add the `ordered` feature to `test_vars.sh`.
Currently the `serde` feature requires an allocator, this is a hang over from code being written for the `bitcoin` crate. This was not found because we are not testing `primitives` correctly in CI (there is the `fuzz` bug and also `primitives` is not even in the CRATES list).
This should have been done when we introduced the `primitives` crate - epic fail. Intentionally put it before `fuzz` because of the known bug in the CI script where nothing after `fuzz` gets tested.
The `hash_reader` function is new and unreleased, it should never have been put into the `sha256t_hash_newtype` macro, and its broken.
The `hash_reader` function is only available when `bitcoin-io` is enabled - it should be feature gated.
We lost this impl at some stage in the last few weeks and did not notice because of a bug in `run_task` [0]. Implement `JsonSchema` for `siphash` as we do for all the other hash types. [0] rust-bitcoin/rust-bitcoin-maintainer-tools#10
In rust-bitcoin#3010 we added a `length` field to the `sha256::Midstate` which broke the `schemars` test but we did not notice because of the current bug [0] in the `run_task` CI script. [0] rust-bitcoin/rust-bitcoin-maintainer-tools#10
Note: - The `extra_test.sh` script runs for all toolchains. - The `schemars` crate does not use the repo lock files. - We need to pin some deps when building the `schemars` test. Pin in the `extra_test.sh` script, and mention it in the docs so the docs don't go stale next MSRV update. This was previously unnoticed because of the `run_task` bug. ref: rust-bitcoin/rust-bitcoin-maintainer-tools#10
In recent work making functions on hash types inherent it seems we missed `siphash`. Add inherent getters/setters to the `siphash::Hash` type and call through to them from the `Hash` trait impl.
The `serde` impls and `FromStr` are missing from `siphash`, add them.
There is a bug in `run_task` [0] that drastically reduces our test coverage. We can bypass it by putting `fuzz` last in the list. Note, we re-order all crates before `fuzz` to be in alphabetic order, this is required because when adding `primitives` we poked it in before `fuzz` to make sure it ran. [0] rust-bitcoin/rust-bitcoin-maintainer-tools#10
This was referenced Aug 22, 2024
Closed
Member
|
Oh, I see, #3195 comes first. That doesn't match the PR discription and it's also a bit annoying that you rebased the long one (and the one that is already reviewed and ACKed). I'll still merge 3200. |
Member
|
Closing in favor of #3220. |
Member
Author
Ugh! I played around with the order and actually did both locally to see the difference then picked one, obviously I did not then re-read/write the description. I never considered the ack vs no-ack when thinking which order :( |
This file contains hidden or 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
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 #3200 followed by #3195 to make it possible to merge the whole lot of CI fixes in a single PR.