Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,9 @@ working on the Rust portions of the project:
```

validating formatting across the Rust workspace without modifying files.
- `make lint` executes its Rustdoc and Clippy steps through
`RUST_DEBUG_CARGO`, then runs Whitaker separately:
- `make lint` runs `lint-clippy`, `lint-whitaker`, and spelling in sequence.
Its Rustdoc and Clippy leaf runs through `RUST_DEBUG_CARGO`; Whitaker is a
separate fragment-free leaf:

```sh
cd rust && RUSTDOCFLAGS="$(RUSTDOC_FLAGS)" \
Expand Down
46 changes: 39 additions & 7 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,23 @@ TEST_JOBS ?= 1
TEST_FLAGS ?= $(CARGO_FLAGS) --jobs $(TEST_JOBS)
DOCTEST_FLAGS ?= --workspace --doc --all-features $(BUILD_JOBS)
TEST_RUSTFLAGS ?= $(RUST_FLAGS) -C codegen-units=1
WHITAKER_CARGO_FLAGS ?= $(CARGO_FLAGS) --jobs 1
WHITAKER_RUSTFLAGS ?= $(RUST_FLAGS) -C codegen-units=1
# Whitaker's `--all` selects its lint libraries, rather than Cargo packages.
# Keep the package boundary explicit and Makefile-owned so every Rust member is
# checked even when the wrapper's library-selection semantics change.
override WHITAKER_PACKAGES := cuprum-rust cuprum-streams cuprum-native-io

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate the fixed package list against the workspace

When another member is added to rust/Cargo.toml, this independent hard-coded list remains unchanged, so lint-whitaker silently skips the new package while test_whitaker_target_covers_the_exact_workspace_packages still passes because its _PACKAGES tuple duplicates the same stale three names. Compare this list with Cargo workspace metadata in the contract test, or derive the arguments from that metadata, so the promised every-target lint gate cannot drift.

AGENTS.md reference: AGENTS.md:L219-L220

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in b3e9d05. This one I could reproduce exactly as described, and the pre-fix result is worth recording because it is the whole finding:

# workspaces members -> 4, ahead of the fix (HEAD~1 test file, literal _PACKAGES)
$ uv run ... pytest cuprum/unittests/test_whitaker_make_contract.py -q
11 passed          # <-- the drift was invisible

So a fourth member added to rust/Cargo.toml left lint-whitaker skipping it while every contract test still passed, because the test's _PACKAGES duplicated the same three names. The test was checking the Makefile against a copy of itself.

Fix: _workspace_packages() reads rust/Cargo.toml with tomllib (already a stdlib import in this tree) and returns the manifest's members. _expected_cargo_arguments() builds the expected --package … list from that, and all four assertions that previously compared against the literal now compare against the manifest. I chose to read the manifest in the test rather than derive the Makefile value from Cargo at build time: the Makefile must not shell out to Cargo to decide what to lint, and an independent reader in the test is what makes the two lists able to disagree.

Same mutation, after the fix:

$ sed -i 's/"cuprum-native-io"\]/"cuprum-native-io", "cuprum-extra"]/' rust/Cargo.toml
$ uv run ... pytest cuprum/unittests/test_whitaker_make_contract.py -q
5 failed, 7 passed   # every-package promise is now checkable

The guide sentence at docs/developers-guide.md:412 that named only WHITAKER_PACKAGES is updated to record both the full override chain and the manifest-backed check.

