Skip to content

Should test:unit:rust / dist:python / dist:c chain commands with && (or accumulate $status) instead of ;? #550

@G4614

Description

@G4614

What I noticed

While running the test matrix on a fresh clone, I noticed make test:unit:rust reported exit 0 even though cargo had clearly printed error: test failed and 29 lib tests under -p boxlite had failed. Tracing it back, it looks like a POSIX shell behaviour in a few recipes — but I wasn't sure if it was intentional, so figured I'd ask before sending a PR.

The pattern

Three recipes in make/test.mk and make/dist.mk chain multiple commands inside one shell with ; instead of &&:

make/test.mk:175-183test:unit:rust

@if command -v cargo-nextest >/dev/null 2>&1; then \
    cargo nextest run -p boxlite --no-default-features --lib $(NEXTEST_FILTER); \
    cargo nextest run -p boxlite-shared --lib $(NEXTEST_FILTER); \
else \
    cargo test -p boxlite ... ; \
    cargo test -p boxlite-shared ... ; \
fi

POSIX shell evaluates if A; then B; C; fi to the rc of C. If -p boxlite lib tests fail but -p boxlite-shared passes, the recipe exits 0 — and CI sees green.

Reproducer: in an environment where some -p boxlite tests fail (e.g. AppArmor unprivileged_userns blocks bwrap on Ubuntu 24.04 default), a captured log shows test result: FAILED. 674 passed; 29 failed and error: test failed, to rerun pass -p boxlite --lib, followed by a final line of exit=0.

make/dist.mk:5-15dist:python Linux branch

source .venv/bin/activate; \
bash $(SCRIPT_DIR)/build/build-guest.sh; \
cibuildwheel --platform linux sdks/python; \

build-guest.sh failure is masked by cibuildwheel's rc, even though cibuildwheel shouldn't really proceed without a fresh guest binary.

make/dist.mk:22-28dist:c per-platform branches

@if [ "$$(uname)" = "Darwin" ]; then \
    cp target/release/libboxlite.dylib sdks/c/dist/lib/; \
    cp target/release/libboxlite.a sdks/c/dist/lib/; \
elif [ "$$(uname)" = "Linux" ]; then \
    cp target/release/libboxlite.so sdks/c/dist/lib/; \
    cp target/release/libboxlite.a sdks/c/dist/lib/; \
fi

If the dynamic-lib cp fails but the static-lib cp succeeds, dist exits 0 with a half-staged output.

My questions

  1. Is the ; chaining here intentional — e.g. wanting both crates to attempt regardless of the first crate's outcome, with rc-loss accepted as a tradeoff?
  2. If not, would you prefer && (fail-fast, simplest), or $status accumulation (both crates always run, all failures surfaced, final exit reflects any failure)? My instinct was $status for test:unit:rust (where seeing both crates' failures in one run is useful) and && for the two dist:* cases (where the downstream command can't proceed without the upstream artifact anyway). Does that match your taste?
  3. Side observation: .pre-commit-config.yaml's lint-fix and full-test-matrix hooks only have make/quality.mk in their files: regexes. So changes to make/test.mk / make/dist.mk / other makefiles bypass both hooks even after pre-commit install. Is that an intentional scope choice, or would adding make/.*\.mk to those filters fit your gating strategy?

Happy to open a PR (~13-line diff across make/test.mk and make/dist.mk) if any of this resonates — wanted to check intent first.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions