fix: repair dev, which the last main merge left uncompilable - #3399
Merged
Conversation
…n merge `Merge main into dev` (6947217) brought main's new `broadcast_moq_transport_20_current_group_join` into `rs/moq-tokio/tests/broadcast.rs` verbatim. On main that test lives in `rs/moq-native/tests/broadcast.rs` and predates both #3225 (announcements are prefix routes) and the moq-native to moq-tokio split, so it does not compile on dev: `Origin::random()`, `moq_native::{Server,Client}Config`, `create_broadcast(path, Route)`, and `Update::broadcast` are all gone. The merge also glued `next_announce`'s doc comment onto the new test. Ported onto dev's API, matching the neighbouring tests: `origin::spawn` plus a separate `announce`, `moq_tokio::{listen,connect}::Config`, `connect_once`, and resolving the announce through `request_broadcast`. The assertions the test exists for are unchanged. 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. |
`cargo shear` fails on dev: `rs/moq-cli` declares `humantime` and no source file uses it, so `just check` fails for every branch based on dev whose diff reaches moq-cli. The usage-rs migration (#3030) replaced clap's `value_parser = humantime::parse_duration` with usage-rs' own duration parsing and removed the dependency in the same commit. main still had the clap spelling, so `Merge main into dev` (6947217) took main's side of the manifest and put the dependency back without any of the uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cargo sort --workspace --check --no-format` fails on the root manifest: `moq-sock` sits above `moq-rtc` and `moq-rtmp` in `[workspace.dependencies]` instead of after them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
devdoes not passjust check, so every branch based on it fails CI (including #3396). Three independent pieces of fallout fromMerge main into dev(6947217), where a conflict-free textual merge was not a semantic one.1.
rs/moq-tokio/tests/broadcast.rsdoes not compileThe merge brought main's new
broadcast_moq_transport_20_current_group_jointest in verbatim. On main that test lives inrs/moq-native/tests/broadcast.rsand predates both #3225 (announcements are prefix routes) and the moq-native to moq-tokio split, so it references four things that no longer exist on dev:Origin::random().produce()→moq_tokio::origin::spawn(Hop::random())create_broadcast(path, broadcast::Route::new().with_announce(true))→create_broadcast(path)plus a separateannounce(path, ..)moq_native::{ServerConfig,ClientConfig}→moq_tokio::{listen,connect}::Config(withtls.disable_verify→tls.insecure, andinit()→init(..)+listen())announce::Update::broadcast→Update { prefix, route, active }, resolved throughrequest_broadcastThe merge also glued
next_announce's doc comment onto the new test's, leavingnext_announceundocumented and the test's own doc opening on an unrelated sentence.Ported onto dev's API, matching the neighbouring tests. The assertions the test exists for (group 0 stitched from the fill fetch stream plus the subgroup stream, in order, ending exactly once) are unchanged.
2.
cargo shear: an unusedhumantimers/moq-clideclareshumantimeand no source file uses it. The usage-rs migration (#3030) replaced clap'svalue_parser = humantime::parse_durationwith usage-rs' own duration parsing and dropped the dependency in the same commit; main still had the clap spelling, so the merge took main's side of the manifest and put the dependency back without any of the uses.3.
cargo sort:moq-sockout of ordermoq-socksits abovemoq-rtcandmoq-rtmpin the root[workspace.dependencies].cargo sort --workspace --check --no-formatfails on it.Each of the three is independently fatal, and the second and third only surface once the first stops failing the build, which is why they came out one CI round at a time.
Test plan
cargo check -p moq-tokio --testsclean.cargo test -p moq-tokio --test broadcast broadcast_moq_transport_20_current_group_joinpasses.cargo check -p moq-cliclean without the dependency.cargo shearreports no issues;cargo sort --workspace --check --no-formatclean.cargo fmt -p moq-tokioclean.Test,WASM,Swift, andOBSalready pass on this branch;Checkis what these three fix.Note
#3396 carries its own copy of fix 1 (
38be5113b), so whichever lands first, the other needs a rebase. Fixes 2 and 3 are only here, and #3396 cannot go green without them.🤖 Generated with Claude Code