fix(dev): make the documented checks pass on a fresh clone - #22
Open
RickyYii wants to merge 1 commit into
Open
Conversation
CONTRIBUTING tells contributors to run `cargo test`, `cargo fmt --check`
and `cargo clippy -- -D warnings`. None of the three passed on a clean
checkout.
`cargo test` never reached a test on Linux or macOS. The Tauri build
script aborts when a bundle resource path is missing, and both
`engines/linux/brush/` and `engines/macos/arm64/` are gitignored, so they
do not exist until `setup:engines` downloads binaries into them. CI never
saw this because it installs the engines first. Track a README in each
directory, following the convention already used for engines/ffmpeg,
engines/colmap and engines/brush, so the directories survive a clone
while the binaries stay out of Git.
Two tests then failed for reasons unrelated to the code under test:
- `linux_root_is_flat_and_discovery_can_fall_back_to_path` asserted
`discovered.ffmpeg.is_file()`, which requires FFmpeg on PATH. Assert the
resolver contract instead - override wins, then PATH, otherwise the
managed path is kept - so the test covers the same behaviour on a host
with or without FFmpeg.
- `cancellation_terminates_descendant_processes` asserted that
`/proc/{pid}` no longer exists. A killed descendant is reparented to
PID 1, and PID 1 does not reap in every container, so the process
correctly stops running but stays visible as a zombie and the assertion
fails. Assert termination instead: on Linux the entry is gone or the
state is `Z`, on other Unix targets signal 0 reports no such process.
The old assertion was also vacuous on macOS, where `/proc` never
exists; the new one is not. The fixed 50ms sleep becomes a bounded poll
so the check does not race the reaper.
`cargo clippy -- -D warnings` failed on current stable Rust at
`reserve_gaussian_video_export` (`clippy::nonminimal_bool`). CI installs
Rust with an unpinned `rustup update stable`, so this turns the Ubuntu and
macOS workflows red as runners pick the toolchain up.
Verified on Ubuntu 24.04 x86_64: cargo test 68 passed (was 66 passed /
2 failed), cargo fmt --check clean, cargo clippy --all-targets -D
warnings clean, npm test 48 passed, tsc clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012vBtVi2gXiSpUVoBeFuvu2
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.
CONTRIBUTING asks contributors to run
cargo test,cargo fmt -- --checkandcargo clippy --all-targets -- -D warnings, and none of the three passes on a clean checkout today.cargo testnever reaches a test on Linux or macOS: the Tauri build script aborts when a declared bundle resource path is missing, andsrc-tauri/tauri.linux.conf.jsonandsrc-tauri/tauri.macos.conf.jsondeclareengines/linux/brush/andengines/macos/arm64/, which are gitignored and therefore absent untilsetup:enginesdownloads binaries into them, so a fresh clone fails withresource path `../engines/linux/brush` doesn't existbefore compiling anything; CI never sees this because it installs the engines first, so it only bites new contributors. This PR tracks a README in each of those two directories, following the convention already used forengines/ffmpeg,engines/colmapandengines/brush, so the directories survive a clone while the binaries stay out of Git. Once the build runs, two tests fail for reasons unrelated to the code under test:linux_root_is_flat_and_discovery_can_fall_back_to_pathassertsdiscovered.ffmpeg.is_file(), which requires FFmpeg on PATH and is not stated as a prerequisite anywhere, so it now asserts the resolver contract instead — an explicitOOOSPLAT_FFMPEGoverride wins, then PATH, and otherwise the managed path is kept so engine health can report the exact file it expected — covering the same behaviour on a host with or without FFmpeg; andcancellation_terminates_descendant_processesasserts that/proc/{pid}no longer exists, but a killed descendant is reparented to PID 1 and PID 1 does not reap in every container, so the process correctly stops running yet lingers as a zombie with its/procentry intact and the assertion fails, which is what happens in a plain container today. It now asserts termination directly: on Linux the entry is gone or the state isZ, and on other Unix targets signal 0 reports no such process. Worth flagging that the old assertion was also vacuous on macOS, where/procnever exists andexists()is always false, so that test asserted nothing there; the new one does assert, and while the reasoning holds — the orphanedsleepis reparented to launchd and reaped promptly, well inside the bounded three-second poll that replaces the previous fixed 50ms sleep — I have no macOS machine to confirm it on, so please do watch the macOS workflow on this PR. Finally,cargo clippy --all-targets -- -D warningsfails on current stable Rust atreserve_gaussian_video_exportinsrc-tauri/src/commands/mod.rswithclippy::nonminimal_bool; the workflows install Rust with an unpinnedrustup update stable, so this turns the Ubuntu and macOS workflows red as runners pick the toolchain up, and it is fixed here because otherwise this PR could not show a green run either. Verified on Ubuntu 24.04 x86_64 with Rust 1.94.1:cargo test68 passed / 0 failed (66 passed / 2 failed before),cargo fmt -- --checkclean,cargo clippy --all-targets -- -D warningsclean,npm test48 passed,tsc -bclean. The fresh-clone claim was checked end to end by cloning the branch into an empty directory and runningcargo testwith nomkdirand nosetup:engines: 68 passed. No pipeline stage was exercised — this change touches only test assertions, two tracked placeholder READMEs, a.gitignorenegation and one boolean expression, and does not touch FFmpeg, COLMAP, Brush or PLY output.