feat(tokio): reject an unsupported feature build with one honest error - #3152
feat(tokio): reject an unsupported feature build with one honest error#3152kixelated wants to merge 3 commits into
Conversation
`cargo doc -p moq-tokio` with default features fails on the crate-level `[`mdns`]` link: the module is gated behind the non-default `mdns` feature, so rustdoc has no item to resolve. `RUSTDOCFLAGS="-D warnings"` makes it a hard error, and `just check`'s doc pass is diff-scoped, so it fires on any branch whose diff selects moq-tokio. The nightly `features` recipe never saw it because it documents the workspace with `--all-features`, which compiles the module in. Name the module and its feature in prose rather than linking it, so the sentence reads the same in every build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cargo check -p moq-tokio --no-default-features` failed with 8 errors, none of which named the cause: `QuicBackend` becomes an empty enum, `RequestKind` an empty match, and `worker`/`server` reference `util`/`DEFAULT_BIND` that their own gates removed. The count grows as the crate does; it was 5 when this was filed. Rather than make a backend-less build work, state that it is unsupported. `worker`, `steer`, and `server` all assume a backend exists, so a `compile_error!` matches reality and names the features to enable. Auditing the feature space turned up a second unsupported shape the issue did not cover. `quinn` and `noq` take their crypto provider from the `aws-lc-rs`/`ring` features, since their rustls dependency is built with default features off: - `--features noq` alone fails with 3 unrelated E0599s about missing `EndpointConfig::default` and `ServerConfig::with_crypto`. - `--features quinn` alone *compiles*, then has no initial cipher at runtime, so the failure lands on a connection attempt instead of the build. That one is worth catching precisely because it is silent. `quiche` needs neither, bringing its own through boringssl, and is the only single-feature build that is actually supported. Workspace builds cannot reach either shape: cargo unifies features per crate, so a member depending on moq-tokio with a backend enables one for everyone. That is why nightly's `--workspace --no-default-features` leg never caught this, and it still passes unchanged. Only `-p moq-tokio` or an external consumer with `default-features = false` selects the broken shapes, so `just rs _unsupported-features` asserts each one still fails on its own message rather than on whatever breaks first inside the modules. `compile_error!` does not halt compilation, so the downstream errors still follow it; the change is that the first one names the cause and the fix. Closes #2979 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a659cd6e22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[cfg(all( | ||
| any(feature = "quinn", feature = "noq"), | ||
| not(any(feature = "aws-lc-rs", feature = "ring")) | ||
| ))] |
There was a problem hiding this comment.
When an external consumer enables only moq-tokio/quinn and installs a Rustls provider itself before constructing the client or server, this guard rejects the build even though that configuration previously worked. crypto::provider() explicitly checks CryptoProvider::get_default() first, so requiring the provider to come specifically from a moq-tokio Cargo feature removes the supported application-installed-provider path. The check should not infer runtime provider availability solely from this crate's feature flags.
Useful? React with 👍 / 👎.
…rovider
The crypto-provider guard covered `quinn` as well as `noq`, which removes a
supported configuration. `crypto::provider()` resolves the provider at
runtime and takes `CryptoProvider::get_default()` first, so an application
that calls `install_default()` works with `quinn` and no provider feature.
Its panic names that path as the equal alternative to the features:
"no CryptoProvider available; install_default() or enable either the
aws-lc-rs or ring feature"
Refusing the build made it unreachable, turning a working setup into a
compile error for any consumer doing this.
`noq` keeps the guard, because it is not a runtime question there: without a
provider feature `web-transport-noq` is built without its own, and the
constructors this crate calls stop existing, so the build fails regardless of
what the application installs later.
Found by Codex review (P2) and verified against `crypto.rs`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 530014e118
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| //! A build needs a QUIC backend (`quinn`, `quiche`, or `noq`). `noq` additionally | ||
| //! needs a crypto provider (`aws-lc-rs` or `ring`) to compile at all. `quinn` takes | ||
| //! one from either feature or from a provider the application installed with | ||
| //! `CryptoProvider::install_default()`, and `quiche` brings its own. The `default` |
There was a problem hiding this comment.
When a consumer follows this guidance and enables only quiche, server startup still calls crypto::provider() from QuicheServer::new while computing certificate fingerprints; with neither provider feature nor an application-installed Rustls provider, that function panics. Quiche's BoringSSL provider does not populate Rustls's process-wide provider, so document/install a Rustls provider for this configuration or avoid using Rustls for the fingerprint.
AGENTS.md reference: AGENTS.md:L107-L109
Useful? React with 👍 / 👎.
|
Superseded by #3150, which took the other option from #2979 and landed while this was open. #3150 made the backend-less build compile (gating So this PR is not merely redundant, it is incompatible. Its One finding does survive, filed as #3160: Closing rather than rebasing: the useful remainder is one guard, and it belongs on top of #3150's structure rather than inside a change built on the opposite premise. (written by Claude Opus 5) |
1 participant
Closes #2979.
The reported bug
cargo check -p moq-tokio --no-default-featuresfails with 8 errors, none naming the cause:QuicBackendbecomes an empty enum,RequestKindan empty match, andworker/serverreference items their own gates removed. The count was 5 when the issue was filed and grows as the crate does, which is itself the argument against making this configuration work.The issue offered two options; this takes the first.
worker,steer, andserverall assume a backend exists, so acompile_error!matches reality and names the features to enable.A second unsupported shape, not in the issue
Auditing the feature space rather than just the reported case turned up another one.
quinnandnoqtake their crypto provider fromaws-lc-rs/ring, since their rustls dependency is built with default features off (the manifest already says so: "a backend without aws-lc-rs/ring has no initial cipher").tcpnoqE0599s aboutEndpointConfig::default/ServerConfig::with_cryptoquinnquichequinn,aws-lc-rs,noq,aws-lc-rsnoqis the only backend that gets this guard, and the reason is that it is not a runtime question there: without a provider featureweb-transport-noqis built without its own and the constructors this crate calls stop existing, so the build fails whatever the application does later.An earlier revision of this PR also guarded
quinn, and that was wrong.crypto::provider()resolves the provider at runtime and checksCryptoProvider::get_default()first, so an application that callsinstall_default()works withquinnand no provider feature. The function's own panic names that as the equal alternative:Refusing the build made a supported configuration unreachable. Caught by the Codex review on this PR (P2), verified against
crypto.rs, and reverted in 530014e;--features quinncompiles clean again.Why CI never saw any of this, and still passes
Cargo unifies features per crate, so in a workspace build a member that depends on moq-tokio with a backend enables one for everyone. Nightly's
cargo check --locked --workspace --no-default-featurestherefore never compiles moq-tokio backend-less, which is why it stayed green over a broken configuration. I ran that exact command with these guards in place and it still passes unchanged.Only
-p moq-tokioor an external consumer withdefault-features = falseselects the broken shapes, sojust rs _unsupported-features(added to the nightlyfeaturesrecipe) asserts each still fails on its own message rather than on whatever breaks first inside the modules. Verified it catches a regression: neutering either guard makes the recipe fail withmoq-tokio --features quinn should fail on: require a crypto provider.This is also why adding
--all-features/--no-default-featurestojust checkwould not have fixed the issue: two extra full workspace compiles per PR, and feature unification means neither would compile moq-tokio backend-less anyway.Note:
compile_error!does not halt compilation, so the downstream errors still follow it. The change is that the first error names the cause and the fix.Drive-by: a doc link that breaks any moq-tokio PR
Separate first commit, kept apart so it can be reverted on its own.
cargo doc -p moq-tokiowith default features already fails ondev:mdnsis behind a non-default feature, so rustdoc has no item to resolve, andRUSTDOCFLAGS="-D warnings"makes it a hard error.just check's doc pass is diff-scoped, so this fires on any branch whose diff selects moq-tokio, including this one. Nightly missed it because it documents with--all-features, which compiles the module in.Confirmed pre-existing by stashing this branch's changes and reproducing on clean
dev. Same class as thetarget/doc/moqcollision in #3113: a doc-pass failure that only fires on certain package selections.Base
Targets
devbecausemoq-tokioonly exists there. No published API changes shape: the guards reject builds that already did not work, and the doc fix is prose.🤖 Generated with Claude Code
(Written by Claude Opus 5)