chore: check external types in CI#643
Conversation
Performance Comparison Report
|
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5937.2 Mbps | 7922.1 Mbps | -25.1% | 97.3% / 98.8% |
| medium-concurrent | 5489.8 Mbps | 7768.1 Mbps | -29.3% | 96.4% / 97.9% |
| medium-single | 4159.1 Mbps | 4748.9 Mbps | -12.4% | 96.7% / 99.0% |
| small-concurrent | 3851.0 Mbps | 5250.4 Mbps | -26.7% | 98.0% / 99.9% |
| small-single | 3496.0 Mbps | 4774.1 Mbps | -26.8% | 96.5% / 98.7% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3072.3 Mbps | 4078.6 Mbps | -24.7% |
| lan | 782.4 Mbps | 800.8 Mbps | -2.3% |
| lossy | 69.8 Mbps | 55.9 Mbps | +24.9% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 24.1% slower on average
428d2610874ba82d607c26d94271eb9549029a98 - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5403.7 Mbps | 7866.1 Mbps | -31.3% | 94.9% / 98.8% |
| medium-concurrent | 5259.5 Mbps | 7632.0 Mbps | -31.1% | 95.0% / 99.6% |
| medium-single | 3591.4 Mbps | 4750.0 Mbps | -24.4% | 96.8% / 150.0% |
| small-concurrent | 3838.1 Mbps | 5288.0 Mbps | -27.4% | 91.3% / 99.5% |
| small-single | 3464.0 Mbps | 4827.8 Mbps | -28.2% | 92.6% / 101.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3072.9 Mbps | 4121.2 Mbps | -25.4% |
| lan | 782.4 Mbps | 810.4 Mbps | -3.5% |
| lossy | 69.9 Mbps | 69.8 Mbps | ~0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 27.9% slower on average
ec9ab8bfdab7cf293e25449cc1944190584adcd6 - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5461.4 Mbps | 8051.5 Mbps | -32.2% | 98.2% / 149.0% |
| medium-concurrent | 5441.7 Mbps | 7930.1 Mbps | -31.4% | 97.2% / 148.0% |
| medium-single | 4271.6 Mbps | 4658.7 Mbps | -8.3% | 93.5% / 101.0% |
| small-concurrent | 3861.4 Mbps | 5251.6 Mbps | -26.5% | 95.6% / 102.0% |
| small-single | 3564.2 Mbps | 4733.1 Mbps | -24.7% | 98.9% / 153.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3073.2 Mbps | 4064.5 Mbps | -24.4% |
| lan | 782.4 Mbps | 810.4 Mbps | -3.5% |
| lossy | 69.8 Mbps | 55.8 Mbps | +25.1% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 25.3% slower on average
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/643/docs/noq/ Last updated: 2026-05-11T12:37:18Z |
flub
left a comment
flub
left a comment
There was a problem hiding this comment.
Looking forward to having this!
| # Pin to the nightly that the pinned `cargo-check-external-types` | ||
| # release was last tested against. Update both together. | ||
| CARGO_CHECK_EXTERNAL_TYPES_VERSION: "0.4.0" | ||
| CARGO_CHECK_EXTERNAL_TYPES_NIGHTLY: "nightly-2025-10-18" |
There was a problem hiding this comment.
this seems to be the wrong version because CI is currently failing?
There was a problem hiding this comment.
This version is correct but it's not passed to the cargo make invocation. Will fix.
| script = ''' | ||
| set -e | ||
| fail=0 | ||
| for crate in noq-udp noq-proto noq; do | ||
| echo "::group::$crate" | ||
| if ! cargo +nightly check-external-types \ | ||
| --manifest-path "$crate/Cargo.toml" \ | ||
| --features __all_without_fips; \ | ||
| then | ||
| fail=1 | ||
| fi | ||
| echo "::endgroup::" | ||
| done | ||
| exit $fail | ||
| ''' |
There was a problem hiding this comment.
Can you not set workspace = true and avoid having to iterate the crates here in a script?
There was a problem hiding this comment.
where would i set workspace = true? cargo check-external-types has no support for a workspace AFAICS (but there's an open PR for it)
There was a problem hiding this comment.
Ah right, on the cargo make level. Should be possible indeed, trying this out now.
There was a problem hiding this comment.
It does work, but then we also have bench, fuzz, perf, book being checked which we don't want, so I'd keep the manual loop over the crates.
There was a problem hiding this comment.
I think you can do this as well: https://sagiegurari.github.io/cargo-make/#usage-workspace-support-skip-include-members
I'd still prefer that over manually writing this script I think.
There was a problem hiding this comment.
FWIW, I'd like cargo make to be something everyone can run locally before pushing to a PR. And a script like this will break at least on windows. Hence why I'm looking to avoid this. (And yes, being able to execute this check locally is great!)
There was a problem hiding this comment.
I managed to remove the script.
| "arbitrary::Arbitrary", # gated behind `arbitrary` feature | ||
| "criterion::Criterion", # gated behind `bench` feature |
There was a problem hiding this comment.
Is it worth not including those and not enabling those features for the check instead? Or is that more effort than it is worth it? I guess the odds of those accidentally ending up in our real API are pretty small.
There was a problem hiding this comment.
Yeah, we can also list the features manually instead. Wasn't sure which way was "safer" for us.
There was a problem hiding this comment.
Not entirely sure but I think I'd prefer keeping Arbitrary listed and running the check with all features enabled. Otherwise we could still sneak in new types under feature flags.
Note that I didn't use --all-features but a new __all_without_fips feature, because the FIPS feature of aws-lc-rs fails to build on my machine because my GCC apparently is too new. YMMV. Can switch to --all-features but would have to check first if our CI runners can build the FIPS feature.
| allowed_external_types = [ | ||
| "arbitrary::Arbitrary", # gated behind `arbitrary` feature | ||
| "criterion::Criterion", # gated behind `bench` feature | ||
| "identity_hash::IdentityHashable", |
There was a problem hiding this comment.
Where does this show up in our API?
There was a problem hiding this comment.
impl identity_hash::IdentityHashable for PathId {}
noq/noq-proto/src/connection/paths.rs
Line 35 in 6a114f5
There was a problem hiding this comment.
Goodness that's miserable. Why can't we keep impls like that private?
There was a problem hiding this comment.
heh, I was just complaining about rust. Wasn't asking for a solution 😄
Frando
force-pushed
the
Frando/check-external-types
branch
from
5ad40b1 to
428d261
Compare
| # Install the required toolchain | ||
| rustup -q toolchain install $TOOLCHAIN -c rustc,cargo,rust-std |
There was a problem hiding this comment.
Hum, I'm not a fan of automatically installing tools. I kind of find this an anti-pattern. Can we not error and print a message with a hint of how to install it?
I know cargo-make does have this as a built-in feature too. But I've been wondering whether we should disable that by default.
There was a problem hiding this comment.
Removed the script altogether, now it fails if the toolchain is missing.
| script = ''' | ||
| set -e | ||
| fail=0 | ||
| for crate in noq-udp noq-proto noq; do | ||
| echo "::group::$crate" | ||
| if ! cargo +nightly check-external-types \ | ||
| --manifest-path "$crate/Cargo.toml" \ | ||
| --features __all_without_fips; \ | ||
| then | ||
| fail=1 | ||
| fi | ||
| echo "::endgroup::" | ||
| done | ||
| exit $fail | ||
| ''' |
There was a problem hiding this comment.
I think you can do this as well: https://sagiegurari.github.io/cargo-make/#usage-workspace-support-skip-include-members
I'd still prefer that over manually writing this script I think.
| allowed_external_types = [ | ||
| "arbitrary::Arbitrary", # gated behind `arbitrary` feature | ||
| "criterion::Criterion", # gated behind `bench` feature | ||
| "identity_hash::IdentityHashable", |
There was a problem hiding this comment.
heh, I was just complaining about rust. Wasn't asking for a solution 😄
2 participants
Description
This adds a
cargo maketask and CI job to ensure that no foreign crates appear in the public API apart from those explicitly allow-listed.Change checklist