# Both the package list and the flags built from it are `override`n. Marking
# only the list would leave the derived flags caller-writable, and a caller
# could pass `WHITAKER_PACKAGE_FLAGS=--package evil` to displace every audited
# package while the list itself still looked intact.
override WHITAKER_PACKAGE_FLAGS := $(foreach package,$(WHITAKER_PACKAGES),--package $(package))
# The audited Whitaker command is Makefile-owned end to end. It does not forward
# `CARGO_FLAGS`: that variable is caller-overridable, so a caller could pass
# `--config ../tools/dev-fast/config.toml` after Whitaker's `--` separator and
# quietly acquire the development fragment on a route that must stay
# fragment-free. The flags below are the whole of what Whitaker may receive.
override WHITAKER_CARGO_FLAGS := $(WHITAKER_PACKAGE_FLAGS) \
--all-targets --all-features --jobs 1
# Extra flags for the `maturin develop` invocation in the `develop` target.
# Empty by default: a debug build is what contributors and the extension-tests
# job want. The benchmark ratchet needs an optimized build, and an optimized
Expand Down Expand Up @@ -201,15 +216,30 @@ SKYLOS_WHITELIST_LOCK ?= .skylos-whitelist.lock
MDLINT_FILES_FIND = bash -o pipefail -c 'git ls-files -z --cached --others --exclude-standard -- "$$@" | while IFS= read -r -d "" markdown_file; do if [ -f "$$markdown_file" ] && [ ! -L "$$markdown_file" ]; then case "$$markdown_file" in -*) printf "./%s\0" "$$markdown_file" ;; *) printf "%s\0" "$$markdown_file" ;; esac; fi; done' -- $(MARKDOWN_GLOBS)
MDLINT_FIX_COMMAND = unset FORCE_COLOR; $(LOCAL_TOOL_ENV) xargs -0 -r $(MDLINT) --fix < "$$markdown_files"
MDLINT_CHECK_COMMAND = unset FORCE_COLOR; $(LOCAL_TOOL_ENV) xargs -0 -r $(MDLINT) < "$$markdown_files"
.PHONY: help all clean build build-release lint python-lint rust-lint \
github-actions-lint \
.PHONY: help all clean build build-release lint python-lint rust-lint lint-clippy \
lint-whitaker github-actions-lint \
lint-windows fmt check-fmt \
markdownlint spelling nixie test test-python test-rust loom typecheck \
test-extension test-markdown-format develop makeutil skylos-allow \
test-dev-fast-contract dev-fast-check dev-build dev-test msrv-check \
benchmark-micro benchmark-e2e \
$(TOOLS) $(VENV_TOOLS)
.NOTPARALLEL: lint
# Serialize the lint hierarchy. Each aggregate target lists its leaves as plain
# prerequisites, so a caller's `-f` override file is honoured directly by the
# one Make process that reads it, with no forwarding to get wrong.
#
# `.WAIT` would express the ordering in the prerequisite list itself, but it is
# GNU Make 4.4-only. `.NOTPARALLEL` is not: the prereq-less form dates from Make
# 3.79, and `.NOTPARALLEL` with prerequisites from 4.4. Naming the aggregates
# here therefore works on both, and degrades safely in the direction that
# matters: Make 4.3 ignores the prerequisites and serializes the whole run,
# while Make 4.4 serializes exactly the two named subtrees. Both were verified
# against real 4.3 and 4.4 binaries, so the hosted runners' Make needs no pin.
#
# Leaves are deliberately not named here. Naming `lint` alone would leave
# `rust-lint`'s own prerequisites free to interleave under `-j`; naming both
# subtrees is what fixes the order without serializing unrelated work.
.NOTPARALLEL: lint rust-lint

.DEFAULT_GOAL := all

Expand Down Expand Up @@ -301,11 +331,13 @@ python-lint: ruff uv ## Run Ruff, interrogate, pylint, df12-python-lints, and am
$(AMBRLEAKS) cuprum/unittests scripts/tests tests
$(SKYLOS) $(SKYLOS_PRODUCTION_TARGETS) --exclude $(SKYLOS_EXCLUDE_FOLDERS) --category dead_code --gate --format concise --no-upload --no-provenance --no-grep-verify

rust-lint: $(RUST_DEBUG_PREREQUISITE) ## Run Rust documentation, Clippy, Whitaker, and spelling checks
rust-lint: lint-clippy lint-whitaker spelling ## Run Rust documentation, Clippy, Whitaker, and spelling checks

lint-clippy: $(RUST_DEBUG_PREREQUISITE) ## Run Rust documentation and Clippy
cd $(RUST_DIR) && RUSTDOCFLAGS="$(RUSTDOC_FLAGS)" $(RUST_DEBUG_CARGO) doc --no-deps $(DOC_FLAGS) && $(RUST_DEBUG_CARGO) clippy $(CLIPPY_FLAGS)
@if ! $(LOCAL_TOOL_ENV) command -v $(WHITAKER) >/dev/null 2>&1; then echo "whitaker is required for linting. Install it before running this target." >&2; exit 1; fi

lint-whitaker: ## Run Whitaker for every Rust workspace package
cd $(RUST_DIR) && $(LOCAL_TOOL_ENV) RUSTFLAGS="$(WHITAKER_RUSTFLAGS)" $(WHITAKER) --all -- $(WHITAKER_CARGO_FLAGS)
+$(MAKE) spelling

skylos-allow: export SKYLOS_SYMBOL = $(value SYMBOL)
skylos-allow: export SKYLOS_REASON = $(value REASON)
Expand Down
62 changes: 62 additions & 0 deletions cuprum/unittests/test_dev_fast_contract.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@
FRAGMENT_SHA256 = "8619efda5ea1c3232f413ae96ff56869ab6b2b7cd5bdef5a001ad16ebeac23a5"
FRAGMENT = "tools/dev-fast/config.toml"
RUST_MEMBERS = ("cuprum-rust", "cuprum-streams", "cuprum-native-io")
# The Clippy leaf's own arguments are identical on every host; only the Cargo
# route in front of them varies. `RUST_DEBUG_CARGO` prepends the dev-fast
# toolchain and its `--config` fragment on Linux and nothing at all elsewhere,
# so anchoring an order assertion on that fragment raises ValueError on macOS
# and Windows even though the leaf order under test is unchanged.
_CLIPPY_LEAF = " clippy --all-targets"
SAFE_MATURIN_FLAGS = st.sampled_from((
"--release",
"-r",
Expand Down Expand Up @@ -300,6 +306,62 @@ def test_whitaker_and_windows_lint_remain_fragment_free() -> None:
)


def test_portable_lint_orchestration_preserves_leaf_order() -> None:
"""The aggregate lint gates avoid GNU Make 4.4-only prerequisite markers."""
makefile = (repo_root() / "Makefile").read_text(encoding="utf-8")
output = _dry_run("lint")

# Strip comments before looking for `.WAIT`: the Makefile describes the
# marker in prose precisely to record why it is not used, and a raw text
# search would match that explanation instead of any real prerequisite.
directives = "\n".join(
line for line in makefile.splitlines() if not line.lstrip().startswith("#")
)
assert ".WAIT" not in directives, "hosted Make must not require GNU Make 4.4"
# `.NOTPARALLEL` with prerequisites is itself GNU Make 4.4-only, so the
# ordering cannot rest on it alone. Make 4.3 ignores the prerequisites and
# serializes the whole run instead, which preserves the order either way.
assert ".NOTPARALLEL: lint rust-lint" in directives, (
"both lint aggregates must be serialized, present on GNU Make 4.3 and 4.4"
)
assert "RECURSIVE_MAKE" not in makefile, (
"lint leaves must run in one Make process so caller `-f` files are "
"honoured without forwarding a `MAKEFILE_LIST` that cannot distinguish "
"`-f` inputs from included files"
)
python_lint = output.index("python-lint")
clippy = output.index(_CLIPPY_LEAF)
whitaker = output.index("whitaker --all --")
spelling = output.index("typos-config-builder gate --repository . --scope all")
workflow_lint = output.index("yamllint --strict --config-file")

assert python_lint < clippy < whitaker < spelling < workflow_lint, (
"lint must serialize Python, Rust leaves, and GitHub Actions validation"
)


def test_leaf_order_is_identical_on_both_cargo_routes() -> None:
"""The serialized order must not depend on which Cargo route a host selects.

The leaf order is a property of the prerequisite graph, not of the toolchain
in front of it. Asserting it on the Linux route alone would let a change
that reorders only the stable route pass unnoticed, so both selectors are
checked here.
"""
for selector in ("yes", ""):
output = _dry_run("lint", variables={"DEV_FAST_HOST_IS_LINUX": selector})
python_lint = output.index("python-lint")
clippy = output.index(_CLIPPY_LEAF)
whitaker = output.index("whitaker --all --")
spelling = output.index("typos-config-builder gate --repository . --scope all")
workflow_lint = output.index("yamllint --strict --config-file")

assert python_lint < clippy < whitaker < spelling < workflow_lint, (
f"lint must serialize its leaves identically with "
f"DEV_FAST_HOST_IS_LINUX={selector!r}"
)


def test_msrv_verification_keeps_the_stable_route_fragment_free() -> None:
"""The accelerated lint compiler cannot replace published-MSRV verification."""
output = _dry_run("msrv-check")
Expand Down
16 changes: 10 additions & 6 deletions cuprum/unittests/test_skylos_lint_contract.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,11 @@
"$${SKYLOS_REASON}",
)
_SKYLOS_WHITELIST_LOCK_TOKENS: typ.Final = (".skylos-whitelist.lock",)
_LINT_PREREQUISITES: typ.Final = (
"python-lint",
"rust-lint",
"github-actions-lint",
)
_LINT_PREREQUISITES: typ.Final = ("python-lint", "rust-lint", "github-actions-lint")
# `lint` carries no recipe: the leaves are prerequisites, and `.NOTPARALLEL`
# serializes them. Running the leaves in the same Make process is what keeps a
# caller's `-f` override file effective without forwarding `MAKEFILE_LIST`,
# which cannot distinguish `-f` inputs from files pulled in by `include`.
_DOCUMENTED_WHITELIST_NAMES: typ.Final = frozenset({"_check_rust_available"})
_RUNTIME_PARAMETER_ENTRY_POINTS: typ.Final = frozenset({
"cuprum.adapters.metrics_adapter.InMemoryMetrics.inc_counter.labels",
Expand Down Expand Up @@ -305,7 +305,11 @@ def test_lint_recipe_runs_the_production_dead_code_gate() -> None:
subject="lint target prerequisites",
)
assert lint_prerequisites == _LINT_PREREQUISITES, (
"Skylos lint delegation contract must retain the Python lint target"
"lint must sequence its leaves as prerequisites in a single Make process"
)
lint_recipes = _sole_recipe_rule("lint", require_recipes=False).get("recipes")
assert _objects(lint_recipes, subject="lint recipes") == [], (
"lint must delegate to its leaves as prerequisites, not recursive sub-makes"
)
skylos_commands = [
command
Expand Down
Loading
Loading