feat: Remove bindgen dependency for MTU crate#3337
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3337 +/- ##
==========================================
+ Coverage 94.30% 94.41% +0.11%
==========================================
Files 127 133 +6
Lines 38837 40081 +1244
Branches 38837 40081 +1244
==========================================
+ Hits 36626 37844 +1218
- Misses 1376 1391 +15
- Partials 835 846 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
martinthomson
left a comment
martinthomson
left a comment
There was a problem hiding this comment.
Not sure if I'm following the CI parts, but don't you want CI to (mostly) just check that what is generated matches what is in the tree? But then you might also have CI propose a PR to update what is in-tree on the affected branch.
That is probably overkill. I don't really expect these bindings to ever change, since OSs have ABI promises. Edit: But Rust version changes may cause bindgen to generate slightly different bindings. Let me look into the PR thing. |
There was a problem hiding this comment.
Pull request overview
This PR removes the bindgen build dependency from the MTU crate by replacing runtime binding generation with pre-generated platform-specific binding files. Instead of using bindgen during the build process, the PR checks in generated binding files for each supported platform (Linux, macOS, FreeBSD, NetBSD, OpenBSD, Solaris) and uses conditional compilation to select the appropriate bindings for the target platform.
Changes:
- Removed
bindgenfrom build dependencies and simplified the build script - Added pre-generated binding files for all supported platforms
- Added CI workflow steps to regenerate bindings and verify they remain up-to-date
- Updated lint attributes to use
#[expect]instead of#[allow]where appropriate
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| mtu/Cargo.toml | Removed bindgen dependency and updated cargo-machete ignore list |
| mtu/build.rs | Removed bindgen-related code, keeping only platform alias configuration |
| mtu/src/linux.rs | Updated lint attributes and changed to use static binding module path |
| mtu/src/bsd.rs | Updated lint attributes and changed to use static binding module paths per platform |
| mtu/src/bindings/*.rs | Added pre-generated binding files for all supported platforms |
| mtu/src/bindings/*.h | Added header files used as input for binding generation |
| .github/workflows/check-mtu.yml | Added CI jobs to generate bindings and verify they match committed versions |
| .github/actions/check-vm/action.yml | Added bindgen installation and binding generation steps to VM-based tests |
larseggert
force-pushed
the
feat-mtu-no-bindgen
branch
2 times, most recently
from
401f12e to
da05690
Compare
larseggert
force-pushed
the
feat-mtu-no-bindgen
branch
from
da05690 to
bd4c008
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Good approach — replacing runtime bindgen with pre-generated, CI-verified bindings eliminates the libclang build-time dependency for downstream consumers while keeping bindings provably in sync via the check-bindings job. The centralized LINUX_BINDGEN_ARGS / BSD_BINDGEN_ARGS env vars in the workflow address earlier DRY concerns, and the auto-PR mechanism for push events is a nice touch.
One blocking issue: the gecko feature and mozbuild build-dependency are now orphaned — build.rs no longer references mozbuild at all. This should be cleaned up to avoid confusing Firefox integration builds (see inline comment on Cargo.toml).
CI observability gaps: the check-bindings job can silently skip platforms whose upstream VM jobs failed (no artifact = no comparison), and the VM jobs report success even when all tests were skipped due to binding drift. Neither issue causes incorrect results, but both reduce the signal CI provides. Details in inline comments.
The Rust code changes (lint attributes, #[cfg_attr] path selection for per-platform binding modules) are clean and correct. The unsafe blocks are pre-existing and unchanged. The #[expect] vs #[allow] usage for dead_code is deliberate and well-considered across platforms.
There was a problem hiding this comment.
Summary
Good approach — replacing runtime bindgen with pre-generated, CI-verified bindings eliminates the libclang build-time dependency for downstream consumers while keeping bindings provably in sync via the check-bindings job. The Rust code changes are clean and correct.
Previous review items now resolved in the latest commit:
- The
geckofeature andmozbuildbuild-dependency have been properly cleaned up from bothmtu/Cargo.tomlandneqo-transport/Cargo.toml. - The DRY concern about duplicated bindgen arguments is addressed: args are now centralized as
LINUX_BINDGEN_ARGS/BSD_BINDGEN_ARGSenv vars incheck-mtu.ymland passed to the VM action via thebindgen-argsinput.
One should-fix issue (see inline): the auto-PR dedup check in check-bindings uses gh pr list --head "chore-update-mtu-bindings-" which does exact branch-name matching. Since branches are created with a SHA suffix (chore-update-mtu-bindings-<sha>), this will never match, resulting in duplicate PRs on every push to main with binding changes.
Previous review items still present (not re-raised inline):
check-vm/action.yml:92-93:exit 0on binding drift causes the VM job to report success even though all compile/test/clippy checks were skipped. The::warning::annotation provides some signal, but CI status checks won't surface the skip.check-mtu.yml:123:if: always() && !cancelled()oncheck-bindingsmeans it runs even if upstream jobs failed without producing artifacts. The::warning::on line 142 mitigates this partially.
There was a problem hiding this comment.
Summary
Good approach — replacing runtime bindgen with pre-generated, CI-verified bindings eliminates the libclang build-time dependency for downstream consumers while keeping bindings provably in sync. The centralized LINUX_BINDGEN_ARGS / BSD_BINDGEN_ARGS env vars and the auto-PR mechanism for push events are well designed.
Rust code is clean: the #[cfg_attr(..., path = "...")] pattern for per-platform module selection is idiomatic, and the compile-time size/alignment/offset assertions in the generated bindings provide a strong safety net against ABI drift. The gecko / mozbuild cleanup (flagged in a previous review) is now properly resolved across both mtu/Cargo.toml and neqo-transport/Cargo.toml.
Two issues flagged inline (one new, one reinforcing an existing thread):
bsd.rsexpect(dead_code)will error on Solaris — the exclusion must also cover Solaris, not just NetBSD, because all generated items are transitively reachable on both platforms. See inline for details and a suggestion.--headdedup incheck-bindingsnever matches — reinforcing the existing observation with a concrete fix using a fixed branch name.
Previously raised issues I reviewed and agree with (not re-raised inline):
exit 0on binding drift causes misleading green CI for VM jobs — worth addressing but not blocking.if: always() && !cancelled()oncheck-bindingscan mask missing artifacts from failed upstream jobs — the::warning::annotations partially mitigate this.- OpenBSD bindings generated with bindgen 0.72.0 vs. 0.72.1 elsewhere — cosmetic; CI will catch real drift.
There was a problem hiding this comment.
Summary
Clean, well-motivated change that eliminates the bindgen build-time dependency for the mtu crate by checking in pre-generated platform-specific binding files. This removes the libclang requirement for downstream consumers — a significant ergonomic improvement, especially for the quinn integration.
Approach: The include!(env!("BINDINGS")) pattern is replaced with #[cfg_attr(target_os = "...", path = "bindings/....rs")] module path selection. Bindgen-generated .rs files are committed for all 6 platforms, with CI infrastructure (check-mtu.yml + check-vm) to detect and auto-update drifted bindings. The compile-time size/alignment assertions in each binding file serve as effective safety nets against ABI mismatches.
What looks good:
- The conditional
expect(dead_code)onbsd.rs(excluding NetBSD/Solaris whereRTA_IFPis used) is well-reasoned. - Version bump to
0.4.0correctly signals the semver-breaking removal of thegeckofeature. - The
check-bindingsCI job with its auto-PR-on-push / fail-on-PR design is a sound workflow for keeping bindings in sync. - Header files (
bsd.h,linux.h) are retained as documentation of bindgen inputs — good for reproducibility.
Key items from prior reviews that appear addressed in the latest revision:
gecko/mozbuilddead code inCargo.toml— removed.allowvsexpectinconsistency — bothbsd.rsandlinux.rsnow use#[expect].- PR dedup logic — now uses a fixed branch name (
chore/update-mtu-bindings) with force-push.
Remaining open concerns from prior reviews (not re-raised inline):
openbsd.rswas generated with bindgen 0.72.0 while all others use 0.72.1 — worth aligning.- The
exit 0on binding drift incheck-vmmeans the VM job reports success even when compile/test/clippy are skipped. Thecheck-bindingsjob catches the drift, but the green VM job status can be misleading. Consider setting a job output or using a distinct exit code.
New observations are in the inline comments.
Benchmark resultsSignificant performance differences relative to 53527a2. streams/walltime/1000-streams/each-1-bytes: 💚 Performance has improved by -1.5415%. time: [12.267 ms 12.293 ms 12.322 ms]
change: [-1.7926% -1.5415% -1.2911] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severeAll resultstransfer/1-conn/1-100mb-resp (aka. Download): Change within noise threshold. time: [203.69 ms 204.01 ms 204.40 ms]
thrpt: [489.23 MiB/s 490.17 MiB/s 490.95 MiB/s]
change:
time: [-0.6173% -0.3254% -0.0539] (p = 0.03 < 0.05)
thrpt: [+0.0539% +0.3264% +0.6212]
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severetransfer/1-conn/10_000-parallel-1b-resp (aka. RPS): Change within noise threshold. time: [288.73 ms 290.56 ms 292.41 ms]
thrpt: [34.198 Kelem/s 34.416 Kelem/s 34.634 Kelem/s]
change:
time: [+0.6852% +1.5116% +2.4324] (p = 0.00 < 0.05)
thrpt: [-2.3747% -1.4891% -0.6805]
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mildtransfer/1-conn/1-1b-resp (aka. HPS): No change in performance detected. time: [38.800 ms 38.980 ms 39.178 ms]
thrpt: [25.525 B/s 25.654 B/s 25.773 B/s]
change:
time: [-0.4034% +0.1929% +0.8372] (p = 0.56 > 0.05)
thrpt: [-0.8303% -0.1925% +0.4050]
No change in performance detected.
Found 11 outliers among 100 measurements (11.00%)
2 (2.00%) high mild
9 (9.00%) high severetransfer/1-conn/1-100mb-req (aka. Upload): No change in performance detected. time: [204.82 ms 205.15 ms 205.50 ms]
thrpt: [486.61 MiB/s 487.44 MiB/s 488.24 MiB/s]
change:
time: [-0.2780% -0.0054% +0.2553] (p = 0.97 > 0.05)
thrpt: [-0.2546% +0.0054% +0.2787]
No change in performance detected.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) low mild
1 (1.00%) high mild
1 (1.00%) high severestreams/walltime/1-streams/each-1000-bytes: Change within noise threshold. time: [586.19 µs 588.79 µs 591.69 µs]
change: [-1.5296% -0.8228% -0.1029] (p = 0.02 < 0.05)
Change within noise threshold.
Found 14 outliers among 100 measurements (14.00%)
14 (14.00%) high severestreams/walltime/1000-streams/each-1-bytes: 💚 Performance has improved by -1.5415%. time: [12.267 ms 12.293 ms 12.322 ms]
change: [-1.7926% -1.5415% -1.2911] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severestreams/walltime/1000-streams/each-1000-bytes: Change within noise threshold. time: [44.772 ms 44.818 ms 44.864 ms]
change: [-1.0873% -0.9301% -0.7734] (p = 0.00 < 0.05)
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/walltime/pacing-false/varying-seeds: No change in performance detected. time: [81.981 ms 82.034 ms 82.089 ms]
change: [-0.0764% +0.0354% +0.1543] (p = 0.56 > 0.05)
No change in performance detected.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/walltime/pacing-true/varying-seeds: Change within noise threshold. time: [81.604 ms 81.710 ms 81.868 ms]
change: [+0.1745% +0.3366% +0.5277] (p = 0.00 < 0.05)
Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) low mild
1 (1.00%) high mild
1 (1.00%) high severetransfer/walltime/pacing-false/same-seed: Change within noise threshold. time: [81.072 ms 81.254 ms 81.429 ms]
change: [-2.0628% -1.8193% -1.5531] (p = 0.00 < 0.05)
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/walltime/pacing-true/same-seed: Change within noise threshold. time: [81.854 ms 81.950 ms 82.060 ms]
change: [-1.0100% -0.8811% -0.7328] (p = 0.00 < 0.05)
Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high severeDownload data for |
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
|
Client/server transfer resultsPerformance differences relative to 53527a2. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Table above only shows statistically significant changes. See all results below. All resultsTransfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
Fixes quinn-rs/quinn#2465 (comment)