Skip to content

fix(dev): make the documented checks pass on a fresh clone - #22

Open
RickyYii wants to merge 1 commit into
ooolabdev:mainfrom
RickyYii:claude/code-review-feedback-hzid1l
Open

fix(dev): make the documented checks pass on a fresh clone#22
RickyYii wants to merge 1 commit into
ooolabdev:mainfrom
RickyYii:claude/code-review-feedback-hzid1l

Conversation

@RickyYii

Copy link
Copy Markdown

CONTRIBUTING asks contributors to run cargo test, cargo fmt -- --check and cargo clippy --all-targets -- -D warnings, and none of the three passes on a clean checkout today. cargo test never reaches a test on Linux or macOS: the Tauri build script aborts when a declared bundle resource path is missing, and src-tauri/tauri.linux.conf.json and src-tauri/tauri.macos.conf.json declare engines/linux/brush/ and engines/macos/arm64/, which are gitignored and therefore absent until setup:engines downloads binaries into them, so a fresh clone fails with resource path `../engines/linux/brush` doesn't exist before 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 for engines/ffmpeg, engines/colmap and engines/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_path asserts discovered.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 explicit OOOSPLAT_FFMPEG override 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; and cancellation_terminates_descendant_processes asserts 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 /proc entry 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 is Z, and on other Unix targets signal 0 reports no such process. Worth flagging that the old assertion was also vacuous on macOS, where /proc never exists and exists() is always false, so that test asserted nothing there; the new one does assert, and while the reasoning holds — the orphaned sleep is 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 warnings fails on current stable Rust at reserve_gaussian_video_export in src-tauri/src/commands/mod.rs with clippy::nonminimal_bool; the workflows install Rust with an unpinned rustup 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 test 68 passed / 0 failed (66 passed / 2 failed before), cargo fmt -- --check clean, cargo clippy --all-targets -- -D warnings clean, npm test 48 passed, tsc -b clean. The fresh-clone claim was checked end to end by cloning the branch into an empty directory and running cargo test with no mkdir and no setup:engines: 68 passed. No pipeline stage was exercised — this change touches only test assertions, two tracked placeholder READMEs, a .gitignore negation and one boolean expression, and does not touch FFmpeg, COLMAP, Brush or PLY output.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant