Restore the BUILD_SAMPLES option; state BUILD_MOQTEST; fix the XLOG smoke test - #376
Conversation
Upstream D116974940 split moqtest out of BUILD_SAMPLES into a new BUILD_MOQTEST option. In standalone/CMakeLists.txt it replaced the BUILD_SAMPLES declaration instead of adding beside it, on the premise that standalone never declared BUILD_SAMPLES -- it did, at the same line. Standalone saves BUILD_SAMPLES, forces it off while the fetched deps configure, then restores it. With no declaration the saved value is empty, so the restore sets it to "" rather than ON and samples/ and media_server/ stop building. The published artifacts lose moqdateserver, moqtextclient and the media server binaries; moqx relay_chain and relay_hops_cycle fail against the 91f2387 tarball. moxygen's own CI missed it because standalone's test directory is gated on the moqdateserver and moqtextclient targets existing, so the XLOG category test silently stopped running instead of failing.
Every build site now states which binaries it needs instead of inheriting whatever BUILD_SAMPLES and BUILD_MOQTEST default to. Test builds (ci-pr, ci-main) keep BUILD_SAMPLES=ON: the XLOG category smoke test drives moqdateserver and moqtextclient. Artifact builds (ci-main publish, version-release) set BUILD_SAMPLES=OFF and BUILD_MOQTEST=ON, so the tarball carries moqtest_client, moqtest_server and moqperf_test_client and drops the sample and media server binaries. That makes the media_server strip in collect-artifacts-standalone.sh dead, so it goes. moqx resolves moqdateserver and moqtextclient from the tarball today, so its next moxygen sync fails until its test scripts move to the moqtest binaries. Verified by configuring standalone with each flag set: the artifact set builds the three moqtest binaries and no samples while keeping samples/util, and the test set still registers xlog_category_roots.
The XLOG category smoke test was the last thing in this tree needing the sample binaries. It now drives moqtest_server and moqtest_client, so BUILD_SAMPLES has no consumer and standalone stops declaring, saving and restoring it. Samples are never built here, matching upstream, whose standalone build has never declared the option either. The test also got faster. Every marker it looks for is emitted during connection setup, and the client runs until killed, so the old shape spent all its time in fixed waits: four sessions of sleep 1 plus timeout 3 accounted for 17.2s of a 17.44s run. A readiness poll on the server's UDP socket replaces the sleep and a 0.5s bound replaces the timeout. Measured on one machine against the same ASAN build, with identical assertions and skips: 18.42s before, 6.98s after. Skips with 77 on bash < 4 rather than reporting a pass. The marker maps and assert_scoping's nameref need bash 4, macOS ships 3.2, and without set -e the failures were silent, so the test has reported Passed 0.01s on macOS since it landed without running anything (#378). xlog_category_pico is unregistered. Its only driver pair is built under BUILD_SAMPLES and moqtest has no picoquic transport, so there is nothing to move it to yet (#377).
afrind
left a comment
There was a problem hiding this comment.
There's a mix of things here. I think we want to do one PR that adds BUILD_MOQTEST and removes BUILD_TESTS, and leave BUILD_SAMPLES alone for now.
@afrind reviewed 9 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on gmarzot).
.github/workflows/omoq-version-release.yml line 246 at r1 (raw file):
-DCMAKE_BUILD_TYPE=Release \ -DCMAKE_POSITION_INDEPENDENT_CODE=ON \ -DBUILD_TESTS=ON \
I believe we no longer need BUILD_TESTS either. The lone test util library consumed by moqx is now always built.
openmoq/scripts/collect-artifacts-standalone.sh line 57 at r1 (raw file):
# ── Step 0: Drop binaries that are not part of the relay artifact ──────────── # Built under BUILD_SAMPLES beside binaries we do ship, so excluded here. rm -f "$INSTALL_PREFIX"/bin/moq_media_server* "$INSTALL_PREFIX"/bin/moq_mp4_receiver*
I think moq_media_server is still built under BUILD_SAMPLES, we can't turn off BUILD_SAMPLES until we convert all moqx tests to use moqtest instead of date/text, which is forthcoming.
standalone/CMakeLists.txt line 282 at r1 (raw file):
# Save original BUILD_TESTS/BUILD_SAMPLES/BUILD_TESTING values, then disable for fetched dependencies set(_MOXYGEN_BUILD_TESTS ${BUILD_TESTS}) set(_MOXYGEN_BUILD_SAMPLES ${BUILD_SAMPLES})
This is premature
standalone/test/CMakeLists.txt line 3 at r1 (raw file):
# Regression guard for the per-layer XLOG category rooting in ../CMakeLists.txt: # drives a short mvfst session and asserts each layer's --logging selector scopes # to that layer. Skips (77) on bash < 4.
what is this?
standalone/test/local.cmake line 5 at r1 (raw file):
# xlog_category_pico is unregistered: its only drivers, pico_evb_relay_server and # pico_evb_text_client, build under BUILD_SAMPLES, which nothing sets. Restore it # once moqtest speaks picoquic.
? why would moqtest speak picoquic?
standalone/test/xlog_category_smoke.sh line 18 at r1 (raw file):
set -uo pipefail # The marker maps and assert_scoping's nameref need bash 4; on 3.2 both fail
You can ask claude to make a bash3.2 compat version
Reverts the BUILD_SAMPLES removal. moqx still resolves moqdateserver and moqtextclient from the tarball, and the moqtest rewrite that replaces them is in progress upstream, so the option stays until that lands. The media_server strip and xlog_category_pico come back with it. What remains is the sync fix plus two changes to the smoke test. BUILD_MOQTEST is now stated at all four configure sites rather than inherited from the option default, so a sync dropping a declaration cannot silently change what a build produces. The smoke test runs on bash 3.2. The marker map and assert_scoping's nameref needed bash 4; macOS ships 3.2, where both fail, and without set -e the failures were silent, so the test reported Passed 0.01 sec on macOS since it landed without running anything. Parallel arrays and a lookup function replace them. It is also faster. Every marker is emitted during connection setup and the client runs until killed, so the old shape spent all its time in fixed waits: four sessions of sleep 1 plus timeout 3 accounted for 17.2s of a 17.44s run. A readiness poll on the server's UDP socket replaces the sleep, and the client is backgrounded and killed rather than run under timeout(1), which macOS lacks. 18.42s to 4.0-4.9s across three runs against the same ASAN build, with the same three PASS and three SKIP lines. Verified under bash 3.2.57 on macOS with stub binaries, exercising the parallel arrays, the nameref-free assert_scoping and the lsof readiness path where ss is absent. BUILD_TESTS stays. The googletest acquisition is inside if(BUILD_TESTS) in standalone/CMakeLists.txt, and moqx does find_package(GTest REQUIRED CONFIG) against the installed prefix. Nothing moxygen exports carries GTest, so that coupling is incidental and belongs in moqx, but dropping the flag before moqx supplies its own googletest breaks its test configure.
Samples still ship, stripped; only their .debug sidecars are dropped, which keeps the -dbg tarball under GitHub's 2 GB asset limit with BUILD_SAMPLES on. Good enough to unblock the moxygen -> moqx sync; the proper fix comes with the moqtest rewrite.
afrind
left a comment
There was a problem hiding this comment.
@afrind reviewed 8 files and all commit messages, made 1 comment, and resolved 5 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on gmarzot).
openmoq/scripts/collect-artifacts-standalone.sh line 89 at r2 (raw file):
# whole folly/proxygen/mvfst stack, so a sidecar apiece adds ~130 MB of duplicate # DWARF and pushes the -dbg tarball past GitHub's 2 GB asset limit. NO_SIDECAR_BINS='moqchatclient moqdateserver moqflvreceiverclient moqflvstreamerclient moqtextclient'
moqx depends on moqtextclient
afrind
left a comment
There was a problem hiding this comment.
@afrind made 1 comment and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on gmarzot).
openmoq/scripts/collect-artifacts-standalone.sh line 89 at r2 (raw file):
Previously, afrind wrote…
moqx depends on moqtextclient
aha, this is just for dbg sections
The artifact script deleted moq_media_server and moq_mp4_receiver from
bin/ after install. moxygen installs every executable into
moxygen-exports, so the imported targets stayed behind pointing at files
that were gone, and any consumer calling find_package(moxygen) failed:
The imported target "moxygen::moq_media_server" references the file
".../bin/moq_media_server"
but this file does not exist.
That was masked while 91f2387 had BUILD_SAMPLES off, since the media
server was never built there. Restoring the option in #376 brought the
targets back and the moqx sync went red.
Deleting files after install cannot work while those files are exported.
BUILD_MEDIA_SERVER gates the directory instead, so nothing is built,
installed or exported, and the post-install deletion goes away. It
follows BUILD_MOQTEST: declared in the root CMakeLists and defaulting
on, so getdeps and full-repo builds are unchanged, while the standalone
build does not process that file and leaves the directory out. Same
mechanism BUILD_SAMPLES already relies on.
Nothing outside moxygen/media_server links its targets.
Verified by configuring standalone: no media_server, moq_media_server or
moq_mp4_receiver targets exist, while moqdateserver, moqtextclient and
the three moqtest binaries still build.
Restores the
BUILD_SAMPLESoption that sync #375 dropped, statesBUILD_MOQTESTexplicitly at every configure site, and fixes the XLOG category smoke test.Reworked from r1 per review: the
BUILD_SAMPLESremoval is reverted and left for after the moqtest rewrite lands.BUILD_TESTSstays too — see the last section.The sync fix
Upstream
D116974940insertedoption(BUILD_MOQTEST ...)into its ownstandalone/CMakeLists.txt. Upstream's copy has never declaredBUILD_SAMPLES, so that is correct there; ours declares it on that exact line, and the merge in065e3784replaced our line instead of keeping both.Standalone saves
BUILD_SAMPLES, forces it off while the fetched dependencies configure, then restores it. With no declaration the saved value was empty, the restore set"",samples/stopped building, and the tarball for91f2387lostmoqdateserverandmoqtextclient. openmoq/moqx#620 failedrelay_chainandrelay_hops_cyclewithnot found or not executable: .../bin/moqdateserver.BUILD_MOQTEST=ONis now passed at all four configure sites rather than inherited from the option default, so a sync dropping a declaration cannot silently change what a build produces.The smoke test runs on bash 3.2
xlog_category_rootshas reportedPassed 0.01 secon macOS since it landed in #312, without running anything. The marker map useddeclare -Aandassert_scopingused alocal -nnameref; both need bash 4, macOS ships 3.2, and withoutset -ethe failures were silent —assert_scopingnever ran,failstayed 0, exit 0.Parallel arrays and a
marker_forlookup replace them. Verified under bash 3.2.57 on a macOS box with stub binaries, exercising the arrays, the nameref-freeassert_scoping, and thelsofreadiness path wheressis absent.This means macOS will genuinely run this test for the first time. Worth watching that job.
The smoke test is faster
Every marker the test looks for is emitted during connection setup, and the client runs until killed, so the old shape spent essentially all its time waiting. Four sessions of
sleep 1plustimeout 3account for 17.2s of a 17.44s CI run.A readiness poll on the server's UDP socket replaces the sleep. The client is backgrounded and killed rather than run under
timeout(1), which macOS lacks.Same ASAN build, same three PASS and three SKIP lines. In CI terms 17.4s to roughly 4s.
Worth being clear this is not a CI speedup: the linux job is 21m28s, of which 18m48s is the build and 72s is all 1555 tests. What it fixes is an outlier — this one test took 24% of test time and ran 11x longer than the next slowest.
On dropping BUILD_TESTS
Agreed in principle, but it breaks moqx today and the fix belongs in moqx.
moqtest_utilsis indeed always built now. It is not the only thing moqx takes from the tarball though — the googletest acquisition is insideif(BUILD_TESTS)atstandalone/CMakeLists.txt:245, and moqx'stest/CMakeLists.txt:1isfind_package(GTest REQUIRED CONFIG). WithBUILD_TESTS=OFFthe tarball ships nolib/cmake/GTest/GTestConfig.cmakeand that is a hard failure on any box without a CONFIG-mode system googletest. The comment at that gate anticipates it: CONFIG mode is used precisely because a module-mode-only system googletest will not satisfy moqx.Nothing moxygen exports carries GTest — the only consumers are
moxygen/test/andmoxygen/mlog/test/, neither installed, andmoqtest_utilslinks onlymoxygen_moq. So there is no ABI reason for the two to share a googletest and moqx should bring its own. That is a moqx change, andBUILD_TESTS=OFFfollows it.Same shape as the samples ordering: two moqx changes gate a genuinely minimal artifact, one of them already in progress.
This change is