diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b484069..0e73b37 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -309,6 +309,33 @@ jobs: - name: Verify the packaged artifacts, composed (clean-install smoke) shell: bash run: python scripts/clean_install_smoke.py --skip-build + # The other half of "the artifact works": the clean-install smoke opens + # avatars through the four bundles, and nothing until 2026-08-30 had run a + # *tool* member out of an installed product. WORKSPACE.md §5 ships the + # motion profiles as product data so that this can pass, and the first run + # of it failed -- the tool lands at `tools//bin/` and looked for + # `/../share/...`, which is one directory too shallow inside the + # product's own prefix. + # + # `--product` rather than a fresh package: the archive this step is about + # is the one the previous steps just built and proved digest-reproducible, + # and re-packaging here would smoke a different artifact than the one the + # release stages. + - name: Verify the packaged BVH path from the product alone + shell: bash + run: | + set -euo pipefail + product="$(jq -r '.data.product.archive // empty' .ost-ci/package.json)" + if command -v cygpath > /dev/null; then product="$(cygpath -u "$product")"; fi + # `// empty` and this guard together, for the reason the staging step + # below has the same pair: a bare `jq -r` on a missing key prints the + # string "null", `dirname` turns that into ".", and the smoke would be + # pointed at the repository root and fail with a message about the + # wrong thing. + if [ -z "$product" ] || [ ! -f "$product" ]; then + echo "::error title=release packaging::workspace product archive is missing" ; exit 1 + fi + python scripts/artifact_only_bvh_smoke.py --product "$(dirname "$product")" - name: Verify the packaged VRMA reader independently shell: bash run: ost plugin test plugins/usdVrmaFileFormat --from-package --target ${{ matrix.platform }} --profile ${{ matrix.profile }} --up-to 5 --json diff --git a/CHANGELOG.md b/CHANGELOG.md index 6556930..888571a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,37 @@ Current schema contract version: **1**. ### Fixed +- **A tool member of the installed product could not find the product's own + data.** `motion_bvh_convert` derives its profile directory from its own + executable path, and its installed-prefix rule was + `/../share/usd-vrm-plugins/profiles/motion` — correct for a `cmake + --install` prefix and for a member archive unpacked on its own, and one + directory too shallow for the aggregate product, where `ost plugin product + install` lands a tool member at `/tools//bin/` while the + product's data goes to `/share/`. A converter that finds no profile + refuses every file it is given, so the whole BVH path was unusable from a + release artifact: the profiles shipped, byte-identical, to the directory + WORKSPACE.md §5 names, and the tool beside them looked somewhere else. The + locator now carries both installed layouts. + + Both three-parent rules — the new one and the repository one beside it, which + had always been unguarded — are now offered **only when the executable really + is in a `tools//bin/`**. From a `cmake --install` prefix they would + otherwise climb two levels *above* it, which is where a sibling install of + this product puts its own `share/`, and the result is not a refusal but a + conversion reading another prefix's profile. That is the near-miss the + no-default-profile rule exists to prevent, arriving through the search path + instead of through a flag. Caught in review before either rule shipped, and + reproduced first: an executable at `a/b/prefix/bin/` with a profile only at + `a/share/…` converted instead of refusing. + + **It was found by running it, and nothing else could have found it.** The + destination is stated in `openstrata.toml`, in the root `CMakeLists.txt`, in + WORKSPACE.md §5 and in `ProfileLocator.h`, all four agree, and one of them was + describing a different prefix. `ost` 0.22.3 supplied the staging in August and + report 36 §4 recorded in as many words that the staging was what had been + proven and not the run; this is the run, and it failed the first time. + - **A rotation too small to square came back un-normalised, in the two conversions that had not been fixed.** `GfQuatf::GetLength()` squares in float, and both `vrmAdapterVmc::ToCanonicalRotation` and @@ -130,6 +161,29 @@ Current schema contract version: **1**. ### Added +- **An artifact-only smoke for the BVH path** + (`scripts/artifact_only_bvh_smoke.py`), which closes the v0.7.0 release + condition *both paths running from release artifacts alone, profiles + included* for the recorded half. It packages the aggregate product, runs + `ost plugin product verify`, installs it to a fresh prefix **outside** this + repository, and converts a real 17-second mocopi export there — 853 frames at + 50 Hz through 22 bound joints — with no `--profile-dir`, no + `USDVRM_MOTION_PROFILE_PATH`, and nothing from this source tree on any search + path. + + Two of its checks are the ones worth naming. Every shipped profile is compared + **byte for byte** against `profiles/motion/`, because the failure that shape + replaces was a *copy* that had stopped being the file + `scripts/check_motion_profiles.py` validates. And after the conversion + succeeds the installed profile is moved aside and the same command is re-run, + which must now refuse — so "the tool found a profile" cannot pass for "the + tool found the one this product ships". + + It runs in `release.yml` beside the clean-install smoke, against the archive + that lane just proved digest-reproducible rather than a fresh package of its + own. That places it in the one workflow no PR event runs, which is a standing + caveat of that lane and not a new one. + - **`osc`, the OSC wire format once instead of once per adapter.** Packets, bundles and their flattening, addresses, type tags, arguments, and a refusal that names the byte and the address it refused at. It knows no address diff --git a/docs/architecture/WORKSPACE.md b/docs/architecture/WORKSPACE.md index f5511ae..e2ed4f3 100644 --- a/docs/architecture/WORKSPACE.md +++ b/docs/architecture/WORKSPACE.md @@ -702,9 +702,13 @@ smoke test of the BVH path impossible to pass. --install` met it.** A packaged product did not: `ost` packaged a tool member out of the `directories:` its descriptor declared, had no notion of a data-only member, and the measured `motion_bvh` archive was exactly its two executables -and its descriptor. Unpacked and run, the converter refused a real capture and -named `/share/usd-vrm-plugins/profiles/motion` as the first directory it -looked in — so the layout was agreed and only the staging was missing. Declaring +and its descriptor. Unpacked and run — a *member* archive, on its own, so the +executable sat at `/bin/` — the converter refused a real capture and +named `/share/usd-vrm-plugins/profiles/motion` as the first directory it +looked in, which read at the time as the layout being agreed and only the +staging being missing. The qualification is added in hindsight and the next +paragraph is why: that is one of two installed layouts, and the tool searched +the product's first only after 2026-08-30. Declaring `directories: [bin, share]` did stage it, and was rejected: `directories:` names subdirectories of the *member root*, so it would have put the layer's data inside one tool's directory and the copy that shipped would have stopped being the file @@ -727,13 +731,32 @@ rather than copied under any member root. The file that ships is the file `scripts/check_motion_profiles.py` validates, which is what `directories: [bin, share]` could not promise. -**What is proven is the staging, not yet the run.** The product archive carries -the profiles; nothing in this repository has yet extracted that product to a -prefix and driven `motion_bvh_convert` from it, so the artifact-only smoke this -paragraph exists to make possible is now *possible* rather than *passing*. That -distinction is the one to keep: v0.7.0 recorded the condition unmet because the -mechanism was missing, and the mechanism arriving is not the same thing as the -test existing +**The run happened on 2026-08-30 and it failed, which is why the distinction +above was worth keeping.** `scripts/artifact_only_bvh_smoke.py` packages the +product, verifies it, installs it to a prefix outside this repository, and +drives `motion_bvh_convert` there with no `--profile-dir` and no +`USDVRM_MOTION_PROFILE_PATH`. The profiles arrived exactly where this section +says — byte-identical to `profiles/motion/` — and the converter refused the +capture anyway, because `ost plugin product install` lands a tool member at +`/tools//bin/` and the locator looked at +`/../share/usd-vrm-plugins/profiles/motion`, one directory too shallow +inside the product's own prefix. + +The paragraph this replaces recorded that "the layout was agreed and only the +staging was missing", and the agreement was real but with a *different* layout: +it was measured on a member archive unpacked on its own, where the executable +does sit at `/bin/`. Two installed layouts put the data in the same place +relative to the prefix and the tool at different depths inside it, so an +executable-relative rule serves one of them at a time. The locator now carries +both, the smoke passes — 853 frames at 50 Hz, 22 bound joints, from the artifact +alone — and it proves the profile it read was the installed one by moving that +file aside and requiring the refusal to come back. + +**None of this changes the destination**, which is the part worth stating: the +contract in this section was right, `[[workspace.install_data]]` puts the files +there, and the defect was one reader of it. That is the argument for the smoke +rather than for more review — the search path was documented, the destination +was documented, the two were written from each other, and they still disagreed ([report 36](../reports/ost/36-2026-08-25-v0.22.3-canonical-runtimes-and-release-membership.md) §4). That split is the one to check when a future reader arrives: a reader is in the diff --git a/docs/contributing/RELEASE_NOTES_TEMPLATE.md b/docs/contributing/RELEASE_NOTES_TEMPLATE.md index 6f941a5..d4c83ac 100644 --- a/docs/contributing/RELEASE_NOTES_TEMPLATE.md +++ b/docs/contributing/RELEASE_NOTES_TEMPLATE.md @@ -50,10 +50,13 @@ VRMA motion clips as canonical `UsdSkelAnimation` data. > §5 keeps optional network and SDK dependencies out of the core distribution — > and they build from the source archive. > -> **No member carries data.** `motion_bvh_convert` needs a producer profile from -> `share/usd-vrm-plugins/profiles/motion/`, which reaches no archive, so run it -> from a `cmake --install` prefix or pass `--profile` a path. The profiles ship -> in the source archive. +> **The product carries the motion profiles.** `motion_bvh_convert` needs a +> producer profile, and `ost plugin product install --prefix ` puts the +> shipped ones at `/share/usd-vrm-plugins/profiles/motion/` where the +> tool finds them with no flag — verified per release by +> `scripts/artifact_only_bvh_smoke.py`. No *member* archive carries them: a +> `motion_bvh` archive unpacked on its own is its two executables, so that route +> still needs a `cmake --install` prefix or a `--profile` path. Each binary bundle carries a `buildInfo.json` stamp in its resources (git commit / build OS / compiler / build type / OpenUSD version / schema diff --git a/docs/roadmap/current.md b/docs/roadmap/current.md index f35f24d..9fca4b4 100644 --- a/docs/roadmap/current.md +++ b/docs/roadmap/current.md @@ -193,6 +193,10 @@ checks are listed rather than remembered: - [ ] the CHANGELOG names the **architecture** changes, not only the features: two shared libraries extracted, a third adapter, and every adapter's package config gaining a dependency it was missing; +- [ ] the artifact-only BVH smoke is green on **Linux and macOS**, not only on + the workstation that wrote it — it is a `release.yml` step, so a tag is + the first time those two cells run it, and the `workflow_dispatch --ref` + dry run below is what turns that from a surprise into a measurement; - [ ] `scripts/check_docs.py`, `check_motion_profiles.py` and `verify_corpus.py` are green, and `release.yml` is dry-run with `workflow_dispatch --ref` before the tag — a green PR lane proves nothing about it. @@ -228,13 +232,25 @@ checks are listed rather than remembered: adapter artifact in CI, so whether a release carries them is open ([report 36](../reports/ost/36-2026-08-25-v0.22.3-canonical-runtimes-and-release-membership.md) §2, §3). -- ⬜ **The profiles reach the product; the smoke that would prove it does not - exist.** Both halves of the staging are closed — the plain-CMake install - (2026-08-05) and the packaged one (2026-08-25, `ost` 0.22.3's - `[[workspace.install_data]]`, `data_files: 3`). **What remains is the test**: - nothing has extracted the product to a prefix and driven `motion_bvh_convert` - from it, so the entry stays open on a written smoke rather than on a missing - mechanism +- ✅ **The profiles reach the product, and the smoke that proves it found a + defect** *(2026-08-30: `scripts/artifact_only_bvh_smoke.py`)*. Both halves of + the staging were already closed — the plain-CMake install (2026-08-05) and the + packaged one (2026-08-25, `ost` 0.22.3's `[[workspace.install_data]]`, + `data_files: 3`) — and this entry stayed open on the test rather than on a + mechanism. **The first run of that test failed.** The profiles installed + byte-identically to `share/usd-vrm-plugins/profiles/motion/` and + `motion_bvh_convert` refused the capture anyway: `ost plugin product install` + lands a tool member at `/tools//bin/`, and the locator's + installed-prefix rule was `/../share/…`, which is the layout of a *member + archive* unpacked on its own. The fix is one more search-path rule in + [ProfileLocator.cpp](../../tools/motionBvh/src/ProfileLocator.cpp); the run is + 853 frames at 50 Hz through 22 bound joints, from the artifact alone, and it + ends by moving the installed profile aside and requiring the refusal to come + back — so "it found *a* profile" cannot pass for "it found the one this + product ships". Wired into `release.yml` beside the clean-install smoke, on + all three of that lane's cells — which means it inherits that lane's standing + caveat: **no PR event runs it**, so the only host it has been measured on is a + Windows workstation, and the first Linux and macOS runs happen at a tag ([report 36](../reports/ost/36-2026-08-25-v0.22.3-canonical-runtimes-and-release-membership.md) §4). ### Carried out of v0.7.0 — evidence an operator produces @@ -258,13 +274,16 @@ None of these closes by writing code, and each is stated with what it costs. with hashes, every measured statistic and no bytes — a session is a real person's motion and a skeleton packet is a body measurement of that person. Getting a publishable one needs the vendor's `BVH Sender`, not a device. -- ⬜ **Both paths running from release artifacts alone, profiles included.** - No longer blocked on the toolchain. Every member the two paths need is in the - product, and as of `ost` 0.22.3 so are the profiles. `mocopi_record` is not — - it left the aggregate by declaration when the exclusion stopped being a - version pin — so this run composes the product with the adapter's own artifact, - which `ost library package` can now produce. It stays open because the run has - not been performed. +- 🚧 **Both paths running from release artifacts alone, profiles included.** + **The recorded path is done** (2026-08-30): `motion_bvh_convert` converts a + real mocopi export from an installed product prefix with nothing from this + source tree on any search path, and the profiles it uses are the product's own + — see the packaging entry above. What is left is the **live** path, and it is + left for the reason it always was rather than a new one: `mocopi_record` is + not in the aggregate — it left by declaration when the exclusion stopped being + a version pin — so that run composes the product with the adapter's own + artifact, which `ost library package` can now produce, and nothing has + performed it. ### Still Motion Phase G diff --git a/docs/roadmap/recorded-motion-sources.md b/docs/roadmap/recorded-motion-sources.md index ba8b446..78f0906 100644 --- a/docs/roadmap/recorded-motion-sources.md +++ b/docs/roadmap/recorded-motion-sources.md @@ -587,22 +587,29 @@ Still open: characterisation, so a later rule has to change that test before it changes the behaviour, and `motion_retarget` names the bone on stderr rather than losing it in silence. -- ⬜ **The profiles reach a packaged product; the artifact-only smoke does not - exist.** Both halves of the staging are closed — the root project installs - `profiles/motion/*.yaml` to `share/usd-vrm-plugins/profiles/motion/` - (2026-08-05, verified against a scratch prefix), and `ost` 0.22.3's - `[[workspace.install_data]]` gives the mapping a product-level owner, with the - aggregate reporting `data_files: 3` +- ✅ **The profiles reach a packaged product and the artifact-only smoke runs it** + *(2026-08-30, `scripts/artifact_only_bvh_smoke.py`)*. Both halves of the + staging closed first — the root project installs `profiles/motion/*.yaml` to + `share/usd-vrm-plugins/profiles/motion/` (2026-08-05, verified against a + scratch prefix), and `ost` 0.22.3's `[[workspace.install_data]]` gives the + mapping a product-level owner, with the aggregate reporting `data_files: 3` ([report 36](../reports/ost/36-2026-08-25-v0.22.3-canonical-runtimes-and-release-membership.md) §4). - Through v0.7.0 it did not: a `motion_bvh` archive was its two executables and + Through v0.7.0 neither did: a `motion_bvh` archive was its two executables and its descriptor, so a converter unpacked from a product found nothing on its executable-relative search path and refused every file it was given — the specific consequence [WORKSPACE.md §5](../architecture/WORKSPACE.md) put the profiles beside the tools to prevent, and an `ost` ask rather than something a `--profile-dir` flag closes, because "works if you pass a flag naming a directory the artifact does not contain" is not an artifact-only smoke. - **What stops BVH-3 closing is now only that nobody has extracted the product - and run the converter from it.** + + **The smoke then failed for a second reason, which was ours.** The data + arrived byte-identically and the converter still refused: an installed product + puts a tool member at `/tools//bin/`, and the locator's + installed-prefix rule assumed `/bin/`. So this entry closes on two + fixes in different layers, and the sequence is the point — the packaging ask + was real, closing it was not sufficient, and only running the thing + distinguished the two. The passing run converts the committed 17-second mocopi + export from the prefix alone: 853 frames at 50 Hz, 22 of 27 joints bound. Two measured facts worth keeping, because both are easy to assume the other way: diff --git a/scripts/artifact_only_bvh_smoke.py b/scripts/artifact_only_bvh_smoke.py new file mode 100644 index 0000000..d0b0d90 --- /dev/null +++ b/scripts/artifact_only_bvh_smoke.py @@ -0,0 +1,377 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: Apache-2.0 +"""Drive the BVH path from the *installed product* and nothing else. + +WORKSPACE.md §5 ships the motion profiles as product data — +`share/usd-vrm-plugins/profiles/motion/` — for one stated reason: *a converter +with no profile available refuses every file it is given, which would make an +artifact-only smoke test of the BVH path impossible to pass.* That sentence has +been a requirement without a test since v0.7.0. `ost` 0.22.3 supplied the +missing half of the mechanism (`[[workspace.install_data]]`, product manifest +`data_files: 3`), and report 36 §4 said in as many words that **the staging is +what was proven and not the run**. This is the run. + + ost plugin package --workspace --product -> the product dist + ost plugin product verify -> archive + every member checksum + ost plugin product install --prefix -> a fresh prefix outside the repo + /tools/motion_bvh/bin/motion_bvh_convert --profile + +What makes it an *artifact-only* run rather than another way of running the +tool: + +* the prefix is created outside the repository and the script refuses one + inside it, so nothing in this source tree can answer a lookup; +* `--profile-dir` is never passed and `USDVRM_MOTION_PROFILE_PATH` is removed + from the environment, so the profile is found by the tool's own installed + layout or it is not found at all; +* the only environment the run is given is the runtime `ost env` prints and the + activation the product itself declares in `openstrata.activation.json` — + which is what the install contract tells a consumer to use; +* the profile that answered is then *proved* to be the installed one: the file + is moved aside, the same command is re-run, and it must refuse. + +The last of those is the check worth keeping. A converter that found a profile +somewhere else on the host would pass every other assertion here, and "which +profile was used" is the one reproducible fact a conversion carries. + +Exit codes follow `clean_install_smoke.py`: 0 pass, 1 the smoke ran and an +assertion failed, 2 the harness itself is misconfigured. + +Usage: + python scripts/artifact_only_bvh_smoke.py # package, then run + python scripts/artifact_only_bvh_smoke.py --product dist/products/... + python scripts/artifact_only_bvh_smoke.py --keep +""" +from __future__ import annotations + +import argparse +import json +import os +import pathlib +import re +import shutil +import subprocess +import sys +import tempfile +from typing import NoReturn + +REPO_ROOT = pathlib.Path(__file__).resolve().parents[1] + +# The tool member and the executable inside it, as `openstrata.product.json` +# names them. Spelled out rather than discovered: a script that scanned the +# prefix for something convert-shaped would pass on the day the product stopped +# carrying this one. +TOOL_MEMBER = "motion_bvh" +TOOL_EXECUTABLE = "motion_bvh_convert" + +# Where the product's data must land. This is the contract WORKSPACE.md §5 +# states, `openstrata.toml`'s `[[workspace.install_data]]` maps to, and +# `ProfileLocator.cpp` derives from the executable's own path — three places +# that have to agree, which is why the destination is written here as a literal +# and compared rather than read out of the manifest. +PROFILE_DESTINATION = ("share", "usd-vrm-plugins", "profiles", "motion") + +# The committed export and the profile written from it, named for the reason +# `test_motion_bvh_convert.py` names them: a smoke that scanned a directory +# would pass on the day the file it is about stopped being there. +DEFAULT_BVH = ("libs/motionBvh/tests/corpus/recorded/redistributable/" + "mocopi-mobile-arm-raise-turn.bvh") +DEFAULT_PROFILE_ID = "mocopi-mobile-bvh-default-v1" + + +class Failures: + def __init__(self) -> None: + self.messages: list[str] = [] + + def check(self, condition: bool, message: str) -> bool: + if not condition: + self.messages.append(message) + return condition + + def report(self) -> int: + if not self.messages: + return 0 + for message in self.messages: + print(f"FAIL: {message}", file=sys.stderr) + return 1 + + +def run(cmd: list[str], **kw) -> subprocess.CompletedProcess: + print(f"$ {' '.join(cmd)}", flush=True) + return subprocess.run(cmd, check=True, text=True, **kw) + + +def ost_json(cmd: list[str]) -> dict: + proc = run(cmd, capture_output=True) + return json.loads(proc.stdout) + + +def fail_setup(msg: str) -> NoReturn: + """Exit 2 — the harness is misconfigured, which is not the same answer as + the product failing its own smoke.""" + print(f"SETUP: {msg}", file=sys.stderr) + raise SystemExit(2) + + +def workspace_target() -> tuple[str, str]: + """The platform and profile `openstrata.toml` requires. + + Read with a regex rather than a TOML parser so the script runs under any + Python this repository's lanes have, and read from the manifest rather than + passed in so the smoke cannot be pointed at a runtime the product was not + built against. + """ + text = (REPO_ROOT / "openstrata.toml").read_text(encoding="utf-8") + requires = re.search(r"^\[requires\]\s*$(.*?)(?=^\[|\Z)", text, + re.MULTILINE | re.DOTALL) + if requires is None: + fail_setup("openstrata.toml has no [requires] table") + body = requires.group(1) + values = {} + for key in ("platform", "profile"): + found = re.search(rf'^\s*{key}\s*=\s*"([^"]+)"', body, re.MULTILINE) + if found is None: + fail_setup(f"openstrata.toml [requires] has no {key}") + values[key] = found.group(1) + return values["platform"], values["profile"] + + +def package_product(ost: str, platform: str, profile: str) -> pathlib.Path: + """Package the workspace's aggregate product and return its dist directory. + + The tree must already be built — this is the packaging half only, exactly as + `release.yml` runs it after `ost build` and the per-bundle builds. + """ + result = ost_json([ost, "plugin", "package", "--workspace", "--product", + "--target", platform, "--profile", profile, "--json"]) + product = result["data"].get("product") + if not product: + fail_setup("`ost plugin package --workspace --product` reported no " + "product") + archive = pathlib.Path(product["archive"]) + print(f"packaged product {product['name']} {product.get('version', '')} " + f"-> {product['archive_digest']}") + return archive.parent + + +def apply_runtime_env(env: dict, ost: str, platform: str, + profile: str) -> None: + """The runtime the product was built against, as `ost env` reports it. + + Every entry is a prepend: `ost env` emits one row per path element, and a + row that replaced the caller's value would take `PATH` away from the + subprocess on Windows, where the C runtime lives on it. + """ + result = ost_json([ost, "env", platform, "--profile", profile, "--json"]) + for entry in result["data"]["env"]: + name, value = entry["name"], entry["value"] + existing = env.get(name) + env[name] = f"{value}{os.pathsep}{existing}" if existing else value + + +def apply_product_activation(env: dict, prefix: pathlib.Path) -> None: + """The product's own activation, from the file the install contract names. + + Read rather than reconstructed: the file says which variable each list of + paths belongs to, and on a POSIX host the loader variable is not `PATH`. + """ + activation_path = prefix / "openstrata.activation.json" + if not activation_path.is_file(): + fail_setup(f"the install wrote no activation file at {activation_path}") + activation = json.loads(activation_path.read_text(encoding="utf-8")) + variables = activation.get("environment", {}) + for key, paths_key in (("loader", "library_paths"), + ("plugin", "plugin_paths"), + ("python", "python_paths")): + name = variables.get(key) + if not name: + continue + values = [str(prefix / relative) + for relative in activation.get(paths_key, [])] + if not values: + continue + joined = os.pathsep.join(values) + existing = env.get(name) + env[name] = f"{joined}{os.pathsep}{existing}" if existing else joined + + +def check_profiles_installed(failures: Failures, + prefix: pathlib.Path) -> pathlib.Path: + """Every shipped profile reaches the prefix, byte for byte. + + Byte for byte rather than merely present, because the failure this replaces + was a *copy* that stopped being the file `scripts/check_motion_profiles.py` + validates (report 35 §4). A check that only counted files would have passed + against it. + """ + installed = prefix.joinpath(*PROFILE_DESTINATION) + source = REPO_ROOT / "profiles" / "motion" + if not failures.check(installed.is_dir(), + f"the product installed no {'/'.join(PROFILE_DESTINATION)}"): + return installed + authored_profiles = sorted(source.glob("*.yaml")) + # A comparison over an empty list passes having compared nothing, which is + # the shape of green this whole script exists to distrust. + if not failures.check( + bool(authored_profiles), + f"{source} holds no *.yaml, so the byte comparison below would " + f"check nothing and report success"): + return installed + for authored in authored_profiles: + shipped = installed / authored.name + if not failures.check(shipped.is_file(), + f"{authored.name} did not reach the prefix"): + continue + failures.check(shipped.read_bytes() == authored.read_bytes(), + f"{authored.name} in the prefix is not the file " + f"profiles/motion/ holds") + return installed + + +def run_converter(tool: pathlib.Path, env: dict, bvh: pathlib.Path, + profile_id: str, + output: pathlib.Path) -> subprocess.CompletedProcess: + """The one command this whole script exists to run. + + No `--profile-dir`. The caller has already removed + `USDVRM_MOTION_PROFILE_PATH` from `env`, so the two directories left in the + search order are both derived from this executable's own location — and the + executable is inside the prefix, which is outside the repository. + """ + cmd = [str(tool), str(bvh), "--profile", profile_id, + "--output", str(output)] + print(f"$ {' '.join(cmd)}", flush=True) + return subprocess.run(cmd, env=env, text=True, capture_output=True) + + +def check_the_run(failures: Failures, tool: pathlib.Path, env: dict, + bvh: pathlib.Path, profile_id: str, + installed_profiles: pathlib.Path, + work: pathlib.Path) -> None: + output = work / "artifact-only-clip.usda" + result = run_converter(tool, env, bvh, profile_id, output) + if not failures.check( + result.returncode == 0, + f"the installed converter exited {result.returncode} with no " + f"--profile-dir. This is the artifact-only condition failing:\n" + f"{result.stdout}{result.stderr}"): + return + + if not failures.check(output.is_file(), + f"the converter reported success and wrote no " + f"{output.name}"): + return + clip = output.read_text(encoding="utf-8") + failures.check(profile_id in clip, + f"the clip does not record profileId {profile_id!r}") + failures.check("SkelAnimation" in clip, + "the clip carries no SkelAnimation") + print(result.stdout, end="") + + # And the negative: the file that answered was the installed one. + # + # Without this the smoke would pass on a host that happens to have a + # profile of the same id somewhere the search order reaches, which is the + # one way "it found its profile" can be true and mean nothing. + shipped = installed_profiles / f"{profile_id}.yaml" + hidden = installed_profiles / f"{profile_id}.yaml.hidden" + if not failures.check(shipped.is_file(), + f"{shipped} is missing, so the negative check cannot " + f"run"): + return + shipped.rename(hidden) + try: + result = run_converter(tool, env, bvh, profile_id, + work / "must-not-exist.usda") + # Exit 2 alone is not enough. `convert_main.cpp` returns it for every + # refusal about the command or something it named -- a profile that + # will not parse, a profile whose id disagrees with the request, an + # output that cannot be created. So a *foreign* profile of this id that + # happened to be malformed would satisfy an exit-code-only check and + # tell us the opposite of what this check is for. The refusal has to be + # the one that says the file is not there. + not_found = f"no profile '{profile_id}' was found" + failures.check( + result.returncode == 2 and not_found in result.stderr, + f"with the installed profile moved aside the converter exited " + f"{result.returncode} and did not report {not_found!r}, so the " + f"profile it read the first time was not the one this product " + f"ships:\n{result.stdout}{result.stderr}") + finally: + hidden.rename(shipped) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--product", default=None, + help="an existing product dist directory, manifest.json " + "or .tar.zst; default packages the workspace") + parser.add_argument("--bvh", default=DEFAULT_BVH, + help="the recorded export to convert") + parser.add_argument("--profile-id", default=DEFAULT_PROFILE_ID, + help="the profile id the product must supply") + parser.add_argument("--keep", action="store_true", + help="keep the installed prefix for inspection") + parser.add_argument("--ost", default="ost", help="ost executable") + args = parser.parse_args() + + platform, profile = workspace_target() + bvh = (REPO_ROOT / args.bvh).resolve() + if not bvh.is_file(): + fail_setup(f"no BVH file at {bvh}") + + ost = args.ost + if args.product is None: + product = package_product(ost, platform, profile) + else: + product = pathlib.Path(args.product).resolve() + if not product.exists(): + fail_setup(f"no product at {product}") + + # Verify before install, in that order and both of them, because the + # install contract in `openstrata.product.json` names both and a smoke that + # skipped the first would be testing a different contract than the one a + # consumer is told to follow. + run([ost, "plugin", "product", "verify", str(product)]) + + scratch = pathlib.Path(tempfile.mkdtemp(prefix="vrm-artifact-only-")) + prefix = scratch / "prefix" + if REPO_ROOT in prefix.resolve().parents or prefix.resolve() == REPO_ROOT: + fail_setup(f"the install prefix is inside the repository: {prefix}") + + failures = Failures() + try: + run([ost, "plugin", "product", "install", "--prefix", str(prefix), + str(product)]) + + installed_profiles = check_profiles_installed(failures, prefix) + + suffix = ".exe" if os.name == "nt" else "" + tool = prefix / "tools" / TOOL_MEMBER / "bin" / (TOOL_EXECUTABLE + suffix) + if not failures.check( + tool.is_file(), + f"the product installed no {TOOL_EXECUTABLE} at {tool}"): + return failures.report() + + env = dict(os.environ) + # The whole point of the run. Its presence would make the smoke pass + # against a product that ships no profile at all. + env.pop("USDVRM_MOTION_PROFILE_PATH", None) + apply_runtime_env(env, ost, platform, profile) + apply_product_activation(env, prefix) + + check_the_run(failures, tool, env, bvh, args.profile_id, + installed_profiles, scratch) + finally: + if args.keep: + print(f"kept the installed prefix at: {prefix}") + else: + shutil.rmtree(scratch, ignore_errors=True) + + rc = failures.report() + print("artifact-only BVH smoke: " + ("PASS" if rc == 0 else "FAIL")) + return rc + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/motionBvh/README.md b/tools/motionBvh/README.md index 5d94bc9..0d38a2b 100644 --- a/tools/motionBvh/README.md +++ b/tools/motionBvh/README.md @@ -110,14 +110,28 @@ first hit wins: 1. every `--profile-dir`, in the order given 2. `USDVRM_MOTION_PROFILE_PATH`, a list in the platform's PATH separator -3. `/../share/usd-vrm-plugins/profiles/motion` — an install prefix -4. `/../../../profiles/motion` — this repository - -The third is why a packaged artifact works with no flags at all: the profiles -ship beside the tools, and a converter with none available refuses every file it -is given. A request that *is* a path is opened as given. The difference matters -in one more place: when you name an id, the file's own `id` must match it, so a -profile renamed on disk cannot make a conversion record an id it never read. +3. `/../share/usd-vrm-plugins/profiles/motion` — a `cmake --install` + prefix, where the tools land in `/bin/` +4. `/../../../share/usd-vrm-plugins/profiles/motion` — an installed + product, where `ost plugin product install` puts a tool member in + `/tools//bin/` and the product's data in `/share/` +5. `/../../../profiles/motion` — this repository + +The last two are offered only when the executable really is in a +`tools//bin/`; from anywhere else they would climb past a prefix +entirely, and finding a *different* prefix's profile is worse than finding +none. + +The third and the fourth are why an artifact works with no flags at all: the +profiles ship beside the tools, and a converter with none available refuses +every file it is given. Which of the two answers depends on how the artifact was +installed, and `scripts/artifact_only_bvh_smoke.py` is what keeps the product +half honest — it installs the product to a clean prefix and converts a real +export there with neither the flag nor the environment variable set. + +A request that *is* a path is opened as given. The difference matters in one +more place: when you name an id, the file's own `id` must match it, so a profile +renamed on disk cannot make a conversion record an id it never read. ### Report and exit status diff --git a/tools/motionBvh/src/ProfileLocator.cpp b/tools/motionBvh/src/ProfileLocator.cpp index c6af872..406396e 100644 --- a/tools/motionBvh/src/ProfileLocator.cpp +++ b/tools/motionBvh/src/ProfileLocator.cpp @@ -135,13 +135,38 @@ ProfileSearchPath(const std::vector& extraDirs) const std::filesystem::path executableDir = ExecutableDirectory(); if (!executableDir.empty()) { + // /bin/ -> /share/... : a `cmake --install` + // prefix, and a member archive unpacked on its own. directories.push_back(executableDir.parent_path() / "share" / "usd-vrm-plugins" / "profiles" / "motion"); - // tools//bin/ -> the repository root's profiles/motion. - directories.push_back(executableDir.parent_path() - .parent_path() - .parent_path() - / "profiles" / "motion"); + // /tools//bin/ -> /share/... : an + // installed product, and this repository's own build tree. The two + // installed layouts agree about where the data is relative to the + // prefix and disagree about how deep the tool sits inside it, so each + // needs its own rule. + // + // **Both of these are gated on the tool actually being inside a + // `tools//bin/`**, and the guard is load-bearing rather than + // tidiness. Climbing three parents unguarded from a `cmake --install` + // prefix — `/bin/` — lands two levels *above* the prefix, + // which is where a sibling install of this product puts its own + // `share/`. The result would not be a refusal: it would be a + // conversion reading some other prefix's profile, which is the + // "subtly misassembled rather than absent" outcome this whole path is + // shaped to prevent. Found by review on 2026-08-30, before either rule + // had shipped, and reproduced: an executable at `a/b/prefix/bin/` with + // a profile only at `a/share/...` converted instead of refusing. + const std::filesystem::path memberDir = executableDir.parent_path(); + const std::filesystem::path toolsDir = memberDir.parent_path(); + if (executableDir.filename() == "bin" && toolsDir.filename() == "tools" + && !memberDir.filename().empty()) { + const std::filesystem::path prefix = toolsDir.parent_path(); + directories.push_back(prefix / "share" / "usd-vrm-plugins" + / "profiles" / "motion"); + // tools//bin/ -> the repository root's + // profiles/motion. + directories.push_back(prefix / "profiles" / "motion"); + } } return directories; } diff --git a/tools/motionBvh/src/ProfileLocator.h b/tools/motionBvh/src/ProfileLocator.h index 1742722..8a0e9e5 100644 --- a/tools/motionBvh/src/ProfileLocator.h +++ b/tools/motionBvh/src/ProfileLocator.h @@ -37,22 +37,64 @@ // // 1. every `--profile-dir`, in the order given // 2. `USDVRM_MOTION_PROFILE_PATH`, a list in the platform's PATH separator -// 3. `/../share/usd-vrm-plugins/profiles/motion` — an install prefix -// 4. `/../../../profiles/motion` — this repository, whose tools stage +// 3. `/../share/usd-vrm-plugins/profiles/motion` — a `cmake --install` +// prefix, where the tools land in `/bin/` +// 4. `/../../../share/usd-vrm-plugins/profiles/motion` — an installed +// **product**, where `ost plugin product install` lands a tool member in +// `/tools//bin/` and the product's own data in +// `/share/` +// 5. `/../../../profiles/motion` — this repository, whose tools stage // their executables in `tools//bin/` // -// `share` in the third is literal on every platform, and that is the contract -// rather than an assumption about GNUInstallDirs: WORKSPACE.md §5 names -// `share/usd-vrm-plugins/profiles/motion/`, and both install rules that place -// these files spell it the same way for the same reason. A lookup that followed -// a configurable data directory while the packager's rule followed another would -// leave the converter finding nothing — and finding nothing means refusing every -// file it is given, which is a failure that looks like a broken build rather -// than like a misplaced directory. +// The last two are offered **only when the executable really is in a +// `tools//bin/`**, and both are absent from the search path — and from +// the refusal's list — when it is not. // -// The fourth is a convenience and is stated rather than hidden: a build tree +// **The fourth is what an artifact-only run needs, and it was missing until +// 2026-08-30.** Both installed layouts put the profiles at the same place +// relative to the *prefix* and the tool at a different depth inside it, so one +// executable-relative rule can only ever serve one of them. The third was +// written against a member archive unpacked on its own — `/bin/` +// beside `/share/` — and WORKSPACE.md §5 recorded from that measurement +// that "the layout was agreed and only the staging was missing". The staging +// then arrived (`ost` 0.22.3's `[[workspace.install_data]]`), the first run +// from an installed product was performed, and the agreement turned out to be +// with a layout the product does not use. Which is the whole argument for +// writing the smoke rather than reasoning about the destination: this file +// named `/share/usd-vrm-plugins/profiles/motion` correctly and looked +// somewhere else. +// +// `share` in the third and the fourth is literal on every platform, and that is +// the contract rather than an assumption about GNUInstallDirs: WORKSPACE.md §5 +// names `share/usd-vrm-plugins/profiles/motion/`, and every install rule that +// places these files spells it the same way for the same reason. A lookup that +// followed a configurable data directory while the packager's rule followed +// another would leave the converter finding nothing — and finding nothing means +// refusing every file it is given, which is a failure that looks like a broken +// build rather than like a misplaced directory. +// +// The fifth is a convenience and is stated rather than hidden: a build tree // that found no profile would send whoever ran it looking for a packaging bug // that is not there. +// +// **That condition is why the fourth and the fifth are guarded rather than +// unconditional.** Both climb three directories, so from a `bin/` directly +// under a prefix — the `cmake --install` layout — they reach two levels +// *above* it, which is exactly where a sibling install of this product puts +// its own `share/`. Saying "the third is tried first and a correct prefix +// answers there" is not an answer: a prefix installed without its own +// profiles is reachable, because both CMake rules that place them are guarded +// on `profiles/motion` existing. What that costs is not a refusal but a +// conversion reading another prefix's profile — a near-miss producing motion +// that is *subtly misassembled rather than absent*, which is the outcome the +// no-default-profile rule exists to prevent, arriving through the search path +// instead of through a flag. The guard was added on review, before either rule +// shipped, after reproducing it: an executable at `a/b/prefix/bin/` with a +// profile only at `a/share/…` converted instead of refusing. +// +// It tightens the fifth as well, which had always climbed unguarded. In this +// repository the executable is at `tools//bin/` and the rule is +// unchanged; outside one it now declines to guess. #pragma once #include