Skip to content

Fix CI#3206

Closed
tcharding wants to merge 11 commits intorust-bitcoin:masterfrom
tcharding:08-22-all-ci-fixes
Closed

Fix CI#3206
tcharding wants to merge 11 commits intorust-bitcoin:masterfrom
tcharding:08-22-all-ci-fixes

Conversation

@tcharding
Copy link
Copy Markdown
Member

This is #3200 followed by #3195 to make it possible to merge the whole lot of CI fixes in a single PR.

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
@apoelstra
Copy link
Copy Markdown
Member

This looks like it's based on #2861 (though that may be an illusion because you force-pushed to #2861) rather than #3200. So I'm not sure what's going on.

I am just going to merge #3200, which has two ACKs, and let you rebase #3195.

@apoelstra
Copy link
Copy Markdown
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.

@apoelstra
Copy link
Copy Markdown
Member

Closing in favor of #3220.

@tcharding tcharding closed this Aug 22, 2024
@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Aug 22, 2024

That doesn't match the PR discription and it's also a bit annoying that you rebased the long one

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 :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-hashes PRs modifying the hashes crate C-primitives doc test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants