Repository navigation
ci(container): [#2670] build the shipped binaries without test features - #2676
Conversation
… baseline Re-verify the audit of the commands that build shipped artifacts on develop and record the baseline the later comparison needs: 1531 tests in 40 binaries (list saved), a cold release build of 4 min 59 s, a cold debug build of 1 min 55 s and a warm release rebuild of 2 min 53 s. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ped binary tracker_binary() now reads TORRUST_TRACKER_BINARY before the variables nextest and Cargo set, so the container test stages can point the black-box tests at the separately built shipped binary. Without the variable the resolution is unchanged. Three unit tests cover the order, and the test-design review and the red and green runs are recorded in the evidence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ure reaches a shipped package Add the shipped-features-check developer tool. It runs cargo tree for the edges and features of a shipped package's build (normal and build dependencies, without dev-dependencies, which cargo metadata cannot separate) and fails when a workspace package has the testing feature enabled in that tree. It follows the global CLI output contract and is tested against scratch workspaces with the real cargo tree, because no real crate has a testing feature yet. The crate is a workspace member and not a default member, is admitted in .dockerignore and is listed in the AGENTS.md catalogue and the CLI contract ADR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…hipped package Add the Shipped Features job, which runs shipped-features-check for the torrust-tracker package. The job and the Containerfile's shipped build must name the same package and features, so each names the other in a comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…able Cargo-built With TORRUST_TRACKER_BINARY the native-tracker tests can spawn the shipped binary, so the spawn failure message and the doc comment no longer claim that Cargo built it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… features The release and debug images took torrust-tracker and http_health_check from the nextest archives, built with --tests and --all-features, which would ship every testing feature once rows 3 to 7 of torrust#2482 add them. Add cook and build stages that run cargo build for the shipped binaries without a test flag or feature, take /app/bin from them, and run the native-tracker tests against the shipped binary through TORRUST_TRACKER_BINARY. The test archives keep --all-features. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… the in-container test decision Describe the test build, the shipped build, the TORRUST_TRACKER_BINARY variable and the shipped-features check in docs/containers.md, and note in ADR 20260603 that the in-container tests no longer all run the exact shipped binary. Tick AC1, AC2 and AC4. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
An empty TORRUST_TRACKER_BINARY made the native-tracker tests spawn an empty path and fail with a confusing NotFound. The empty value is now ignored, like an unset variable. Found by the independent pre-PR review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n tree output it cannot read The check printed checked and exited 0 when no line of the cargo tree output parsed as a workspace package, which a change in how Cargo prints paths would cause, and then a real violation would pass. It now fails with a runtime error unless the shipped package itself is listed as a workspace package. A checkout path that contains the separator is now read correctly, and the scratch tests pin a renamed dependency and a feature that implies testing. Found by the independent pre-PR review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e and the completion review Record the independent pre-PR review and what was done with each finding, the final image evidence (1539 tests, binaries identical to the build_shipped stage, healthy), the implementation completion review and the retrospective, and tick the checkpoints that are true. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Relative executable overrides break the existing configuration test that changes the child process's working directory.
1 open issue
What changed in this PR
This PR separates shipped container binaries from test-enabled builds, preparing for workspace testing features under #2482 while preserving in-container testing.
Changes:
- Adds dedicated release and debug builds and runs native-tracker tests against the shipped executable.
- Introduces
shipped-features-checkand a CI job to detect forbidden workspace features. - Updates container documentation, architectural records, and implementation evidence.
| File | Description |
|---|---|
tests/common/native_tracker/mod.rs |
Updates documentation and spawn wording for executable overrides. |
tests/common/native_tracker/failed_start.rs |
Updates failed-start spawn wording. |
tests/common/native_tracker/command.rs |
Adds executable override resolution and tests. |
docs/issues/open/2670-2482-build-shipped-artifacts-without-test-features/manual-verification-evidence.md |
Records build, test, and feature-check evidence. |
docs/issues/open/2670-2482-build-shipped-artifacts-without-test-features/ISSUE.md |
Tracks implementation and acceptance progress. |
docs/issues/open/2670-2482-build-shipped-artifacts-without-test-features/implementation-retrospective.md |
Records implementation lessons and limitations. |
docs/containers.md |
Explains separate builds, overrides, and feature checks. |
docs/adrs/20260603000000_keep_unit_tests_inside_container_build.md |
Amends the relationship between tested and shipped binaries. |
docs/adrs/20260519000000_define_global_cli_output_contract.md |
Registers the check's output class. |
contrib/dev-tools/checks/shipped-features-check/tests/cli.rs |
Tests the checker against scratch workspaces. |
contrib/dev-tools/checks/shipped-features-check/src/main.rs |
Implements CLI arguments, Cargo invocation, and diagnostics. |
contrib/dev-tools/checks/shipped-features-check/src/lib.rs |
Parses dependency trees and detects forbidden features. |
contrib/dev-tools/checks/shipped-features-check/Cargo.toml |
Defines the new developer-tool crate. |
Containerfile |
Builds and ships binaries separately from test archives. |
Cargo.toml |
Registers the checker as a workspace member. |
Cargo.lock |
Records the checker's dependencies. |
AGENTS.md |
Adds the checker to the developer-tool catalog. |
.github/workflows/testing.yaml |
Adds the Shipped Features job. |
.dockerignore |
Admits the checker into the container build context. |
Review effort: Balanced
… at the test working directory A relative override such as ./target/release/torrust-tracker made the configuration test that changes the child's working directory fail to spawn, because the child looks the path up inside that directory. The resolved path is now joined to the test process's working directory when it is relative and has a directory part; an absolute path and a bare name keep their behaviour. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…2676 Record the Copilot finding about relative TORRUST_TRACKER_BINARY paths and its fix, the hosted run of the Shipped Features job (V6) and the relative-path reproduction (V7), set M6 and AC3 to done and the spec status to in-review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
ACK 248f958 |
…ssues/closed e66f1f7 docs(pr-reviews): [#2670] record the second Copilot review of PR 2679 (Jose Celano) 171ed10 docs(pr-reviews): [#2670] record the review of PR 2679 (Jose Celano) f784dfb chore(issues): archive closed issue #2670 spec to docs/issues/closed (Jose Celano) Pull request description: Related to #2670 Archives the spec of #2670 (row 9 of sub-EPIC #2482), which PR #2676 implemented and closed. The spec folder (`ISSUE.md`, evidence, retrospective and the baseline test list) moves from `docs/issues/open/` to `docs/issues/closed/`, its status becomes `done`, and the remaining checkpoint boxes that are now true are ticked. Live references are repaired: row 9 of the #2482 Subissues table is DONE with a Progress Log entry, the inventory row says "Resolved by #2670", and the audits of PR #2671 and PR #2676 point at the new path. Docs only; no code, workflow or `Containerfile` change. ACKs for top commit: josecelano: ACK e66f1f7 Tree-SHA512: 4b10df990b1f1c221053e636762acf8ddff1345718c4afb25e078cd68bbd28bbb9a9458821a4a26047e1bd6d23210ada52b38503979c56863a029b97e3a54a73
da2ce7
left a comment
da2ce7
left a comment
There was a problem hiding this comment.
Reviewed at 248f95836e6592f36ff2c3739a190b983dffaeb3 (post-merge). Recomputed from the bytes at the merged head, against the base a86f7ed4f; the PR merged on 2026-10-28 at 09:34:42Z as 09263a46c.
Label: POST_MERGE_FINDINGS — Review requested 2026-10-27T21:04:18Z; the PR merged before this round ran. The findings below stand at the merged head for a follow-up; no ACK follows a merged PR.
Change: separate release/debug build stages supply the shipped executables; the archive retains all features and the native-tracker adapter selects the shipped executable first. The checker runs cargo tree with normal/build edges, parses local package paths and exact feature names, and does not inspect a binary or use metadata. Manifest inspection finds no workspace testing feature at this head; the remaining database compatibility feature only gates test modules.
Tests: 13 parser tests, 10 command-line tests and seven new adapter tests cover detection, feature selection, malformed root output, error status and override resolution. Positive leak cases reject a checker that always succeeds; dev-only and clean cases reject an overbroad checker. The default adapter tests intentionally preserve prior behavior. Execution was not available in this review.
Records and hygiene: 12 commits, 21 files, +3196/-46; merge-base confirmed. Commit subjects are conventional and identities match; attribution footers are present, with no prohibition located. The closing-pattern scan finds only the intended issue 2670 reference. Changed Markdown file targets resolve. Historical timings, image hashes and red/green execution claims cannot be independently reproduced from source alone. The audit maps F1 to the captured reply and resolved thread.
The review bot: one substantive review, one finding (F1), agreed. The merged adapter anchors directory-bearing relative paths before the child changes directory, with relative/absolute/bare-name tests. The captured reply cites the matching fix subject; no remaining instance of that reported path defect was found.
Group relationship: implements the specification from PR #2671; PR #2679 subsequently archives it. Runner storage work is operationally related but changes no feature-selection logic.
Findings: F2 and F3 are record corrections for follow-up. Checked, no finding: shipped artifact provenance through the container stage graph, exclusion of dev edges, exact feature matching, duplicate suppression, default-member exclusion and workspace registration.
Checks
On the loop's compute hub at the merged head, against the base a86f7ed4f (receipt server-gates-sojka-482; the PR merged as 09263a46c): pre-commit profile gate exit 0 (113 s); linter markdown, linter cspell, linter lychee exit 0; frontmatter-validator --all: 16 errors / 4 warnings at the merged head and 16 / 4 at the base re-measured, the same 20 diagnostics by path and category; the audit validator over docs/pr-reviews/pr-2676-review/PR-REVIEW.md with the review comments and commit list captured at gate time: ok, 1 rows, 1 log entries, 0 failures; cargo test at the merged head: shipped-features-check 23 passed, 0 failed (exit 0, 7 s; no base run); torrust-tracker 211 passed, 0 failed (exit 0, 77 s; 197 at the base); cargo clippy over shipped-features-check torrust-tracker with all targets and features and -D warnings: exit 0, 0 diagnostics. GitHub checks at this head at posting time: "Trivy: completed/success; Publish (Development): completed/skipped; Publish (Release): completed/skipped; Context: completed/success; Package Coverage Regression: completed/success; Docker E2E: completed/skipped; Documented Clippy Allows: completed/success; Unit (stable): completed/success; Workspace Packaging: completed/success; Layer Boundary Bans: completed/success; Unit (nightly): completed/success; Shipped Features: completed/success; Test (Docker) (release): completed/success; Package Coverage (torrust-tracker): completed/success; Build on macos-latest (stable): completed/success; Build on windows-latest (nightly): completed/success; Build on ubuntu-latest (stable): completed/success; Build on macos-latest (nightly): completed/success; Discover Package Coverage: completed/success; Build on windows-latest (stable): completed/success; Docs Lint: completed/success; Build on ubuntu-latest (nightly): completed/success; Security Scan: completed/success; Generate Coverage Report: completed/success". At posting, develop is 968099d93.
| - 2026-10-27 20:13 UTC - AI assistant (Copilot SDK in VS Code, #2482 subissue 9) - T5 is done: the `Containerfile` has shipped cook and build stages for the release and debug images (`cargo build` without test flags or features), the `test` stages run the native-tracker tests against the shipped binary through `TORRUST_TRACKER_BINARY`, and `/app/bin` takes the shipped binaries. Both targets build; 0 of the 1531 baseline tests are missing and 6 were added (T2's tests); cold builds cost +17 s (release) and +18 s (debug) and the warm release rebuild +8 s (V2). The override is honoured: pointing it at a missing file fails the native-tracker tests (V3). An interrupted warm run and a fully cached rerun were discarded and the warm run repeated with a different change. | ||
| - 2026-10-27 20:14 UTC - AI assistant (Copilot SDK in VS Code, #2482 subissue 9) - T6 is done: `docs/containers.md` describes the test build, the shipped build, `TORRUST_TRACKER_BINARY` and the check, and ADR 20260603 carries an amendment note that narrows its "exact binary" claim. Ticked AC1, AC2 and AC4 (validated by the AI assistant against V0 to V3). AC3 stays open until the `Shipped Features` job has run on the implementation pull request (M6). | ||
| - 2026-10-27 21:00 UTC - AI assistant (Copilot SDK in VS Code, #2482 subissue 9) - The independent Task Reviewer's pre-PR review (PASS WITH FINDINGS, no blocking defect) led to three fixes: the check now fails when `cargo tree` output does not list the shipped package as a workspace package, it reads a checkout path that contains the separator, and an empty `TORRUST_TRACKER_BINARY` is ignored; the maintainer approved the test increments through `ask_user`. Rebased onto `develop`, rebuilt the final image (1539 tests, binaries identical to the `build_shipped` stage, healthy; V2b) and ran the full pre-push suite. Wrote the implementation completion review and the retrospective. M6 and AC3 stay open until the `Shipped Features` job has run on the pull request. | ||
| - 2026-10-28 07:07 UTC - AI assistant (Copilot SDK in VS Code, #2482 subissue 9) - Opened PR #2676. The `Shipped Features` job ran green on a hosted runner (M6, V6), so AC3 and M6 are done and the status is `in-review`. Copilot's review found that a relative `TORRUST_TRACKER_BINARY` broke the configuration test that changes the child's working directory (F1 of the audit of PR #2676): reproduced (V7) and fixed by anchoring a relative path at the test working directory, which also fixes the example of `docs/containers.md`. The container workflow criterion stays open until that job is green. |
There was a problem hiding this comment.
[Minor][F2] Complete the task status when recording the hosted run
This new completion entry marks M6 and AC3 done, but the Implementation Plan still leaves T4 IN_PROGRESS (line 272), solely pending that same hosted run (line 323). Update T4 alongside the completed scenario so the task table does not report unfinished implementation after its stated condition has been met. This is anchored to the changed completion entry; the stale table row is outside this diff.
|
|
||
| 1. The spec asked for a scratch-workspace red case because no real crate has a `testing` feature | ||
| yet, so the check was proved able to fail before it had anything real to fail on. | ||
| 2. Every test was run red before green, and the negative tests that passed against the first stub |
There was a problem hiding this comment.
[Minor][F3] Narrow the claim that every test was red before green
The renamed-dependency and implied-feature tests added in the parser-hardening commit already pass with the earlier implementation: both exercise Cargo resolution and the existing exact-feature parser without changing those paths. The retrospective nonetheless says every test was run red before green. Distinguish tests that demonstrated a defect from tests that pin already-working behavior; the evidence file itself makes this distinction for these two cases.
Closes #2670
Row 9 of sub-EPIC #2482: the release and debug images no longer ship binaries built with test features, and CI fails if a workspace
testingfeature reaches a shipped package (rule A.5 of ADR20261021190318). Notestingfeature exists yet, which is why this lands before row 3 adds the first one.What changes
Containerfile: new cook and build stages (Cook Shipped,Build Shipped, release and debug) runcargo build --package torrust-trackerwithout--testsor--all-features, and/app/bintakestorrust-trackerandhttp_health_checkfrom them. The nextest archives keep--all-featuresand still build and run every test (decision D5).tests/common/native_tracker/command.rs:tracker_binary()reads a newTORRUST_TRACKER_BINARYvariable first (an empty value is ignored). The containerteststages set it, so the native-tracker black-box tests run the shipped binary.contrib/dev-tools/checks/shipped-features-check(workspace member, not a default member, admitted in.dockerignore): it readscargo tree -p <package> -e normal,buildand fails when a workspace package has itstestingfeature enabled in that tree. It follows the global CLI output contract and fails, instead of passing, when it cannot read the output.cargo metadatacould not be used because it counts features that only a dev-dependency requests; the spec records the experiment.testing.yaml: aShipped Featuresjob runs the tool fortorrust-tracker.docs/containers.mddescribes the two builds, the variable and the check; ADR20260603000000gets an amendment note because its "the exact binary ships" claim narrows (the native-tracker tests and the E2E suite still exercise the shipped binary).Evidence
The spec folder holds the evidence (
manual-verification-evidence.md), the baseline test list and the retrospective. Measured against the build before the change: 0 of the 1531 baseline tests are missing and 8 were added; cold builds cost +17 s (release) and +18 s (debug), a warm release rebuild +8 s; the binaries in the final image are byte-identical to thebuild_shippedstage output and the image reportshealthy. The check passes vacuously on the real workspace and fails on a scratch workspace in which a dependency enablestesting(red and green runs are recorded; the same cases run on every CI run intests/cli.rs).An independent pre-PR review found no blocking defect; its low-severity findings (a silent pass on unreadable
cargo treeoutput, a path containing the field separator, an empty override) are fixed in their own commits.Not in this pull request
--all-featuresstays in the test archives. Replacing it needs the feature names that rows 3 to 7 add. Acceptance criterion AC3 and scenario M6 (theShipped Featuresjob running on a hosted runner) are completed from this pull request's CI run.