Skip to content

Fix build fail#916

Open
dvush wants to merge 4 commits intodevelopfrom
fix-test-build
Open

Fix build fail#916
dvush wants to merge 4 commits intodevelopfrom
fix-test-build

Conversation

@dvush
Copy link
Copy Markdown
Contributor

@dvush dvush commented Apr 13, 2026

📝 Summary

  1. add --all-targets to makefile checks (otherwise we miss linting tests and benches)
  2. fix compilation errors and warnings in tests

💡 Motivation and Context


✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

@dvush dvush requested a review from ZanCorDX as a code owner April 13, 2026 14:54
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 13, 2026

Claude finished @dvush's task in 1m 15s —— View job


Review of PR #916

  • Gather context and read changed files
  • Review Makefile changes
  • Review benchmark file changes
  • Review deleted order_dumper.rs
  • Review block_orders/mod.rs changes
  • Post review summary

LGTM — this is a clean, straightforward fix. All four changes are correct and well-motivated:

Makefile (--all-targets): Adding --all-targets to both lint and fmt targets ensures clippy catches issues in tests, benches, and examples — not just lib/bin targets. The fmt target also correctly gains --workspace which it was previously missing (the lint target already had it).

Benchmark fix (txpool_fetcher.rs): The subscribe_to_txpool_with_blobs signature was updated upstream to require Arc<MempoolTxsDetector> as the third parameter. The benchmark now passes it correctly. No issues.

order_dumper.rs deletion: OrderDumper and OrderStoreAction are not referenced anywhere else in the codebase — confirmed via grep. The comment "Soon to be replaced my mockall" suggests it was already slated for removal. Clean delete.

block_orders/mod.rs: Simply removes the #[cfg(test)] mod order_dumper; declaration to match the file deletion. No other changes to this file.

No bugs, no security concerns, no performance issues.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 17, 2026

Claude finished @dvush's task in 1m 32s —— View job


PR Review

  • Read changed files
  • Review Makefile changes
  • Review benchmark changes
  • Review block_orders changes and order_dumper deletion
  • Review bundle_tests/setup.rs changes
  • Post review summary

No blocking issues found. A few minor observations:

Makefile — Adding --all-targets to lint is clearly correct. For the fmt target, note that cargo clippy --all-targets --fix will now auto-apply fixes to tests/benches/examples in addition to lib/bin. This is almost certainly desired, but worth being aware of the wider blast radius when running make fmt.

Also the test target (Makefile:160) still doesn't compile/run bench code — only cargo test on the default targets. --all-targets on clippy catches bench compile errors at lint time, which is probably sufficient, but there's no test-time coverage ensuring benches build. Not a regression, just noting.

bundle_tests/setup.rs:215#[allow(clippy::result_large_err)] is a workaround, not a fix. ExecutionError::LowerInsertedValue { before: SimValue, inplace: SimValue } (crates/rbuilder/src/building/mod.rs:576-577) is the large variant pushing the enum over the clippy threshold. Since this is test code called on the happy path, the allow is fine — but if similar warnings surface elsewhere on production paths, boxing SimValue inside the variant would be the proper fix.

txpool_fetcher.rs bench — Correctly matches the updated subscribe_to_txpool_with_blobs signature (crates/rbuilder/src/live_builder/order_input/txpool_fetcher.rs:23-28).

order_dumper.rs deletion — Verified no remaining references to OrderDumper or OrderStoreAction anywhere in the tree. Clean removal.
| Branch: fix-test-build

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 17, 2026

Claude finished @dvush's task in 46s —— View job


PR Review

  • Read newly changed files (since last review)
  • Review parallel_builder/conflict_task_generator.rs changes
  • Review parallel_builder/groups.rs changes
  • Review live_builder/mod.rs changes
  • Review utils/receipts.rs changes
  • Post review summary

No blocking issues. Latest commit 62b0adb is four mechanical Rust 1.95 clippy fixes, all semantically equivalent to the originals:

conflict_task_generator.rs:55sort_by(|a,b| b.len().cmp(&a.len()))sort_by_key(|b| Reverse(b.orders.len())). Equivalent descending sort; Reverse newtype flips Ord. ✓

groups.rs:196.chain(x.into_iter()).chain(x). Iterator::chain takes IntoIterator, so the explicit conversion was redundant. ✓

live_builder/mod.rs:374-375.iter().filter_map(|(_, r)| r.adjustment_fee_payer).values().filter_map(|r| r.adjustment_fee_payer). Same result; .values() is the idiomatic form. ✓

receipts.rs:94.zip(x.into_iter()).zip(x). Iterator::zip takes IntoIterator. ✓

Earlier-reviewed items from prior commits still stand: bundle_tests/setup.rs:215's #[allow(clippy::result_large_err)] is a workaround (root cause is the LowerInsertedValue variant embedding two SimValues — box the variant if this appears on production paths), and make test still doesn't exercise bench compilation — --all-targets on clippy is the only bench-build gate.

No bugs, no security concerns, no performance regressions.
| Branch: fix-test-build

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant