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.
| std = ["alloc", "internals/std", "io/std", "units/std"] | ||
| alloc = ["internals/alloc", "io/alloc", "units/alloc"] | ||
| serde = ["dep:serde", "internals/serde", "units/serde"] | ||
| serde = ["dep:serde", "internals/serde", "units/serde", "alloc"] |
There was a problem hiding this comment.
We should try to avoid this before 1.0 because then it'll become kinda breaking. (Strictly, people should enable every feature they use directly but it's likely they'll miss it.) We should just bound those serde imlps on all(feature = "serde", feature = alloc). Preferably in this PR but I can tolerate a separate PR if it's before release.
|
|
||
| # Crates in this workspace to test (note "fuzz" is only built not tested). | ||
| CRATES=("addresses" "base58" "bitcoin" "fuzz" "hashes" "internals" "io" "units") | ||
| CRATES=("addresses" "base58" "bitcoin" "primitives" "fuzz" "hashes" "internals" "io" "units") |
There was a problem hiding this comment.
The real process mega fail is that we're no longer using automated way to get the list of the crates. I know you said this PR is not perfect but I'd really, really like to see it fixed right here rather than kicking it down the road. You should be able to just copy the command from old revisions. And regarding fuzz order, I suggest to just patch-out the crate from the list and then explicitly add it to the end (why isn't it at the end already?)
|
I'm confused by the ordering requirements of this PR and #3200 to get past your local CI, I don't see any? |
|
They both need to be merged to pass my local CI. It doesn't matter what order. |
Annoyed at me, at yourself, or at our process? |
Some combination of you and Kix that led to a "fix CI which is currently multiply broken on master" PR being mixed up with controversial non-CI changes. |
009e747 CI: Put fuzz last in CRATES list (Tobin C. Harding) aed61bd Implement FromStr and serde impls for siphash (Tobin C. Harding) f884610 siphash: Make functions inherent (Tobin C. Harding) 321d82c hashes: Pin in extra_test (Tobin C. Harding) 42c7617 Fix shchemars test (Tobin C. Harding) b6fda65 Implement JsonSchema for siphash::Hash (Tobin C. Harding) 1af6ff4 hashes: Feature gate hash_reader unit test (Tobin C. Harding) 5230d33 Remove hash_reader from sha256t_hash_newtype (Tobin C. Harding) Pull request description: This is just the bug fixes pulled out of #2861 The root problem here is the bug described in rust-bitcoin/rust-bitcoin-maintainer-tools#10 This PR works around the bug by putting `fuzz` last in the list. ACKs for top commit: apoelstra: ACK 009e747; confirmed that local CI STILL does not pass after this; needs #3195 as well; but it is improved Kixunil: ACK 009e747 but I'd much rather see the siphash problems addressed by splitting up the macro. I can make a PR for that (but just that) later. Tree-SHA512: f724570003b862e994787192b720c4698e6b904ececa7188770ee507dd9e12382ae64db3363fa15ab3f347341c99ed9a75938f4da0438ce3a0fa88717497a233
|
#3200 is in. Can you rebase this? |
|
Rebased in #3220. Closing this in favor of that. |
d03e545 ci: fix `rev` to be `ref` when pinning bitcoin-maintainer-tools (Andrew Poelstra) d04b6aa bitcoin: add a couple missing prelude imports (Andrew Poelstra) 30f6bd4 Add primitives to the CRATES list (Andrew Poelstra) e1e0230 primitives: Enable alloc from serde (Tobin C. Harding) ce36a37 primitives: Test ordered feature (Tobin C. Harding) Pull request description: This is a rebase of #3195 on master (since #3200 was merged) and adds one commit which fixes a few missing imports, which it appears was missed in the other "fix CI" PRs. ACKs for top commit: Kixunil: ACK d03e545 tcharding: ACK d03e545 Tree-SHA512: 6f5dc5c2cbb42716e83e46e70dbf4d644f9e80f6b3d2e75a276e37d33d461f4a86e67eb5c32444dbd23bb9767abedc88bfe958b1beba39852c382cf73736711f
Epic fail here,
primitivesis not being checked in CI. This is the second big CI bug we have had in as many weeks.Turns out we have a bunch
serdecode that assumes there is an allocator. Patch 2 enablesallocfromserde- this is in line with feature gating wildly onallocwhen moving code frombitcoinduring crates smashing.This PR does not claim to be the best solution but is a small step forward.